506207 - possible crash bug in qcms qcms_profile_from_file

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[georgi - hopefully not receiving bugspam]

this seems unreachable buggy code

iccread.c:qcms_profile_from_file

http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/iccread.c#784
783         fread(&length_be, sizeof(length), 1, file);
784         length = be32_to_cpu(length_be);
785         if (length > MAX_PROFILE_SIZE)
786                 return BAD_VALUE_PROFILE;
787 
788         /* allocate room for the entire profile */
789         data = malloc(length);
790         if (!data)
791                 return NO_MEM_PROFILE;
792 
793         /* copy in length to the front so that the buffer will contain the entire profile */
794         *((__be32*)data) = length_be;
795         remaining_length = length - sizeof(length_be);
796 
797         /* read the rest profile */
798         read_length = fread((unsigned char*)data + sizeof(length_be), 1, remaining_length, file);

the check in 785 allows |length == 0|

on 795         remaining_length = length - sizeof(length_be);

subtraction can wrap around if (say) length == 0 or length < sizeof(length_be)

then on 798 fread is passed large |size| value. exit condition in fread is the size of the file

Comment 1

[Joe Drew (not getting mail)]

Jeff, is there anything to this?

Comment 2

[Samuel Sidler (old account; do not CC)]

Jeff: Ping again? Any update here?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It looks like the bigger problem here is that we assume that the malloc'ed 'data' is large enough to store length_be. If it's 0 we could crash here.

However, if we don't crash and remaining_length wraps around to a large value then we will just ignore the profile because read_length doesn't match remaining_length.

The fix is to change the 'if (length > MAX_PROFILE_LENGTH)' check to
'if (length > MAX_PROFILE_LENGTH || length < sizeof(length_be))'. I'll try to make this into a patch tomorrow.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Ensure length is an appropriate size

Comment 5

[Jeff Muizelaar [:jrmuizel]]

The only place we use this function is when reading the user's color profile. So to even execute the code improperly you'd need to control the user's file system or their config files. Do we have any reason to keep this as a security bug?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Check the return value from fread too.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 432632 [details] [diff] [review]
Check the return value from fread too.

sizeof(length_be), and it returns # objects, not # bytes

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Let's try a third time.

- read sizeof(length_be) instead of sizeof(length)
- read in byte sized objects instead of sizeof(length)
- compare with sizeof(length_be)

Comment 9

[(no longer active)]

Comment on attachment 432636 [details] [diff] [review]
Let's try a third time.

Joe, can (should) this land?

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 432636 [details] [diff] [review]
Let's try a third time.

I don't see any reason why this shouldn't land. Jeff, it passes the qcms test suite, right? And it passes try?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

This should land. I haven't tested it recently, so I don't recall it's tested status.

Comment 12

[(no longer active)]

I've pushed it to try.

Comment 13

[(no longer active)]

The try server results were fine.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a9c54c0e8381