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
Component: General → GFX: Color Management
Product: Firefox → Core
QA Contact: general → color-management
Jeff, is there anything to this?
Jeff: Ping again? Any update here?
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.
Assignee: nobody → jmuizelaar
Whiteboard: [sg:investigate] → [sg:critical?]
Created attachment 432626 [details] [diff] [review] Ensure length is an appropriate size
Attachment #432626 - Flags: review?(joe)
Attachment #432626 - Flags: review?(joe) → review+
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?
Summary: potential security bug in qcms qcms_profile_from_file → possible crash bug in qcms qcms_profile_from_file
Whiteboard: [sg:critical?] → [sg:nse]
8 years ago
Created attachment 432632 [details] [diff] [review] Check the return value from fread too.
Comment on attachment 432632 [details] [diff] [review] Check the return value from fread too. sizeof(length_be), and it returns # objects, not # bytes
Attachment #432632 - Flags: review?(joe) → review-
Created attachment 432636 [details] [diff] [review] 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)
Attachment #432636 - Flags: review?(joe) → review+
Comment on attachment 432636 [details] [diff] [review] Let's try a third time. Joe, can (should) this land?
Attachment #432636 - Flags: approval2.0?
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?
Attachment #432636 - Flags: approval2.0? → approval2.0+
This should land. I haven't tested it recently, so I don't recall it's tested status.
I've pushed it to try.
The try server results were fine.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:nse] needs landing → [sg:nse]
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.