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.

Sign up for GitHub

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

Jump to bottom

Add bounds check to MemIo::seek(). #944

Merged
merged 4 commits into from Jul 15, 2019
Merged

Add bounds check to MemIo::seek(). #944

merged 4 commits into from Jul 15, 2019

Conversation

kevinbackhouse
Copy link
Collaborator

Quick fix for bug reported in #943.

@codecov
Copy link

codecov bot commented Jul 3, 2019 •
edited

Codecov Report

Merging #944 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #944      +/-   ##
==========================================
+ Coverage   70.95%   71.12%   +0.17%     
==========================================
  Files         147      148       +1     
  Lines       19288    19323      +35     
==========================================
+ Hits        13685    13743      +58     
+ Misses       5603     5580      -23
Impacted Files Coverage Δ
unitTests/test_basicio.cpp 100% <100%> (ø)
src/basicio.cpp 37.41% <100%> (+0.24%) ⬆️
src/mrwimage.cpp 58.2% <0%> (+36.67%) ⬆️

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 555d801...27c8eec. Read the comment docs.

@D4N
Copy link
Member

D4N commented Jul 3, 2019

@kevinbackhouse Thanks for the quick fix, the changes look good to me. Would you mind adding a unit test that detects regressions in this behavior?

@kevinbackhouse
Copy link
Collaborator Author

Yes, I'll add a test.
I was also thinking that it might be a good idea to add an error flag to MemIo::Impl, so that we can implement MemIo::error() properly. A lot of the clients of this class call error() immediately after seek() to check if anything went wrong. Right now, MemIo::error() always returns 0.

@kevinbackhouse
Copy link
Collaborator Author

I added a unit test. I also changed the bug fix so that it sets the eof_ flag when you seek out of bounds. I decided against adding an error flag because I am not sure what it would mean.

piponazo
piponazo previously approved these changes Jul 9, 2019
View reviewed changes
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.

Thanks a lot for your contributions!

I left two minor suggestions but I am happy with the PR and we could merge it as it is.

unitTests/test_basicio.cpp Outdated
@@ -0,0 +1,24 @@
#include <exiv2/basicio.hpp>

#include <cmath>
Copy link
Collaborator

Choose a reason for hiding this comment

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

[minor] These headers seem to not be needed, right?. The only usage from standard functions that I see is memset which would be available in #include <cstring>

unitTests/test_basicio.cpp Outdated
// The seek was invalid, so the offset didn't change and this read still works.
ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp));

// Reset the file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] It seems that every time you are resetting, your aim is to make a slightly different test/check. I would propose to create different tests for each case, and try to give meaningful names to each of them.

If that's not the case or you prefer to not make more changes here let me know.

D4N
D4N reviewed Jul 10, 2019
View reviewed changes
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.

The changes look good to me, thanks @kevinbackhouse!

Concerning the unit test: I'd prefer to move these into separate functions and to use a fixture for the MemIo initialization.

D4N
D4N requested changes Jul 11, 2019
View reviewed changes
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.

Please mention CVE-2019-13504 in the commit log, in case anyone wants to cherry-pick fix.

@mergify mergify bot dismissed piponazo’s stale review July 11, 2019 08:45

Pull request has been modified.

@kevinbackhouse kevinbackhouse mentioned this pull request Jul 11, 2019
Closed
@clanmills clanmills mentioned this pull request Jul 11, 2019
Merged
@clanmills clanmills added this to the v0.27.2 milestone Jul 14, 2019
@clanmills
Copy link
Collaborator

It would be good to get this into 0.27.2.3 (RC3). To do this, it has be in 0.27-maintenance today (2019-07-14), otherwise it'll have to wait for 0.27.3 (scheduled 2019-09-30).

@D4N D4N self-requested a review July 15, 2019 11:51
@D4N D4N merged commit bd0afe0 into Exiv2:master Jul 15, 2019
mergify bot pushed a commit that referenced this pull request Jul 15, 2019
@kevinbackhouse
Add bounds check to MemIo::seek(). (#944)
983bf21 
- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0)

# Conflicts:
#	src/basicio.cpp
#	unitTests/CMakeLists.txt
@mergify mergify bot mentioned this pull request Jul 15, 2019
Merged
piponazo pushed a commit that referenced this pull request Jul 25, 2019
@kevinbackhouse @piponazo
Add bounds check to MemIo::seek(). (#944)
d9a7301 
- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0)
piponazo pushed a commit that referenced this pull request Jul 25, 2019
@kevinbackhouse @piponazo
Add bounds check to MemIo::seek(). (#944)
9a43c4f 
- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0)
piponazo pushed a commit that referenced this pull request Jul 28, 2019
@kevinbackhouse @piponazo
Add bounds check to MemIo::seek(). (#944)
da92486 
- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0)
piponazo pushed a commit that referenced this pull request Jul 28, 2019
@kevinbackhouse @piponazo
Add bounds check to MemIo::seek(). (#944)
edb4bf7 
- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0)

Additional fixes for 0.27:
- Add fix for the linux variant of MemIo::seek
- Change type of variable from unsigned to signed
piponazo added a commit that referenced this pull request Jul 28, 2019
@piponazo
Merge pull request #961 from Exiv2/mergify/bp/0.27-maintenance/pr-944
735fabf 
Add bounds check to MemIo::seek(). (bp #944)
@kevinbackhouse kevinbackhouse deleted the MemIo_seek_bounds_check branch September 18, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piponazo piponazo piponazo approved these changes

@D4N D4N D4N approved these changes

Assignees

@kevinbackhouse kevinbackhouse

Labels
None yet
Projects
None yet
Milestone
v0.27.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kevinbackhouse @D4N @clanmills @piponazo