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

Use readOrThrow to check error conditions of io.read() #1627

Merged
merged 1 commit into from
May 12, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

It is safer to use readOrThrow, because it checks against the scenario where reading from the file fails.

@kevinbackhouse kevinbackhouse added the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label May 11, 2021
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.

LGTM, Thanks!

@piponazo piponazo merged commit 0f9eb74 into Exiv2:0.27-maintenance May 12, 2021
@piponazo
Copy link
Collaborator

@Mergifyio backport main

@mergify
Copy link
Contributor

mergify bot commented May 13, 2021

Command backport main: success

Backports have been created

@tcullum-rh
Copy link

@kevinbackhouse how does this protect against read of uninitialized memory? I'm not intimately familiar with all of the inner workings of Exiv2, but looking at this, I see there is a length check of iIo.size() < 12, hardcoded len = 4 which seems fine. There is a read into data and data is never actually used, which seems to be just a seek basically. The only issue I could think of is that iIo could have uninitialized data somehow. I just don't see how a call to error() would prevent that, but I'm not often working on C++ either.

@piponazo
Copy link
Collaborator

Hi @tcullum-rh, I think you are right in your analysis. I did not notice the iIo.size() < 12 check at the beginning of the function, which should prevent any situation in which we try to read beyond the iOo reserved memory.

@hassec hassec added this to the v0.27.4 milestone May 21, 2021
@kevinbackhouse
Copy link
Collaborator Author

@tcullum-rh: you are right, I misdiagnosed this. I just stepped through it in gdb and figured out what's going on. The poc is a 15 byte file, which is so short that it triggers an error in isCr2Type. Normally, these "is" functions (isCr2Type, isJpegType, etc.) rewind the file back to the beginning. But it looks like most of them don't bother if they get an EOF error.

So that's why the iIo.size() < 12 isn't working: the file hasn't been rewound back to the beginning. So the subsequent calls to iIo.read get EOF errors which are ignored.

Even though I misunderstood the bug, I think my fix is still the right thing to do. readOrThrow is safer than checking iIo.size(), due to the possibility that file position isn't zero.

@clanmills clanmills mentioned this pull request May 22, 2021
@kevinbackhouse kevinbackhouse deleted the GHSA-6253-qjwm-3q4v branch September 18, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants