Security review Q&As: > 1. Who is/are the point of contact(s) for this review? Seth Fowler (:seth) > 2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.): I'm implementing a new CSS4 property, image-orientation, which allows images to be rotated and flipped manually or automatically based on EXIF data. This has resulted in two additions to the codebase that deal with untrusted data and need security review: * A parser has been added to the codebase to read EXIF data from JPEGs. (Bug 869723) * The CSS parser has been modified to support the new image-orientation property. (Bug 825771) > 3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description: Not sure to what extent this is useful, but for completeness: http://dev.w3.org/csswg/css-images-4/#the-image-orientation http://www.cipa.jp/english/hyoujunka/kikaku/pdf/DC-008-2010_E.pdf > 4. Does this request block another bug? If so, please indicate the bug number Bug 869723 and bug 825771. > 5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review? This is of normal urgency. > 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? Not that I am aware of. > 7. Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.) > * Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users? Yes, it affects all products based on Gecko. > * Are there any portions of the project that interact with 3rd party services? No. > * 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/ckoenig@mozilla.com/Security%20Review.html) and whom to invite. August 2013.

To get a complete patch stack that will build and fully implement this feature, basically just apply all the patches from bug 869723, and then all the patches from bug 825771.

:seth, could you explain the reasons why we backed off the idea to use EasyEXIF and prefer our own parsing implementation instead? What if we intend to use more features of EXIF in the future? Would we then switch to EasyEXIF or extend our own parsing implementation ergo is the current patch for the EXIF parser only a temporary solution?

(In reply to Christoph Diehl [:cdiehl] from comment #2) > :seth, FYI: the colon there is just to make it easy to search for me in Bugzilla. Seth is fine. =) > could you explain the reasons why we backed off the idea to use EasyEXIF and > prefer our own parsing implementation instead? In his review on bug 869723, as well as in discussions on IRC, Joe Drew's feedback was that EasyEXIF was of questionable code quality and it would likely be just as much work to bring it up to snuff as it would be to scrap it and write our own implementation. On reflection, I agreed with him that a reimplementation would be better. One problem with EasyEXIF's design is that it did far more than we need it to by default, and stripping out the unneeded code resulted in #if pragmas all over the place, making the code harder to follow. These changes would be hard to upstream, as well, since it isn't clear what a sensible API would be to expose this type of functionality while allowing EasyEXIF to remain 'easy'. There's also the issue that it is harder to trust code we don't thoroughly understand, and the work to thoroughly understand EasyEXIF (reading through the EXIF spec, etc.) was the bulk of the work to just reimplement it. Given the simplicity of the problem it's solving it seemed to be the best choice just to go ahead and rewrite it, especially since this tangential issue was becoming a blocker for the feature. > What if we intend to use more features of EXIF in the future? Would we then > switch to EasyEXIF or extend our own parsing implementation ergo is the > current patch for the EXIF parser only a temporary solution? We would not switch to EasyEXIF in the future. I tried to write the new parser in such a way that it would be easily extended to handle more EXIF fields. EXIF in the end is essentially a key-value store, and although I didn't write code to parse e.g. string values since we don't use any, the bulk of the code would remain exactly the same no matter how many key-value pairs we decide we care about.

per triage we need to do fuzzing on this so it's a fuzzer battle between :cdeihl and :decoder

Marking this sec-review as done. Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/a24cbd51b6f7 + https://bugzilla.mozilla.org/show_bug.cgi?id=869723 + https://bugzilla.mozilla.org/show_bug.cgi?id=825771 Notes: * The orientation field consists of a single byte value. * The EXIF parser is as of now only parsing the orientation field. * The CSS template used "from-image" in combination with the the fuzzed file. * Jesse's CSS fuzzer should iterate over the CSS property in more detail.

My DOM fuzzer shouldn't need any changes, thanks to bug 825771 adding reftests and an entry in property_database.js. But just to be sure, I added one of the reftest images to "srcTreePageFilenames".