Closed
Bug 506207
Opened 15 years ago
Closed 13 years ago
possible crash bug in qcms qcms_profile_from_file
Categories
(Core :: Graphics: Color Management, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0
People
(Reporter: guninski, Assigned: jrmuizel)
Details
(Whiteboard: [sg:nse])
Attachments
(1 file, 2 obsolete files)
559 bytes,
patch
|
joe
:
review+
joe
:
approval2.0+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•15 years ago
|
Component: General → GFX: Color Management
Product: Firefox → Core
Reporter | ||
Updated•15 years ago
|
QA Contact: general → color-management
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:investigate]
Comment 1•15 years ago
|
||
Jeff, is there anything to this?
Comment 2•15 years ago
|
||
Jeff: Ping again? Any update here?
Assignee | ||
Comment 3•15 years ago
|
||
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.
Updated•14 years ago
|
Assignee: nobody → jmuizelaar
Whiteboard: [sg:investigate] → [sg:critical?]
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #432626 -
Flags: review?(joe)
Updated•14 years ago
|
Attachment #432626 -
Flags: review?(joe) → review+
Assignee | ||
Comment 5•14 years ago
|
||
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?
Group: core-security
Summary: potential security bug in qcms qcms_profile_from_file → possible crash bug in qcms qcms_profile_from_file
Whiteboard: [sg:critical?] → [sg:nse]
Assignee | ||
Updated•14 years ago
|
Group: core-security
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #432626 -
Attachment is obsolete: true
Attachment #432632 -
Flags: review?(joe)
Comment 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
- read sizeof(length_be) instead of sizeof(length) - read in byte sized objects instead of sizeof(length) - compare with sizeof(length_be)
Attachment #432632 -
Attachment is obsolete: true
Attachment #432636 -
Flags: review?(joe)
Updated•14 years ago
|
Attachment #432636 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Whiteboard: [sg:nse] → [sg:nse] needs landing
Comment 9•13 years ago
|
||
Comment on attachment 432636 [details] [diff] [review] Let's try a third time. Joe, can (should) this land?
Attachment #432636 -
Flags: approval2.0?
Comment 10•13 years ago
|
||
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+
Assignee | ||
Comment 11•13 years ago
|
||
This should land. I haven't tested it recently, so I don't recall it's tested status.
Comment 12•13 years ago
|
||
I've pushed it to try.
Comment 14•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a9c54c0e8381
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:nse] needs landing → [sg:nse]
Target Milestone: --- → mozilla2.0
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•