Skip to content
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

security fixes #401

Closed
wants to merge 17 commits into from
Closed

security fixes #401

wants to merge 17 commits into from

Conversation

pgajdos
Copy link
Contributor

@pgajdos pgajdos commented Jun 14, 2019

Should fix #232 and #351.
#232 is little bit problematic, as same overflow will probably happen in applications using similar code as in exrmakepreview or exrmaketiled around setFrameBuffer(), for example kimageformats and vigra. Perhaps some other check could be introduced in library itself?

kdt3rd and others added 17 commits September 8, 2018 18:09
This puts the version numbers into one file, and the settings and
variables for building into another, that is then replicated and
conditionally included when building a standalone package.

Signed-off-by: Kimball Thurston <[email protected]>
Since these modules are binaries, not libraries, there is no need to support pkgconfig for them.
…ecifically calling out the proper Visual Studio version.
Signed-off-by: John Mertic <[email protected]>
@kdt3rd
Copy link
Contributor

kdt3rd commented Jun 15, 2019

Hey, I apologize, we needed to do a force push to fix up a couple of historical commits that were merged prior to clean up, could you rebase / cherry pick your commits against the new master and re-push this?

And thanks for trying to fix these! Once we get our history cleaned up, will start the review - I have only briefly looked, but I think there are a couple of modifications for other corner cases we should add to make the fixes more complete.

@pgajdos
Copy link
Contributor Author

pgajdos commented Jun 17, 2019

@kdt3rd, should I do right now?

@cary-ilm cary-ilm added the CVE A security vulnerability bug label Jun 18, 2019
kdt3rd added a commit to kdt3rd/openexr that referenced this pull request Jul 1, 2019
PR AcademySoftwareFoundation#401 had conflicts, and some of the checks were not in a central
location. This incorporates those changes, moving the extra range checks
to the central sanityCheck already in ImfHeader. Then adds a new utility
function for computing the pointer offsets that can prevent simple
overflow when there are large offsets from origin or widths with
subsampling.

Signed-off-by: Kimball Thurston <[email protected]>
Co-Authored-By: pgajdos <[email protected]>
@kdt3rd
Copy link
Contributor

kdt3rd commented Jul 1, 2019

Hey, it looks like we were modifying some common places. I've added a new commit on my PR #414 that should include your fixes (although actually moves the range checks you added to the common sanityCheck in ImfHeader), and then added a utility function to fix the pointer math in a few places. Could you test that it still fixes the issues you were addressing? Thanks in advance

@pgajdos
Copy link
Contributor Author

pgajdos commented Jul 15, 2019

@kdt3rd, I will happy to test. However I need compilable patch on the top of openexr-2.3.0 sources. See
https://github.com/kdt3rd/openexr/blob/address_part_232/OpenEXR/exrmaketiled/Image.h#L197
for example. w was removed by the fix.

kdt3rd added a commit to kdt3rd/openexr that referenced this pull request Jul 21, 2019
PR AcademySoftwareFoundation#401 had conflicts, and some of the checks were not in a central
location. This incorporates those changes, moving the extra range checks
to the central sanityCheck already in ImfHeader. Then adds a new utility
function for computing the pointer offsets that can prevent simple
overflow when there are large offsets from origin or widths with
subsampling.

Signed-off-by: Kimball Thurston <[email protected]>
Co-Authored-By: pgajdos <[email protected]>
kdt3rd added a commit that referenced this pull request Jul 21, 2019
PR #401 had conflicts, and some of the checks were not in a central
location. This incorporates those changes, moving the extra range checks
to the central sanityCheck already in ImfHeader. Then adds a new utility
function for computing the pointer offsets that can prevent simple
overflow when there are large offsets from origin or widths with
subsampling.

Signed-off-by: Kimball Thurston <[email protected]>
Co-Authored-By: pgajdos <[email protected]>
kdt3rd added a commit that referenced this pull request Jul 21, 2019
PR #401 had conflicts, and some of the checks were not in a central
location. This incorporates those changes, moving the extra range checks
to the central sanityCheck already in ImfHeader. Then adds a new utility
function for computing the pointer offsets that can prevent simple
overflow when there are large offsets from origin or widths with
subsampling.

Signed-off-by: Kimball Thurston <[email protected]>
Co-Authored-By: pgajdos <[email protected]>
@kdt3rd
Copy link
Contributor

kdt3rd commented Jul 21, 2019

ah, sorry, patch is fixed against master, and now merged to the release/2.3 branch. If you could validate that I pulled in (or have a more general version) of your fixes, would appreciate it, and will close this one out. Thank you again for testing and helping!

@pgajdos
Copy link
Contributor Author

pgajdos commented Jul 22, 2019

CVE-2018-18444

BEFORE

