Skip to content

Proposed security fixes for issue #232 #233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

binarycrusader
Copy link
Contributor

@binarycrusader binarycrusader commented Jun 1, 2017

First of all, I want to be clear that image processing is not my domain of expertise.

This pull request attempts to fix all of the OpenEXR CVEs, but with some caveats:

CVE Test Data
CVE-2017-9110 id:000012,sig:11,src:000328+001154,op:splice,rep:16
CVE-2017-9111 id:000087,sig:11,src:000562+000300,op:splice,rep:2
CVE-2017-9112 id:000103,sig:11,src:002037+004745,op:splice,rep:2
CVE-2017-9113 id:000131,sig:11,src:000514+002831,op:splice,rep:16
CVE-2017-9114 id:000132,sig:11,src:000895,op:havoc,rep:32
CVE-2017-9115 id:000104,sig:11,src:001329+000334,op:splice,rep:2
CVE-2017-9116 id:000077,sig:11,src:002575,op:havoc,rep:4

The analysis of these CVEs seems incorrect/incomplete:

CVE-2017-9111, CVE-2017-9115, CVE-2017-9113, CVE-2017-9114

In particular, as far as I can tell, the root cause of the crash in those four cases is a bug in the test program (exr2aces) -- not in the OpenEXR library itself.

For example, the suggested description for CVE-2017-9111 says:

"[Suggested description]
In OpenEXR 2.2.0,
an invalid write of size 8 in the storeSSE function in
ImfOptimizedPixelReading.h could cause the application to crash or
execute arbitrary code."

http://seclists.org/fulldisclosure/2017/May/80

But that's not actually correct, the bug appears to be this (exr2aces/main.cpp):

121 dw = h.dataWindow();

123 width  = dw.max.x - dw.min.x + 1;
124 height = dw.max.y - dw.min.y + 1;
125 p.resizeErase (height, width);

127 in.setFrameBuffer (&p[0][0] - dw.min.x - dw.min.y * width, 1, width);

The lines above that calculate the width and height of the framebuffer to create do so using the dataWindow property. If we use exrheader to dump that information for the related test file, we see:

$ exrheader id:000087,sig:11,src:000562+000300,op:splice,rep:2
...
dataWindow (type box2i): (808464432 808464432) - (808478000 808464432)
dataWinl00 (type box2i): (808464432 808464432) - (808464432 808464432)
displayWindow (type box2i): (0 0) - (63 63)
displayWs0000 (type 0000)
...

Now, if you refer to the OpenEXR technical introduction, pages 5-6:

http://openexr.com/TechnicalIntroduction.pdf

You'll note that there are no specific, obvious restrictions on the placement of the display window and data window. That is, each one defines a rectangular area. The data window defines which area pixel data is available for, while the display window just defines the location of the data to display. There is nothing that requires them to overlap as far as I can tell. It's probably not particularly useful to have the displayWindow be roughly 0,0 to 0,63 and the dataWindow to be many orders of magnitude away, but it doesn't seem to be a violation of the specification.

So really, the only bad thing about this file is wildly impractical dimensions implied by the dataWindow. Technically, the maximum dimensions of an OpenEXR image are INT_32*INT_32 (box2i), so this is still within the limits from what I can see.

Now back to the bug, but with the values from exrheader substituted:

121 dw = h.dataWindow();

123 width  = 808478000 - 808464432 + 1;
124 height = 808464432 - 808464432 + 1;

Substituting the calculated width and height:

125 p.resizeErase (1, 13569);

That leads us to here; again substituting the values:

127 in.setFrameBuffer (&p[0][0] - -10969245413376, 1, width);

If you look above, it's obvious what's happening -- bad pointer math. As far as I can tell, the library itself doesn't have an issue for this particular test case; this is just bad logic in exr2aces.

@binarycrusader binarycrusader changed the title Add additional input validation in an attempt to resolve issue #232 Proposed security fixes for issue #232 Jun 1, 2017
@meshula
Copy link
Contributor

meshula commented Jun 2, 2017

Thanks for the detailed analysis!

@nickrasmussen
Copy link
Contributor

Thanks. Could you go ahead and fill out a CLA and send it in, and I'll take a look at getting this integrated. The CLAs are at the top of the documentation page:

http://www.openexr.com/documentation.html

@lgritz
Copy link
Contributor

lgritz commented Oct 23, 2017

Is this PR festering only because a CLA has not been received?
@binarycrusader, can you send (or have you already sent) a CLA as requested? This is a critical fix.

@binarycrusader
Copy link
Contributor Author

binarycrusader commented Oct 23, 2017

Unfortunately, I was not able to secure a completed CLA from my employer. I can only offer the fix under the terms of the original license (that is, Oracle, at the time, authorized me to provide the changes under the same license as OpenEXR itself, so distributors wishing to apply the patch can assume that much).

@lgritz
Copy link
Contributor

lgritz commented Oct 23, 2017

@binarycrusader: Can you explain? What do you mean by "not able." Are you not the author? And what do you mean by "under the terms of the original license?"

Please re-open the PR, this is a critical issue, let's work harder to find a way to resolve this.

@binarycrusader
Copy link
Contributor Author

binarycrusader commented Oct 23, 2017

I am no longer a representative of Oracle and as such cannot assist further since the fix was developed on their behalf (they own the copyright).

@lgritz
Copy link
Contributor

lgritz commented Oct 23, 2017

Can you provide a contact for somebody in management at Oracle who would have been the one to authorize it if you were still there, so that at least Lucasfilm can try to follow up?

@binarycrusader
Copy link
Contributor Author

Due to changes in staffing, I’m uncertain who could. I can only suggest trying to contact Alan Coopersmith: https://twitter.com/alanc

@nickrasmussen nickrasmussen reopened this Nov 14, 2017
@cary-ilm cary-ilm closed this Dec 7, 2017
@cary-ilm
Copy link
Member

cary-ilm commented Dec 7, 2017

Merged into master and v2.2fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants