Commit 2ffdb5b9bec7c50bee553c36cd6d6fa9879ff175
2018-10-29T18:32:39
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54
diff --git a/src/tree.c b/src/tree.c
index d9faaa7..d1c1d77 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -399,22 +399,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;
}
@@ -438,11 +437,14 @@ int git_tree__parse(void *_tree, git_odb_object *odb_obj)
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);