Re: November Report
Hi Brian,
On Tue, Nov 21, 2017 at 08:12:16AM +1100, Brian May wrote:
> In November I spent my 10 hours mainly working on CVE-2017-9935 /
> tiff. I have understood the problem and I have changes that should fix
> the problem now, that I am currently testing. A multi image tiff
> function can have multiple transfer functions, but we assume it only has
> one.
>
> My current fix involves checking the N value for the transfer table, and
> aborting with an error if it changes.
>
I think that this is the "least bad" approach. I have reviewed your
patch and I believe that I understand the changes that you made. Given
that none of the available approaches that would be candidates for a
security fix are particularly "good," your approach seems the most
sensible and produces a clear unambiguous error message.
> In coming to this point, I discovered two additional problems:
>
> * Pointers to transfer tables are used even though they may not have
> been initialized. I have posted patch to fix this.
>
That definitely seems like a problem that should be fixed.
> * The transfer tables themselves appear to be arrays of unsigned 16 bit
> integers. We define the pointer, however, as pointing to a
> float. Which looks wrong.
>
I puzzled over this. A float is 4 bytes, allowing it to easily
accommodate the uint16 values that are in the specification. The only
use of the transfer functions that I was able to identify was in boolean
conditions, so it is entirely possible that they are never actually used
by the tiff2pdf utility. If the transfer functions are never used or
modified, it is probably not catastrophic to use floats when the
specifiation says short. However, I wonder if coercing the transfer
function from one type to another and back (presumably) might result in
the transfer function actually being silently modified. I can see that
causing problems at some point further down the line while not being a
problem inside of the tiff2pdf utility.
> I am hesistant to fix this problem without more proof. Maybe this code
> for processing transfer functions was never tested? It is starting to
> seem like that.
At first I thought that the code must work or else someone would have
complained. However, after looking at the code and your patch closely
and then re-reading your comments I am leaning more in the direction
that this code must have never been tested.
> I have left feedback on the upstream bug report, however
> so far not received any responses.
>
That is lamentable because I think some sort of feedback from upstream
would be helpful and I would be hesitant to implement this change
without some form of review and/or concurrence from upstream.
The severity of the problem seems to be not that high, and the bug
report is nearly six months with practically no response from upstream.
It might be best to clean up the patch, post it to BugZilla and give it
some time for others to comment.
> My fix for the first problem should ideally check the transfer function
> tables are identical, however I left this off for now due to the
> confusion over what type this data is.
>
> As I am out of hours for this month, if anybody would like to take over,
> please let me know and I will present you with all my work. Otherwise I
> will continue next month.
I suspect that you have a far superior understanding of this than I do,
so I am happy to turn this back over to you if you would like,
especially since the patch is yours. If you need an additional review
of the final patch or any other assistance, let me know and I will be
glad to help.
Regards,
-Roberto
--
Roberto C. Sánchez
Reply to: