505626 - Check that transform_fn is valid before executing it

Status: RESOLVED INVALID

(Core :: Graphics: Color Management, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

qcms uses a single indirect function call. I would guess that this could be a likely exploit vector so it would be nice if we could check that the address pointed to by transform_fn is something expected instead of an exploit. Unfortunately, I don't really have any good ideas about how to implement this.

Comment 1

[Benjamin Smedberg]

Do you know at compile-time what the possible functions are? Got an MXR link so interested people can see what callspot you mean?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #1)
> Do you know at compile-time what the possible functions are? Got an MXR link so
> interested people can see what callspot you mean?

Yes, the possible functions are known at compile time. They are currently the following:

qcms_transform_data_gray_out_precache;
qcms_transform_data_graya_out_precache;
qcms_transform_data_gray_out_lut;
qcms_transform_data_graya_out_lut;
qcms_transform_data_rgb_out_lut_sse;
qcms_transform_data_rgba_out_lut_sse;
qcms_transform_data_rgb_out_lut_precache;
qcms_transform_data_rgba_out_lut_precache;
qcms_transform_data_rgb_out_lut;
qcms_transform_data_rgba_out_lut;


The callspot is here:
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/transform.c#1644

Comment 3

[Benjamin Smedberg]

You could make qcms_transform.transform_fn an enum instead of a function pointer and then switch on it... this also has the advantage of being very branch-predictable by processors, where calls through functions pointers aren't.

Comment 4

[Josh Aas]

This doesn't seem to describe a serious vulnerability and a solution was proposed > 3 years ago. Is this still an issue, and if so, does it need to remain hidden?

Comment 5

[Josh Aas]

No need for this to remain private (I checked with Jeff), doesn't describe an actual vulnerability. We should do this though, if it still makes sense.

Comment 6

[Fran Rojo]

Should this bug be closed since this code/file doesn't exist anymore (converted to rust etc.). I was looking for a "good first bug" and came across this.

Comment 7

[Tom Ritter [:tjr]]

Agreed.