-
Notifications
You must be signed in to change notification settings - Fork 278
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
[main] Fix integer overflow #2179 #2181
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2181 +/- ##
==========================================
+ Coverage 63.29% 63.34% +0.04%
==========================================
Files 99 99
Lines 19591 19595 +4
Branches 9556 9560 +4
==========================================
+ Hits 12400 12412 +12
+ Misses 5117 5112 -5
+ Partials 2074 2071 -3
Continue to review full report at Codecov.
|
a8d113a
to
2a2f164
Compare
size_t pos = sizeFront; | ||
while (0 == Photoshop::locateIptcIrb(pPsData + pos, sizePsData - pos, &record, &sizeHdr, &sizeIptc)) { | ||
long nextSizeData = Safe::add<long>(static_cast<long>(sizePsData), -static_cast<long>(pos)); | ||
enforce(nextSizeData >= 0, ErrorCode::kerCorruptedMetadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is in lines 188-189 & 196-197. I ended up using Safe::add<long>
to make sure that there is not integer overflow and I also used later enforce
to make sure that the substraction is positive.
|
||
TEST(Photoshop_setIptcIrb, detectIntegerOverflow_withDataFromPOC2179) { | ||
const std::array<byte, 141> data{ | ||
0x38, 0x42, 0x49, 0x4d, 0x20, 0x20, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x38, 0x42, 0x49, 0x4d, 0x04, 0x04, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the test which is replicating the case detected by oss-fuzz. Note that the assertion is checking that an exception is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It has been detected that we need to always pass the size==4 in the call to isIrb. Probably it is better to remove that parameter and document that it is assumed for the buffer to have a length of 4 bytes.
Fix #2179
I considered adding a python test to process the POC file, but I realised that the
exiv2
application is not exercising the part of the code that has been modified. In #2179 @kevinbackhouse mentioned that in order to reproduce the issue, one needs to use thefuzz-read-print-write
application. However that application only gets compiled in the FUZZ jobs.Any ideas of how we could try to exercise that part of the code with a python test?
Other possibility would be to try to add a unit test which directly calls
Photoshop::setIptcIrb
. It might be a good opportunity for me to get more familiar with that part of the code base.