Skip to content
Permalink
Browse files Browse the repository at this point in the history
length checks when looking for control files
  • Loading branch information
kyz committed Feb 18, 2019
1 parent cb5d78c commit 2f08413
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
8 changes: 8 additions & 0 deletions libmspack/ChangeLog
@@ -1,3 +1,11 @@
2019-02-18 Stuart Caie <kyzer@cabextract.org.uk>

* chmd_read_headers(): a CHM file name beginning "::" but shorter
than 33 bytes will lead to reading past the freshly-allocated name
buffer - checks for specific control filenames didn't take length
into account. Thanks to ADLab of Venustech for the report and
proof of concept.

2018-11-03 Stuart Caie <kyzer@cabextract.org.uk>

* configure.ac, doc/Makefile.in, doc/Doxyfile.in: remove these
Expand Down
24 changes: 11 additions & 13 deletions libmspack/mspack/chmd.c
Expand Up @@ -483,19 +483,17 @@ static int chmd_read_headers(struct mspack_system *sys, struct mspack_file *fh,

if (name[0] == ':' && name[1] == ':') {
/* system file */
if (memcmp(&name[2], &content_name[2], 31L) == 0) {
if (memcmp(&name[33], &content_name[33], 8L) == 0) {
chm->sec1.content = fi;
}
else if (memcmp(&name[33], &control_name[33], 11L) == 0) {
chm->sec1.control = fi;
}
else if (memcmp(&name[33], &spaninfo_name[33], 8L) == 0) {
chm->sec1.spaninfo = fi;
}
else if (memcmp(&name[33], &rtable_name[33], 72L) == 0) {
chm->sec1.rtable = fi;
}
if (name_len == 40 && memcmp(name, content_name, 40) == 0) {
chm->sec1.content = fi;
}
else if (name_len == 44 && memcmp(name, control_name, 44) == 0) {
chm->sec1.control = fi;
}
else if (name_len == 41 && memcmp(name, spaninfo_name, 41) == 0) {
chm->sec1.spaninfo = fi;
}
else if (name_len == 105 && memcmp(name, rtable_name, 105) == 0) {
chm->sec1.rtable = fi;
}
fi->next = chm->sysfiles;
chm->sysfiles = fi;
Expand Down

3 comments on commit 2f08413

@Beyond-My
Copy link

Choose a reason for hiding this comment

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

Hello, I want to ask why the 33 judged here has become 44, 41, and 105?
I see During this stage at chmd_read_headers() if a CHM file starts file '::' and is shorter than 33 bytes a heap-based buffer overflow happens due to out-of-bands read.
Please help me to answer this doubt,Thanks.

@kyz
Copy link
Owner Author

@kyz kyz commented on 2f08413 Jul 23, 2020

Choose a reason for hiding this comment

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

40, 44, 41 and 105 are the lengths of content_name, control_name, spaninfo_name and rtable_name respectively.

If the file name is shorter than 33 bytes, name_len is also less than 33, and so the short-circuiting expressions name_len == 40, name_len == 44, name_len == 41 and name_len == 105 all evaluate to false, and memcmp() is never called.

The 33 was previously used as it was the length of the prefix all these control filenames shared. To save memory and CPU cycles, the old code checked the filename for this prefix, and then checked the rest of the filename. However, it was flawed, and its replacement is faster as most filenames are not precisely 40/44/41/105 bytes long, so fewer string comparisons are made.

@Beyond-My
Copy link

Choose a reason for hiding this comment

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

40, 44, 41 and 105 are the lengths of content_name, control_name, spaninfo_name and rtable_name respectively.

If the file name is shorter than 33 bytes, name_len is also less than 33, and so the short-circuiting expressions name_len == 40, name_len == 44, name_len == 41 and name_len == 105 all evaluate to false, and memcmp() is never called.

The 33 was previously used as it was the length of the prefix all these control filenames shared. To save memory and CPU cycles, the old code checked the filename for this prefix, and then checked the rest of the filename. However, it was flawed, and its replacement is faster as most filenames are not precisely 40/44/41/105 bytes long, so fewer string comparisons are made.

Thank you very much for your patiently answer, I understand.It is great!

Please sign in to comment.