Closed Bug 570558 Opened 14 years ago Closed 12 years ago

build_sRGB_gamma_table(), curve_from_table() potential integer overflow, sanity check num_entries

Categories

(Core :: Graphics: Color Management, defect)

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: posidron, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: sec-other, Whiteboard: [sg:nse] no problem as used)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: 

I believe both function are never called with a user defined "num_entries" value, but perhaps this should be patched as a prevention for future uses?

http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/iccread.c?mark=448,458,462,484,489,493,497,498,#448


Reproducible: Didn't try
Are you thinking of integer overflow of the size passed to malloc?
Yes
Eg. curve_from_table() -> 4+2+0x7fffffff
So these functions aren't really meant for with num_entries as large as that. Perhaps that burden shouldn't be on the callers, but I'm not really convinced of that.
The first is only called once, with hardcoded num_entries:

655         table = build_sRGB_gamma_table(1024);

curve_from_table() is only called from qcms_profile_create_rgb_with_table(), and that too is only called with a hardcoded num_entries:

660         profile = qcms_profile_create_rgb_with_table(D65, Rec709Primaries, table, 1024);

So for now there's no possible overrun here.

I think build_sRGB_gamma-table() is fine even if you assume malicious callers. num_entries turns out to be signed, and if it's the biggest positive int the multiplication doesn't overflow. If you pass a negative, like 0x80000002, then it could overflow and allocate a very small buffer, but then "i < num_entries" is false so it can't do any damage.

Looks like there really is an overflow possible in curve_from_table because the malloc includes room for sizeof(struct curveType). If num_entries is the max positive int then it overflows, and because num_entries is positive the loop would run and stomp on memory. Again, can't happen in our current Firefox qcms code because there's only the one caller of curve_from_table() with a hardcoded safe num_entries, but maybe an issue if someone reuses this code somewhere.

We should add a sanity check on num_entries, or if these are truly strictly internal methods with perf impact maybe assert num_entries == 1024 so it fires if the context changes at all.

various static-analysis checkers will likely complain about this so we might as well fix it and not have to deal with duplicate reports in the future.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: build_sRGB_gamma_table(), curve_from_table() can write out of bounds → build_sRGB_gamma_table(), curve_from_table() potential integer overflow, sanity check num_entries
Whiteboard: [sg:nse] no problem as used
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
cdiehl: how did this get fixed? Did it just "go away" or do you know a specific patch fixed it?
Flags: needinfo?(cdiehl)
I re-checked the calls in our code for buildsRGB_gamma_table() and curve_from_table() and we are in fact calling those functions with a const values only.

New URL: http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/iccread.c?mark=786,#776

newton:mc-asan cdiehl$ rgrep build_sRGB_gamma_table .
./qcms/iccread.c:776:static uint16_t *build_sRGB_gamma_table(int num_entries)
./qcms/iccread.c:985:	table = build_sRGB_gamma_table(1024);


newton:gfx cdiehl$ rgrep curve_from_table .
./qcms/iccread.c:817:static struct curveType *curve_from_table(uint16_t *table, int num_entries)
./qcms/iccread.c:904:	profile->redTRC = curve_from_table(table, num_entries);
./qcms/iccread.c:905:	profile->blueTRC = curve_from_table(table, num_entries);
./qcms/iccread.c:906:	profile->greenTRC = curve_from_table(table, num_entries);
newton:gfx cdiehl$ nano +904 qcms/iccread.c 
newton:gfx cdiehl$ rgrep qcms_profile_create_rgb_with_table .
./qcms/iccread.c:889:qcms_profile* qcms_profile_create_rgb_with_table(
./qcms/iccread.c:990:	profile = qcms_profile_create_rgb_with_table(D65, Rec709Primaries, table, 1024);


I should have perhaps used WFM or Invalid not Fixed. Sorry.
Flags: needinfo?(cdiehl)
Group: core-security
Resolution: FIXED → WORKSFORME
You need to log in before you can comment on or make changes to this bug.