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

Fix potential PNG decompression DOS #1060

Merged
merged 5 commits into from
Dec 31, 2014
Merged

Conversation

wiredfool
Copy link
Member

@hugovk @aclark4life

Two outstanding bits:

  1. interface for the developer -- It behaves a little differently than the decompression bomb warning in that this just tosses a value error. But it's less likely to have false positives.
  2. It doesn't defend against images that have many just under the limit text segments.

Opinions?

(note that I had to change the test to pass -- I believe that the new version is also a reasonable representation of an invalid iTXt segment)

@aclark4life
Copy link
Member

Thanks @wiredfool. Re: 1, Right now I'm more concerned about preventing a DOS than providing a consistent API and re: 2, can we reasonably defend against images with many-just-under-the-limit text segments?

@wiredfool
Copy link
Member Author

That was less invasive than I'd thought. Implementation allowing 64 * maxsize total memory usage for text chunks, and a test attempting to save 128 of them. ( ~8k or so of compressed text chunks in the image would expand to ~128M)

@landscape-bot
Copy link

Code Health
Repository health increased by 0.01% when pulling 0b75526 on wiredfool:Pillow into 967247d on python-pillow:Pillow.

im = Image.open(test_file)
im.load()
except ValueError as msg:
self.assert_(msg, "Decompressed Data Too Large")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertTrue is a bit clearer (for me) than assert_ here.

@hugovk
Copy link
Member

hugovk commented Dec 30, 2014

👍 in general, with minor review notes.

Did you want test_dos_total_memory() and test_dos_total_memory() to run on Travis CI? I'm guessing not, as they're in a file named check_... and not test_.... Are they terribly slow to run? If not, can they be moved under Travis?

@wiredfool
Copy link
Member Author

They're not especially slow, and as most of the DOS ones go, it's not a problem once we've solved the issue. They may be a bit more of an issue running the tests in a resource constrained environment, so I'd rather not run them by default.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 6696b78 on wiredfool:png-dos into 967247d on python-pillow:master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6696b78 on wiredfool:png-dos into * on python-pillow:master*.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling a59eb39 on wiredfool:png-dos into 967247d on python-pillow:master.

hugovk added a commit that referenced this pull request Dec 31, 2014
Fix potential PNG decompression DOS
@hugovk hugovk merged commit b3e0912 into python-pillow:master Dec 31, 2014
pilz added a commit to syslabcom/recensio.buildout that referenced this pull request Jan 12, 2015
@wiredfool
Copy link
Member Author

Note that this has had a CVE assigned: CVE-2014-9601 (http://www.cvedetails.com/cve/CVE-2014-9601/)

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.

6 participants