possible crash bug in qcms qcms_profile_from_file

RESOLVED FIXED in mozilla2.0

Status

()

Core
GFX: Color Management
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: jrmuizel)

Tracking

Trunk
mozilla2.0
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse])

Attachments

(1 attachment, 2 obsolete attachments)

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
Whiteboard: [sg:investigate]
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.

Updated

8 years ago
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?
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]
Group: core-security
Created attachment 432632 [details] [diff] [review]
Check the return value from fread too.
Attachment #432626 - Attachment is obsolete: true
Attachment #432632 - Flags: review?(joe)
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 #432632 - Attachment is obsolete: true
Attachment #432636 - Flags: review?(joe)
Attachment #432636 - Flags: review?(joe) → review+
Whiteboard: [sg:nse] → [sg:nse] needs landing

Comment 9

7 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 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.

Comment 12

7 years ago
I've pushed it to try.

Comment 13

7 years ago
The try server results were fine.
Keywords: checkin-needed

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a9c54c0e8381
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:nse] needs landing → [sg:nse]
Target Milestone: --- → mozilla2.0
Group: core-security
You need to log in before you can comment on or make changes to this bug.