diff options
author | Patrick Steinhardt <ps@pks.im> | 2018-10-29 18:32:39 +0100 |
---|---|---|
committer | Patrick Steinhardt <ps@pks.im> | 2018-11-02 13:31:09 +0100 |
commit | 7fafec0e53f8711b73912d46b43451c599aeceb3 (patch) | |
tree | 5dca2f4c35fabf0e43844a58d5792f8254153639 | |
parent | f647bbc88d243a81d8771ba2fd1c346c34a3d9d7 (diff) | |
download | libgit2-7fafec0e53f8711b73912d46b43451c599aeceb3.tar.gz |
tree: fix integer overflow when reading unreasonably large filemodes
The `parse_mode` option uses an open-coded octal number parser. The
parser is quite naive in that it simply parses until hitting a character
that is not in the accepted range of '0' - '7', completely ignoring the
fact that we can at most accept a 16 bit unsigned integer as filemode.
If the filemode is bigger than UINT16_MAX, it will thus overflow and
provide an invalid filemode for the object entry.
Fix the issue by using `git__strntol32` instead and doing a bounds
check. As this function already handles overflows, it neatly solves the
problem.
Note that previously, `parse_mode` was also skipping the character
immediately after the filemode. In proper trees, this should be a simple
space, but in fact the parser accepted any character and simply skipped
over it. As a consequence of using `git__strntol32`, we now need to an
explicit check for a trailing whitespace after having parsed the
filemode. Because of the newly introduced error message, the test
object::tree::parse::mode_doesnt_cause_oob_read needs adjustment to its
error message check, which in fact is a good thing as it demonstrates
that we now fail looking for the whitespace immediately following the
filemode.
Add a test that shows that we will fail to parse such invalid filemodes
now.
-rw-r--r-- | src/tree.c | 28 | ||||
-rw-r--r-- | tests/object/tree/parse.c | 8 |
2 files changed, 22 insertions, 14 deletions
diff --git a/src/tree.c b/src/tree.c index 9772f5a2d..d9b59390a 100644 --- a/src/tree.c +++ b/src/tree.c @@ -356,22 +356,21 @@ static int tree_error(const char *str, const char *path) return -1; } -static int parse_mode(unsigned int *modep, const char *buffer, size_t buffer_len, const char **buffer_out) +static int parse_mode(uint16_t *mode_out, const char *buffer, size_t buffer_len, const char **buffer_out) { - const char *buffer_end = buffer + buffer_len; - unsigned char c; - unsigned int mode = 0; + int32_t mode; + int error; - if (*buffer == ' ') + if (!buffer_len || git__isspace(*buffer)) return -1; - while (buffer < buffer_end && (c = *buffer++) != ' ') { - if (c < '0' || c > '7') - return -1; - mode = (mode << 3) + (c - '0'); - } - *modep = mode; - *buffer_out = buffer; + if ((error = git__strntol32(&mode, buffer, buffer_len, buffer_out, 8)) < 0) + return error; + + if (mode < 0 || mode > UINT16_MAX) + return -1; + + *mode_out = mode; return 0; } @@ -393,11 +392,14 @@ int git_tree__parse_raw(void *_tree, const char *data, size_t size) git_tree_entry *entry; size_t filename_len; const char *nul; - unsigned int attr; + uint16_t attr; if (parse_mode(&attr, buffer, buffer_end - buffer, &buffer) < 0 || !buffer) return tree_error("failed to parse tree: can't parse filemode", NULL); + if (buffer >= buffer_end || (*buffer++) != ' ') + return tree_error("failed to parse tree: missing space after filemode", NULL); + if ((nul = memchr(buffer, 0, buffer_end - buffer)) == NULL) return tree_error("failed to parse tree: object is corrupted", NULL); diff --git a/tests/object/tree/parse.c b/tests/object/tree/parse.c index 67b758667..83d22193d 100644 --- a/tests/object/tree/parse.c +++ b/tests/object/tree/parse.c @@ -118,7 +118,13 @@ void test_object_tree_parse__mode_doesnt_cause_oob_read(void) * later fail to parse the OID with a different error * message */ - cl_assert(strstr(giterr_last()->message, "object is corrupted")); + cl_assert_equal_s(giterr_last()->message, "failed to parse tree: missing space after filemode"); +} + +void test_object_tree_parse__unreasonably_large_mode_fails(void) +{ + const char data[] = "10000000000000000000000000 bar\x00" OID1_HEX; + assert_tree_fails(data, ARRAY_SIZE(data) - 1); } void test_object_tree_parse__missing_filename_separator_fails(void) |