From 00ec8b7f2b652abfff96027b7a97733c548af178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= Date: Sun, 10 Oct 2021 21:10:24 +0200 Subject: [PATCH] Correct buffer over-read errors When inserting multiple routes with common slug patterns there are reads beyond end of strings. The scenario is added as a testcase and can be triggered by the address-sanitizer when built using: `CFLAGS="-fno-omit-frame-pointer -fsanitize=address" cmake ..` Indicated as a `buffer-overflow` --- src/node.c | 10 ++++------ src/slug.c | 2 +- tests/check_routes2.c | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/node.c b/src/node.c index 0190949..500f174 100644 --- a/src/node.c +++ b/src/node.c @@ -551,18 +551,16 @@ static r3_iovec_t* router_append_slug(R3Route * route, const char * slug, unsign static void get_slugs(R3Route * route, const char * path, int path_len) { const char *plh = path; - unsigned int l, namel; - l = 0; const char *name; + unsigned int plhl, namel; while (plh < (path + path_len)) { - plh = r3_slug_find_placeholder(plh+l, path_len, &l); + plh = r3_slug_find_placeholder(plh, path_len-(plh-path), &plhl); if (!plh) break; - namel = 0; - name = r3_slug_find_name(plh, l, &namel); + name = r3_slug_find_name(plh, plhl, &namel); if (name) { router_append_slug(route, name, namel); } - if ((plh + l) >= (path + path_len)) break; + plh += plhl; } } diff --git a/src/slug.c b/src/slug.c index 3ecdad9..5f89207 100644 --- a/src/slug.c +++ b/src/slug.c @@ -86,7 +86,7 @@ int r3_slug_parse(r3_slug_t *s, const char *needle, int needle_len, const char * } // there is no slug - if (!r3_path_contains_slug_char(offset, needle_len)) { + if (!r3_path_contains_slug_char(offset, needle_len - (offset-needle))) { return 0; } diff --git a/tests/check_routes2.c b/tests/check_routes2.c index 86e74aa..e9cb14b 100644 --- a/tests/check_routes2.c +++ b/tests/check_routes2.c @@ -23,21 +23,58 @@ START_TEST (greedy_pattern) matched_route = r3_tree_match_route(n, entry); ck_assert(matched_route != NULL); ck_assert(matched_route->data == &uri0); + match_entry_free(entry); entry = match_entry_create("/foo"); matched_route = r3_tree_match_route(n, entry); ck_assert(matched_route != NULL); ck_assert(matched_route->data == &uri0); + match_entry_free(entry); entry = match_entry_create("/foo/"); matched_route = r3_tree_match_route(n, entry); ck_assert(matched_route != NULL); ck_assert(matched_route->data == &uri0); + match_entry_free(entry); entry = match_entry_create("/foo/bar/foo/mmasdfasdfasd/f/asdf/as/df"); matched_route = r3_tree_match_route(n, entry); ck_assert(matched_route != NULL); ck_assert(matched_route->data == &uri0); + match_entry_free(entry); + + r3_tree_free(n); +} +END_TEST + +START_TEST (common_pattern) +{ + R3Node * n = r3_tree_create(10); + match_entry * entry; + R3Route *r, *matched_route; + + char * uri0 = "/foo/{slug}"; + r = r3_tree_insert_routel(n, 0, uri0, strlen(uri0), &uri0); + ck_assert(r != NULL); + char * uri1 = "/foo/{slug}/bar"; + r = r3_tree_insert_routel(n, 0, uri1, strlen(uri1), &uri1); + ck_assert(r != NULL); + + char * err = NULL; + r3_tree_compile(n, &err); + ck_assert(err == NULL); + + entry = match_entry_create("/foo/bar"); + matched_route = r3_tree_match_route(n, entry); + ck_assert(matched_route != NULL); + ck_assert(matched_route->data == &uri0); + match_entry_free(entry); + + entry = match_entry_create("/foo/bar/bar"); + matched_route = r3_tree_match_route(n, entry); + ck_assert(matched_route != NULL); + ck_assert(matched_route->data == &uri1); + match_entry_free(entry); r3_tree_free(n); } @@ -47,6 +84,7 @@ Suite* r3_suite (void) { Suite *suite = suite_create("r3 routes2 tests"); TCase *tcase = tcase_create("testcase"); tcase_add_test(tcase, greedy_pattern); + tcase_add_test(tcase, common_pattern); suite_add_tcase(suite, tcase); return suite; }