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 infinite loop in PsdImage::readMetadata #518

Merged
merged 3 commits into from
Nov 6, 2018
Merged

Fix infinite loop in PsdImage::readMetadata #518

merged 3 commits into from
Nov 6, 2018

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Nov 4, 2018

As described in #426 , a fuzzed POC was causing an infinite loop in PsdImage::readMetadata. The reason is that an addition operator over the resourcesLength variable was causing an overflow. By the addition of the Safe::add operator we can detect such situations.

A regression test has been added.

@piponazo piponazo added the bug label Nov 4, 2018
@piponazo piponazo self-assigned this Nov 4, 2018
@clanmills
Copy link
Collaborator

This change mades good use of Dan's safe arithmetic. I'm not sure what a resourceLength is, however presumably if it's bigger that the file size, the file smells. So, I approve of the safe code and ask you to investigate resourceLength/file-size puzzle.

@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

Merging #518 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #518   +/-   ##
=======================================
  Coverage   63.59%   63.59%           
=======================================
  Files         154      154           
  Lines       20560    20560           
=======================================
  Hits        13075    13075           
  Misses       7485     7485
Impacted Files Coverage Δ
src/psdimage.cpp 83.01% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 933ce58...e4b3bdd. Read the comment docs.

@piponazo
Copy link
Collaborator Author

piponazo commented Nov 5, 2018

Can we know the file size at this point? I thought that we were reading it chunk by chunk, and therefore we did not know its total size.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

I'm thinking:

if ( resourceSize >= io_->size() ) {
   throw Error(kerCorruptedMetadata);
}

Or even:

enforce(resourceSize < io_->size(), Exiv2::kerCorruptedMetadata);

You might even need Dan's Safe arithmetic even to do the comparison as resourceSize is deliberately an extreme integer.

@piponazo
Copy link
Collaborator Author

piponazo commented Nov 5, 2018

Great, I have added also that check that makes total sense and adapted the test code.

@piponazo
Copy link
Collaborator Author

piponazo commented Nov 5, 2018

Note that these changes are also fixing the bug described in #427.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Thanks, Luis. Good Job as always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.27
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants