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

fix(quicktimevideo) avoid out of bounds read, closes #2340 #2341

Merged
merged 3 commits into from
Aug 31, 2022
Merged

fix(quicktimevideo) avoid out of bounds read, closes #2340 #2341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8a200c
fix(quicktimevideo) avoid out of bounds read, closes #2340
hassec Aug 26, 2022
83ca845
test(quicktimevideo) add poc for #2340
hassec Aug 26, 2022
77887c7
fix(doc) mention video support in exiv2.md
hassec Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions exiv2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ and see if `enable_bmff=1`.

- Naked codestream JXL files do not contain Exif, IPTC or XMP metadata.

- Support of video files is limited. Currently **exiv2** only has some
rudimentary support to read metadata from quicktime based video files (e.g.
.MOV/.MP4).


[TOC](#TOC)

Expand Down
4 changes: 2 additions & 2 deletions src/quicktimevideo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@ void QuickTimeVideo::previewTagDecoder(size_t size) {
if (equalsQTimeTag(buf, "PICT"))
xmpData_["Xmp.video.PreviewAtomType"] = "QuickDraw Picture";
else
xmpData_["Xmp.video.PreviewAtomType"] = Exiv2::toString(buf.data());
xmpData_["Xmp.video.PreviewAtomType"] = std::string{buf.c_str(), 4};
Copy link
Collaborator

@kevinbackhouse kevinbackhouse Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I checked the documentation on this: it will copy 4 bytes even if some of them are null. So std::string::size() will return 4 even if the string is actually shorter than that, which is a bit weird. So I wonder if it would be better to increase the size of buf to 5 and put a nul-terminator in buf[4].

Copy link
Collaborator

@piponazo piponazo Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Kevin. I also looked a bit in this function, and this line looked safe to me, taking into account that in line 660 the DataBuf instance is of size 4, and in line 667 we do io_->readOrThrow(buf.data(), 4);.

What it might prevent additional changes in the future would be to do io_->readOrThrow(buf.data(), buf.size()); just in case the side of the DataBuf instance changes in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@piponazo: Yes, I think this line of code is safe. The problem that I'm talking about is only a very minor thing. This is a demo of the weird thing that can happen:

#include <string>
#include <stdio.h>

int main() {
  std::string s("hi\0\0\0", 4);
  printf("%s %lu\n", s.c_str(), s.size());
  return 0;
}

It prints this:

hi 4

But you might expect it to print this:

hi 2

If we never call size() on the string then it doesn't even matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured as a first step, this is a quick fix that fixes the out-of-bounds read.

I think what we really want is a safer utility to read strings. Maybe something like a read_string function on databuf similar to the read_uintxx functions. But that is a larger refactor that I didn't have time to implement.


io_->seek(cur_pos + size, BasicIo::beg);
} // QuickTimeVideo::previewTagDecoder
Expand All @@ -685,7 +685,7 @@ void QuickTimeVideo::keysTagDecoder(size_t size) {
if (equalsQTimeTag(buf, "PICT"))
xmpData_["Xmp.video.PreviewAtomType"] = "QuickDraw Picture";
else
xmpData_["Xmp.video.PreviewAtomType"] = Exiv2::toString(buf.data());
xmpData_["Xmp.video.PreviewAtomType"] = std::string{buf.c_str(), 4};

io_->seek(cur_pos + size, BasicIo::beg);
} // QuickTimeVideo::keysTagDecoder
Expand Down
Binary file added test/data/issue_2340_poc.mp4
View file
Open in desktop
Binary file not shown.
13 changes: 13 additions & 0 deletions tests/bugfixes/github/test_issue_2340.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# -*- coding: utf-8 -*-

from system_tests import CaseMeta, check_no_ASAN_UBSAN_errors

class issue_2320_printDegrees_integer_overflow(metaclass=CaseMeta):
url = "https://github.com/Exiv2/exiv2/issues/2340"
filename = "$data_path/issue_2340_poc.mp4"
commands = ["$exiv2 -q -pa $filename"]
retval = [1]
stderr = ["""$exiv2_exception_message $filename:
$kerCorruptedMetadata
"""]
compare_stdout = check_no_ASAN_UBSAN_errors
1 change: 1 addition & 0 deletions tests/regression_tests/test_regression_allfiles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def get_valid_files(data_dir):
"issue_2190_poc.jp2",
# non-zero return code files, most of them are security POC so we don't
# really need to worry about them here
"issue_2340_poc.mp4",
"2018-01-09-exiv2-crash-001.tiff",
"cve_2017_1000126_stack-oob-read.webp",
"exiv2-bug1247.jpg",
Expand Down