Fix memory safety issues #260
Conversation
If the string ends with a '\', the function tried to read the next character before checking bounds. This commit move the bounds check before the read to avoid the out-of-bounds read. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21578
If the string ends with a Multiline terminator without a newline, the function tried to read the next character to check for a newline without checking if the pointer was past the end of the buffer. This commit adds a bounds check and return early with an error in case of missing newline. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21579
If the input contains '${' but no following '}', ucl_check_variable
should still increment out_len since ucl_expand_variable will copy the
'$' in the destination buffer.
Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24591
I believe this was the intent of the original format string, but two characters got swapped. See printf docs at https://www.gnu.org/software/libc/manual/html_node/Output-Conversion-Syntax.html#Output-Conversion-Syntax. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=25626 Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33041
The strncmp call could read past the bounds of the haystack. The loop now stop when the remaining data in the haystack cannot contain the needle. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28135
If the input ends in '$', calling ucl_check_variable will result in an out-of-bounds read. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34755
Keys may have null bytes, when they are decoded from json in ucl_unescape_json_string and contain \u0000. Not copying the full key resulted in out-of-bounds reads. The copy now relies on memcpy and keylen instead of strdup. Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=38579 Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=38675
This macro is often used in loops before checking whether the end of chunk condition. Adding a bounds check in there prevents reading past the buffer.
This commit improves variable expansion and adds: - an in_len argument to the function in order to avoid reading past the end of the src buffer - a check that the variable name is followed by '}' in strict mode - extra bounds check to prevent out of bounds reads and writes
There was a problem hiding this comment.
Thank you very much for the proposed changes! The only place that might break the compatibility is key copy that will be no longer zero terminated. I would suggest to add a trailing \0 for safety and sanity (well, as long we can consider zero terminated strings sane).
| * @return | ||
| */ | ||
| static const char * | ||
| ucl_expand_single_variable (struct ucl_parser *parser, const char *ptr, | ||
| size_t remain, unsigned char **dest) | ||
| size_t in_len, unsigned char **dest, size_t out_len) |
There was a problem hiding this comment.
Yes, this name is more fortunate, I agree.
| @@ -517,7 +528,7 @@ ucl_expand_variable (struct ucl_parser *parser, unsigned char **dst, | |||
|
|
|||
| p = src; | |||
| while (p != end) { | |||
| if (*p == '$') { | |||
| if (*p == '$' && p + 1 != end) { | |||
| @@ -1919,7 +1919,7 @@ ucl_inherit_handler (const unsigned char *data, size_t len, | |||
|
|
|||
| /* Some sanity checks */ | |||
| if (parent == NULL || ucl_object_type (parent) != UCL_OBJECT) { | |||
| ucl_create_err (&parser->err, "Unable to find inherited object %*.s", | |||
| ucl_create_err (&parser->err, "Unable to find inherited object %.*s", | |||
There was a problem hiding this comment.
Oh, I always forget about the order of * and .. In Rspamd, I have the own printf implementation which has distinction between %.*s and %*.s. But here we use a generic libc printf...
| if (other->key == (const char *)other->trash_stack[UCL_TRASH_KEY]) { | ||
| new->trash_stack[UCL_TRASH_KEY] = malloc(other->keylen); | ||
| memcpy(new->trash_stack[UCL_TRASH_KEY], other->trash_stack[UCL_TRASH_KEY], other->keylen); |
There was a problem hiding this comment.
This change is really good, however, I think it will still be a good idea to terminate key with \0. Otherwise, it will definitely break functions like ucl_object_key which just return a const char*, e.g. how it is done in ucl_copy_value_trash function.
There was a problem hiding this comment.
This makes a lot of sense. I added the null-byte terminator in 110428d.
|
Thank you again, much appreciated! |
|
BTW, I have found an issue with variables expansion after this change, I will fix it shortly. |
Oops, thanks for fixing it! I'd be happy to add a test for it if you'd like. |
This PR fixes some memory safety issues reported by OSS-Fuzz in
ucl_add_string_fuzzerthat were labeled as security relevant:The PR also fixes a couple issues I ran into while fuzzing locally, mainly in variable expansion to avoid out-of-bounds reads and writes. Each fix is in its own commit describing the issue and the fix. I'd be happy to split the PR if that made it easier to review (or to scrap the commits if you think they are not valuable)
The fuzzer still reports out-of-bounds read when parsing values, as the
strto*functions assume the input buffer is null terminated. Theucl_add_string_fuzzerdoes not give a null-terminated buffer, but I wasn't sure if that was a proper use of the API.I ran
make checkand got the same failures as the main branch. If there's other way to tests my changes, I'd be happy to do that as well if you point me in the right direction.