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 memory safety issues #260

Merged
merged 10 commits into from
Jan 6, 2022
Merged

Fix memory safety issues #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ac8d760
ucl_lex_json_string: fix out-of-bounds read
alpire Jan 4, 2022
08de358
ucl_parse_value: fix out-of-bounds read
alpire Jan 4, 2022
3a94514
ucl_check_variable: fix out_len on unterminated variable
alpire Jan 4, 2022
54a5b59
ucl_inherit_handler: fix format string for non-null-terminated strings
alpire Jan 4, 2022
852f752
ucl_strnstr: fix out-of-bounds read
alpire Jan 4, 2022
f9e5446
ucl_expand_variable: fix out-of-bounds read
alpire Jan 4, 2022
a9c0965
ucl_object_copy_internal: use memcpy instead of strdup
alpire Jan 4, 2022
9e0e06f
ucl_chunk_skipc: avoid out-of-bounds read
alpire Jan 5, 2022
9b898ea
ucl_expand_single_variable: better bounds check
alpire Jan 5, 2022
110428d
ucl_object_copy_internal: null terminate keys
alpire Jan 5, 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
57 changes: 37 additions & 20 deletions src/ucl_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ struct ucl_parser_saved_state {
*/
#define ucl_chunk_skipc(chunk, p) \
do { \
if (p == chunk->end) { \
break; \
} \
if (*(p) == '\n') { \
(chunk)->line ++; \
(chunk)->column = 0; \
Expand Down Expand Up @@ -395,6 +398,9 @@ ucl_check_variable (struct ucl_parser *parser, const char *ptr,
}
p ++;
}
if(p == end) {
(*out_len) ++;
}
}
else if (*ptr != '$') {
/* Not count escaped dollar sign */
Expand All @@ -418,13 +424,14 @@ ucl_check_variable (struct ucl_parser *parser, const char *ptr,
* Expand a single variable
* @param parser
* @param ptr
* @param remain
* @param in_len
* @param dest
* @param out_len
* @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)
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this name is more fortunate, I agree.

{
unsigned char *d = *dest, *dst;
const char *p = ptr + 1, *ret;
Expand All @@ -435,7 +442,8 @@ ucl_expand_single_variable (struct ucl_parser *parser, const char *ptr,
bool strict = false;

ret = ptr + 1;
remain --;
out_len --;
in_len --;

if (*p == '$') {
*d++ = *p++;
Expand All @@ -444,28 +452,31 @@ ucl_expand_single_variable (struct ucl_parser *parser, const char *ptr,
}
else if (*p == '{') {
p ++;
in_len --;
strict = true;
ret += 2;
remain -= 2;
out_len -= 2;
}

LL_FOREACH (parser->variables, var) {
if (remain >= var->var_len) {
if (out_len >= var->value_len && in_len >= (var->var_len + strict)) {
if (memcmp (p, var->var, var->var_len) == 0) {
memcpy (d, var->value, var->value_len);
ret += var->var_len;
d += var->value_len;
found = true;
break;
if (!strict || p[var->var_len] == '}') {
memcpy (d, var->value, var->value_len);
ret += var->var_len;
d += var->value_len;
found = true;
break;
}
}
}
}
if (!found) {
if (strict && parser->var_handler != NULL) {
if (parser->var_handler (p, remain, &dst, &dstlen, &need_free,
if (parser->var_handler (p, out_len, &dst, &dstlen, &need_free,
parser->var_data)) {
memcpy (d, dst, dstlen);
ret += remain;
ret += out_len;
d += dstlen;
found = true;
if (need_free) {
Expand All @@ -476,7 +487,7 @@ ucl_expand_single_variable (struct ucl_parser *parser, const char *ptr,

/* Leave variable as is */
if (!found) {
if (strict) {
if (strict && out_len >= 2) {
/* Copy '${' */
memcpy (d, ptr, 2);
d += 2;
Expand Down Expand Up @@ -506,7 +517,7 @@ ucl_expand_variable (struct ucl_parser *parser, unsigned char **dst,
const char *src, size_t in_len)
{
const char *p, *end = src + in_len;
unsigned char *d;
unsigned char *d, *d_end;
size_t out_len = 0;
bool vars_found = false;

Expand All @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch!

p = ucl_check_variable (parser, p + 1, end - p - 1, &out_len, &vars_found);
}
else {
Expand All @@ -538,10 +549,11 @@ ucl_expand_variable (struct ucl_parser *parser, unsigned char **dst,
}

d = *dst;
d_end = d + out_len;
p = src;
while (p != end) {
if (*p == '$') {
p = ucl_expand_single_variable (parser, p, end - p, &d);
while (p != end && d != d_end) {
if (*p == '$' && p + 1 != end) {
p = ucl_expand_single_variable (parser, p, end - p, &d, d_end - d);
}
else {
*d++ = *p++;
Expand Down Expand Up @@ -1053,13 +1065,13 @@ ucl_lex_json_string (struct ucl_parser *parser,
}
else if (c == '\\') {
ucl_chunk_skipc (chunk, p);
c = *p;
if (p >= chunk->end) {
ucl_set_err (parser, UCL_ESYNTAX, "unfinished escape character",
&parser->err);
return false;
}
else if (ucl_test_character (c, UCL_CHARACTER_ESCAPE)) {
c = *p;
if (ucl_test_character (c, UCL_CHARACTER_ESCAPE)) {
if (c == 'u') {
ucl_chunk_skipc (chunk, p);
for (i = 0; i < 4 && p < chunk->end; i ++) {
Expand Down Expand Up @@ -1828,6 +1840,11 @@ ucl_parse_value (struct ucl_parser *parser, struct ucl_chunk *chunk)
while (p < chunk->end && *p >= 'A' && *p <= 'Z') {
p ++;
}
if(p == chunk->end) {
ucl_set_err (parser, UCL_ESYNTAX,
"unterminated multiline value", &parser->err);
return false;
}
if (*p =='\n') {
/* Set chunk positions and start multiline parsing */
chunk->remain -= p - c + 1;
Expand Down
10 changes: 6 additions & 4 deletions src/ucl_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Owner

Choose a reason for hiding this comment

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

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...

(int)len, data);
return false;
}
Expand Down Expand Up @@ -2177,7 +2177,7 @@ ucl_strnstr (const char *s, const char *find, int len)
mlen = strlen (find);
do {
do {
if ((sc = *s++) == 0 || len-- == 0)
if ((sc = *s++) == 0 || len-- < mlen)
return (NULL);
} while (sc != c);
} while (strncmp (s, find, mlen) != 0);
Expand Down Expand Up @@ -3594,9 +3594,11 @@ ucl_object_copy_internal (const ucl_object_t *other, bool allow_array)

/* deep copy of values stored */
if (other->trash_stack[UCL_TRASH_KEY] != NULL) {
new->trash_stack[UCL_TRASH_KEY] =
strdup (other->trash_stack[UCL_TRASH_KEY]);
new->trash_stack[UCL_TRASH_KEY] = NULL;
if (other->key == (const char *)other->trash_stack[UCL_TRASH_KEY]) {
new->trash_stack[UCL_TRASH_KEY] = malloc(other->keylen + 1);
memcpy(new->trash_stack[UCL_TRASH_KEY], other->trash_stack[UCL_TRASH_KEY], other->keylen);
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes a lot of sense. I added the null-byte terminator in 110428d.

new->trash_stack[UCL_TRASH_KEY][other->keylen] = '\0';
new->key = new->trash_stack[UCL_TRASH_KEY];
}
}
Expand Down