849812 - Security Review: WBMP Image Decoder

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

1) Who is/are the point of contact(s) for this review?
2) Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
3) Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
4) Does this request block another bug? If so, please indicate the bug number
5) This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
6) To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list?  If so, which goal?
7) Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
7a) Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
7b) Are there any portions of the project that interact with 3rd party services?
7c) Will your application/service collect user data? If so, please describe
8) If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
9) Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Curtis Koenig [:curtisk] from comment #0)
> 1) Who is/are the point of contact(s) for this review?
Shih-Chiang Chien (schien)
> 2) Please provide a short description of the feature / application (e.g.
> problem solved, use cases, etc.):
Provide a WBMP image decoder for displaying .wbmp image file
> 3) Please provide links to additional information (e.g. feature page, wiki)
http://en.wikipedia.org/wiki/Wbmp
> if available and not yet included in feature description:
> 4) Does this request block another bug? If so, please indicate the bug number
> 5) This review will be scheduled amongst other requested reviews. What is
> the urgency or needed completion date of this review?
The WBMP decoder is required by FFOS v1.1, the deadline for feature completion is 3/15 and the code freeze is 4/26, I'd like to schedule the review before 3/22.
> 6) To help prioritize this work request, does this project support a goal
> specifically listed on this quarter's goal list?  If so, which goal?
Yes, this feature is required by FFOS v1.1.
> 7) Please answer the following few questions: (Note: If you are asked to
> describe anything, 1-2 sentences shall suffice.)
> 7a) Does this feature or code change affect Firefox, Thunderbird or any
> product or service the Mozilla ships to end users?
Yes, it generally adds a new supported image format.
> 7b) Are there any portions of the project that interact with 3rd party
> services?
No.
> 7c) Will your application/service collect user data? If so, please describe
No.
> 8) If you feel something is missing here or you would like to provide other
> kind of feedback, feel free to do so here (no limits on size):
> 9) Desired Date of review (if known from
> https://mail.mozilla.com/home/[e-mail]/Security%20Review.html)
> and whom to invite.
3/14 is good to me.

Comment 2

[Daniel Veditz [:dveditz]]

We don't need a group review meeting of this as a "feature" -- it's an ancient existing format and doesn't add any risks not inherent in image formats.

We _do_ need a security check of the code, probably best accomplished by fuzzing.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> We don't need a group review meeting of this as a "feature" -- it's an
> ancient existing format and doesn't add any risks not inherent in image
> formats.
> 
> We _do_ need a security check of the code, probably best accomplished by
> fuzzing.
Great, what information or test I can provide in addition to the patch?

Comment 4

[Christoph Diehl [:posidron]]

Shih-Chiang, I assume it will be possible to display a WBMP image inside Firefox on a Firefox OS device and WBMP will not be limited to MMS, right? 
In that case I could fuzz-test the format with Firefox desktop rather than using a Firefox OS device or emulator, correct?

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

yep, the patch in bug 847310 will also add the WBMP support for Firefox desktop.

Comment 6

[Christoph Diehl [:posidron]]

Great, than we also have ASan support for that component.
I will look into it today.

Comment 7

[Christoph Diehl [:posidron]]

*) Are there plans to support extended header fields in the future?

*) We have no good support for variable bit fields, which makes fuzzing this format (even if it is very simplistic) not as efficient as I would like to have it.

*) Besides that I am a bit concerned by passing e.g. 4294967289 into the mWidth field.

rowSize = (mWidth + 7) / 8;

The expression mWidth + 7 would then overflow by 1 and assigning a wrong value to rowSize. This might be not dangerours per se but in further code fragments we are using:

uint32_t *d = mImageData + (mWidth * mCurLine); 

Please correct me if I am wrong but I think we should do some more sanitizing of the mWdith and probably mHeight field. Currently, we are only checking if those fields are not equal to zero.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Christoph Diehl [:cdiehl] from comment #7)
> *) Are there plans to support extended header fields in the future?

Not going to support extended header fields since no further spec discussion on WBMP in WAP Forum. 
> 
> *) We have no good support for variable bit fields, which makes fuzzing this
> format (even if it is very simplistic) not as efficient as I would like to
> have it.

Will using std::bitset do any help? Sadly we could not use boost::dynamic_bitset.
> 
> *) Besides that I am a bit concerned by passing e.g. 4294967289 into the
> mWidth field.
> 
> rowSize = (mWidth + 7) / 8;
> 
> The expression mWidth + 7 would then overflow by 1 and assigning a wrong
> value to rowSize. This might be not dangerours per se but in further code
> fragments we are using:
> 
> uint32_t *d = mImageData + (mWidth * mCurLine); 
> 
> Please correct me if I am wrong but I think we should do some more
> sanitizing of the mWdith and probably mHeight field. Currently, we are only
> checking if those fields are not equal to zero.

I can restrict the mWidth/mHeight to a smaller value like what we did in nsBMPDecoder. This can eliminate the overflow case, right?
http://dxr.mozilla.org/mozilla-central/image/decoders/nsBMPDecoder.cpp#l242

Comment 9

[Christoph Diehl [:posidron]]

(In reply to Shih-Chiang Chien [:schien] from comment #8)
> Will using std::bitset do any help? Sadly we could not use
> boost::dynamic_bitset.

That is more a general problem of our fuzzer right now, which we will also need to fix for other file formats.

> I can restrict the mWidth/mHeight to a smaller value like what we did in
> nsBMPDecoder. This can eliminate the overflow case, right?
> http://dxr.mozilla.org/mozilla-central/image/decoders/nsBMPDecoder.cpp#l242

Looks reasonable to me.

It seems like that this is pretty much the whole file format, since we are not going to  support the extended header fields.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Christoph Diehl [:cdiehl] from comment #9)
> (In reply to Shih-Chiang Chien [:schien] from comment #8)
> > I can restrict the mWidth/mHeight to a smaller value like what we did in
> > nsBMPDecoder. This can eliminate the overflow case, right?
> > http://dxr.mozilla.org/mozilla-central/image/decoders/nsBMPDecoder.cpp#l242
> 
> Looks reasonable to me.
> 
> It seems like that this is pretty much the whole file format, since we are
> not going to  support the extended header fields.

Patch was updated in bug 847310.

Comment 11

[Christoph Diehl [:posidron]]

(In reply to Shih-Chiang Chien [:schien] from comment #10)
> Patch was updated in bug 847310.

feedback+ was given.

I believe we are done here, the format is very simplistic and would only need further testing in case we add extended header fields in the future.