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

Avoid negative integer overflow when filesize < io_->tell() #797

Conversation

kevinbackhouse
Copy link
Collaborator

This fixes #791.

I added a new utility function to BasicIO to make it more convenient to read rcount bytes and throw an exception if not enough bytes are available. I called it readOrThrow because I couldn't think of a good name for it.

@kevinbackhouse kevinbackhouse force-pushed the bugfix_791_WebPImage_decodeChunks branch from 8d677ef to 7265293 Compare April 26, 2019 09:22
@piponazo piponazo requested review from D4N and piponazo April 26, 2019 17:05
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

Very nice idea! Ideally we could start replacing step by step the old usages of read by this new readOrThrow so that we are always sure about checking the whether the read operation succeed.

I would even vote for not having the 3rd argument and always throw the same kind of error (Exiv2::kerCorruptedMetadata) .

I would also like to see @D4N 's opinion in this matter.

@D4N
Copy link
Member

D4N commented Apr 27, 2019

I like this new API and will follow up with a proper review hopefully in the next few days. I have two comments:

  • imho this is quite an API change for the 0.27 maintenance branch, not sure if we should introduce this
  • readOrThrow should imho replace read completely, except for rare cases where we don't want an exception.

@clanmills
Copy link
Collaborator

I agree with @D4N Please don't make API changes on 0.27-maintenance.

In fact, I should open an issue on myself to investigate if we made API changes in 0.27.1 and I'll update the release process to verify that we haven't introduced changes. The aim of the v0.27-dots is to avoid API changes so security fixes can be provided by swapping DLLs without recompiling applications.

@piponazo
Copy link
Collaborator

I also agree with @D4N comments:

  1. I did not think about the API change, but it is true. We should not change the API in 0.27-maintenance.
  2. We could keep the read function with the same signature but changing the implementation for always checking if the read was successful or not. In principle it should not break anything and it should make the code safer against some of these cooked POCs.

@clanmills
Copy link
Collaborator

Can we use a macro or template to implement readOrThrow(), then we don't change the API?

@kevinbackhouse kevinbackhouse force-pushed the bugfix_791_WebPImage_decodeChunks branch from 7265293 to d05beb4 Compare April 30, 2019 08:26
@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #797 into 0.27-maintenance will increase coverage by 0.01%.
The diff coverage is 88%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #797      +/-   ##
====================================================
+ Coverage             62.68%   62.69%   +0.01%     
====================================================
  Files                   156      156              
  Lines                 21545    21554       +9     
====================================================
+ Hits                  13505    13514       +9     
  Misses                 8040     8040
Impacted Files Coverage Δ
include/exiv2/webpimage.hpp 100% <ø> (ø) ⬆️
src/webpimage.cpp 67.51% <88%> (+0.76%) ⬆️

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 db87075...51e5d69. Read the comment docs.

@kevinbackhouse
Copy link
Collaborator Author

I changed this to avoid the API change by making readOrThrow a static function in webpimage.cpp. I also posted #813, which adds the API change on master.

@clanmills clanmills added this to the v0.27.2 milestone Apr 30, 2019
@kevinbackhouse kevinbackhouse force-pushed the bugfix_791_WebPImage_decodeChunks branch from 5ee52b2 to d3c3f85 Compare May 2, 2019 09:17
@piponazo
Copy link
Collaborator

piponazo commented May 2, 2019

Hi @kevinbackhouse , as I commented in #813 the build on windows is not passing:

  C:\projects\exiv2-wutfp\src\webpimage.cpp(524): warning C4018: '<=': signed/unsigned mismatch [C:\projects\exiv2-wutfp\build\src\exiv2lib.vcxproj]

If you do not have a windows development environment let me know and I'll take a look at it.

@kevinbackhouse
Copy link
Collaborator Author

@piponazo: I have pushed a commit to fix this. The problem is that long is only 32 bits on Windows. Which is a problem because it means that a uint32_t could overflow when cast to long. So I have changed all the types to long but also added extra checks for overflow.

D4N
D4N previously requested changes May 2, 2019
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me for 0.27-maintenance, I've added a few notes concerning the overflow checks.

tests/bugfixes/github/test_issue_791.py Outdated Show resolved Hide resolved

const uint32_t filesize = Exiv2::getULong(data + WEBP_TAG_SIZE, littleEndian) + 8;
enforce(filesize <= io_->size(), Exiv2::kerCorruptedMetadata);
const long filesize = Exiv2::getULong(data + WEBP_TAG_SIZE, littleEndian) + 8;
Copy link
Member

Choose a reason for hiding this comment

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

See my comment in #813, use Safe::add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

readOrThrow(*io_, chunkId.pData_, WEBP_TAG_SIZE, Exiv2::kerCorruptedMetadata);
readOrThrow(*io_, size_buff, WEBP_TAG_SIZE, Exiv2::kerCorruptedMetadata);
const long size = Exiv2::getULong(size_buff, littleEndian);
enforce(0 <= size, Exiv2::kerCorruptedMetadata);
Copy link
Member

Choose a reason for hiding this comment

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

See my comment in #813, this is relying on undefined behavior. To properly check this, you must do something like this:

const uint32_t size = Exiv2::getULong(size_buff, littleEndian);
enforce(size < std::numeric_limits<long>::max(), Exiv2::kerCorruptedMetadata);
const long long_size = size;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@kevinbackhouse kevinbackhouse force-pushed the bugfix_791_WebPImage_decodeChunks branch from 0b0fa51 to 930f903 Compare May 6, 2019 10:56
piponazo
piponazo previously approved these changes May 6, 2019
@kevinbackhouse kevinbackhouse force-pushed the bugfix_791_WebPImage_decodeChunks branch from 930f903 to 51e5d69 Compare May 12, 2019 09:48
@mergify mergify bot dismissed piponazo’s stale review May 12, 2019 09:48

Pull request has been modified.

@piponazo piponazo dismissed D4N’s stale review May 12, 2019 12:14

Most of the suggestions have been addressed

@piponazo piponazo merged commit 6a44698 into Exiv2:0.27-maintenance May 12, 2019
@kevinbackhouse kevinbackhouse deleted the bugfix_791_WebPImage_decodeChunks branch September 18, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants