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

Status: RESOLVED WORKSFORME

(Core :: Graphics: Color Management, defect)

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Are you thinking of integer overflow of the size passed to malloc?

Comment 2

[Christoph Diehl [:posidron]]

Yes
Eg. curve_from_table() -> 4+2+0x7fffffff

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Daniel Veditz [:dveditz]]

cdiehl: how did this get fixed? Did it just "go away" or do you know a specific patch fixed it?

Comment 6

[Christoph Diehl [:posidron]]

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.