1132468 - (CVE-2015-0811) [qcms] heap info leak

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[groebert]

Attachment: 98c8e33029be249da9724c3836486677

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36

Steps to reproduce:

Load sample 98c8e33029be249da9724c3836486677 or 11dcaacb804945e4574317fc56ebc5d3 in a transformation.


Actual results:

heap-buffer-overflow READ of size 4
lut_interp_linear_float third_party/qcms/src/transform_util.c:101:17

or

Program received signal SIGSEGV, Segmentation fault.
lut_interp_linear_float (value=1.35335546e+18, table=0x7ffff6a02050, length=0)
    at third_party/qcms/src/transform_util.c:101
101             value = table[upper]*(1. - (upper - value)) + table[lower]*(upper - value);

float lut_interp_linear_float(float value, float *table, size_t length)
{
        int upper, lower;
        value = value * (length - 1);

// If length is 0, value will have been adjusted to an arbitrary large float.


        upper = ceil(value);
        lower = floor(value);


        value = table[upper]*(1. - (upper - value)) + table[lower]*(upper - value);

// Here the ASAN alert or SIGSEGV occurs.

        return value;
}

Potentially this can be used for disclosing memory contents.

Comment 1

[groebert]

Attachment: 11dcaacb804945e4574317fc56ebc5d3

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch - disallow 0 length in read_tag_lutmA

Comment 3

[Benoit Girard (:BenWa)]

These sizes don't make any sense so I don't think they is any value in trying to support these profiles. Rejecting these at read time means that we avoid a lot of foot guns later.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8566813 [details] [diff] [review]
patch - disallow 0 length in read_tag_lutmA

Review of attachment 8566813 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/qcms/iccread.c
@@ +585,5 @@
>  		for (i = 0; i < num_in_channels; i++) {
>  			clut_size *= read_u8(src, clut_offset + i);
> +			if (clut_size == 0) {
> +				invalid_source(src, "bad clut_size");
> +				return NULL;

Do you need to return here or can you just let things fall through?

@@ +609,5 @@
>  	for (i = 0; i < num_in_channels; i++) {
>  		lut->num_grid_points[i] = read_u8(src, clut_offset + i);
> +		if (lut->num_grid_points[i] == 0) {
> +			invalid_source(src, "bad grid_points");
> +			return NULL;

Looks like this leaks. You shouldn't return here just mark the source as invalid.

@@ +695,5 @@
>  		num_input_table_entries  = read_u16(src, offset + 48);
>  		num_output_table_entries = read_u16(src, offset + 50);
> +		if (num_input_table_entries == 0 || num_output_table_entries == 0) {
> +			invalid_source(src, "entries is 0");
> +			return NULL;

Same here. Does anything bad happen if these values are large?

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 7

[Benoit Girard (:BenWa)]

Not sure what the process for landing a security patch is these days.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1170bf063bd

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1170bf063bd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Milan, can you please take care of what approval requests this needs? AFAICT, that'd be aurora/beta/esr31/b2g34.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8568805 [details] [diff] [review]
patch v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible info leak
Fix Landed on Version: 39
Risk to taking this patch (and alternatives if risky): Low risk.  We do a quick exit if the data is bad.
String or UUID changes made by this patch: 

Not requesting uplift to the B2G 34 - we're not actually hitting the QCMS code in B2G, so no particular need to do it.  As this hits the beta channel, it will end up in B2G 37, which shouldn't hurt it.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8568805 [details] [diff] [review]
patch v2

Taking this for aurora and beta since it seems low risk, and we're still early in beta -- but not for ESR or release since it doesn't meet the criteria for ESR31 and does not look significant enough to do a second dot release for 36.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/705946ec0d52
https://hg.mozilla.org/releases/mozilla-beta/rev/1f4073c76b2b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1f4073c76b2b