$ valgrind -q exrmultiview left poc right AllHalfValues.exr 12.exr
==11719== Invalid write of size 8
==11719==    at 0x483D604: memset (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==11719==    by 0x10C398: UnknownInlinedFun (makeMultiView.cpp:142)
==11719==    by 0x10C398: main (main.cpp:251)
==11719==  Address 0x5153c50 is 0 bytes after a block of size 16,000 alloc'd
==11719==    at 0x483750F: operator new[](unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==11719==    by 0x10DF01: UnknownInlinedFun (ImfArray.h:277)
==11719==    by 0x10DF01: TypedImageChannel<half>::resize() (Image.h:222)
==11719==    by 0x10C4E2: UnknownInlinedFun (Image.h:162)
==11719==    by 0x10C4E2: UnknownInlinedFun (Image.cpp:100)
==11719==    by 0x10C4E2: UnknownInlinedFun (makeMultiView.cpp:141)
==11719==    by 0x10C4E2: main (main.cpp:251)
==11719== 
==11719== Invalid write of size 8
==11719==    at 0x483D607: memset (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==11719==    by 0x10C398: UnknownInlinedFun (makeMultiView.cpp:142)
==11719==    by 0x10C398: main (main.cpp:251)
==11719==  Address 0x5153c58 is 8 bytes after a block of size 16,000 alloc'd
==11719==    at 0x483750F: operator new[](unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==11719==    by 0x10DF01: UnknownInlinedFun (ImfArray.h:277)
==11719==    by 0x10DF01: TypedImageChannel<half>::resize() (Image.h:222)
==11719==    by 0x10C4E2: UnknownInlinedFun (Image.h:162)
==11719==    by 0x10C4E2: UnknownInlinedFun (Image.cpp:100)
==11719==    by 0x10C4E2: UnknownInlinedFun (makeMultiView.cpp:141)
==11719==    by 0x10C4E2: main (main.cpp:251)
[..]
$

AFTER

$ valgrind  -q exrmultiview left poc right AllHalfValues.exr 12.exr
Error reading pixel data from image file "poc". Unexpected data block y coordinate.
$

CVE-2017-9111

BEFORE

$ exrmakepreview id:000087,sig:11,src:000562+000300,op:splice,rep:2 foo
Segmentation fault (core dumped)
$

AFTER

$ valgrind  -q exrmakepreview id:000087,sig:11,src:000562+000300,op:splice,rep:2 foo
Cannot read image file "id:000087,sig:11,src:000562+000300,op:splice,rep:2". Data window [ (808464432, 808464432) - (808478000, 808478000) ] offset / size will overflow pointer calculations
$

CVE-2017-9113

BEFORE

$ exrmakepreview id:000131,sig:11,src:000514+002831,op:splice,rep:16 foo
Segmentation fault (core dumped)
$

AFTER

$ valgrind  -q exrmakepreview id:000131,sig:11,src:000514+002831,op:splice,rep:16 foo
Cannot read image file "id:000131,sig:11,src:000514+002831,op:splice,rep:16". Data window [ (-858993460, -858993460) - (-858993430, -858993430) ] offset / size will overflow pointer calculations
$

CVE-2017-9115

BEFORE

$ exrmakepreview id:000104,sig:11,src:001329+000334,op:splice,rep:2 foo
Segmentation fault (core dumped)
$

AFTER

$ valgrind  -q exrmakepreview id:000104,sig:11,src:001329+000334,op:splice,rep:2 foo
Cannot read image file "id:000104,sig:11,src:001329+000334,op:splice,rep:2". Data window [ (808464384, 808464384) - (808464432, 808464432) ] offset / size will overflow pointer calculations
$

From my point of view, it is fixed in openexr-2.3.0 plus 45f9912, a7eec54 and ec64836. I would not claim me as coauthor of any of the changes, there's no line left from original patch, consider it rather as a hint.

@kdt3rd
Copy link
Contributor

kdt3rd commented Jul 22, 2019

Thanks for confirming, I believe I used your fix for the image::black function still, but all good - appreciate the help. Hopefully with the revived project under the ASWF, we will handle these kinds of things a bit more expediently in the future. Closing this one out for now.

@kdt3rd kdt3rd closed this Jul 22, 2019
@cary-ilm cary-ilm modified the milestone: v2.4.0 Aug 10, 2019
DominicJacksonBFX pushed a commit to boris-fx/mocha-openexr that referenced this pull request Jun 22, 2022
PR AcademySoftwareFoundation#401 had conflicts, and some of the checks were not in a central
location. This incorporates those changes, moving the extra range checks
to the central sanityCheck already in ImfHeader. Then adds a new utility
function for computing the pointer offsets that can prevent simple
overflow when there are large offsets from origin or widths with
subsampling.

Signed-off-by: Kimball Thurston <[email protected]>
Co-Authored-By: pgajdos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CVE A security vulnerability bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple segmentation faults CVE-2017-9110 to CVE-2017-9116
6 participants