From 5ccb0c35e308d936df5dcfd0733558dba6c2bde5 Mon Sep 17 00:00:00 2001 From: Andy Ragusa Date: Mon, 4 Apr 2022 18:36:42 -0700 Subject: [PATCH] Fixed heap buffer overflow while loading signatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is a possible overflow read when loading PDB and WDB phishing signatures. This issue is not a vulnerability. Changed const char pointers to uint8_t pointers when they are to be used with data, as well as removing asserts and adding additional error checking. Thank you Michał Dardas for reporting this issue. --- libclamav/matcher-ac.c | 62 +++++++-- libclamav/others.h | 38 ++++++ libclamav/regex_list.c | 266 +++++++++++++++++++++++++++++---------- libclamav/regex_suffix.c | 36 +++--- libclamav/unzip.c | 4 +- unit_tests/check_regex.c | 2 +- 6 files changed, 311 insertions(+), 97 deletions(-) diff --git a/libclamav/matcher-ac.c b/libclamav/matcher-ac.c index 02bebf32bd..b99e9e6001 100644 --- a/libclamav/matcher-ac.c +++ b/libclamav/matcher-ac.c @@ -641,10 +641,36 @@ static void ac_free_special(struct cli_ac_patt *p) MPOOL_FREE(mempool, p->special_table); } +/* + * This is a test to see if we have already seen this pointer. If we have, we + * have already freed it, so don't do it again (double-free) + */ +static int need_to_free_trans(struct cli_matcher *root, const size_t idx) +{ + size_t j; + size_t min = idx; + if (root->ac_nodes < idx) { + /*Should never happen, but check just to be safe.*/ + min = root->ac_nodes; + } + + for (j = 0; j < min; j++) { + if (NULL == root->ac_nodetable[j]) { + continue; + } + + if (root->ac_nodetable[idx]->trans == root->ac_nodetable[j]->trans) { + return 0; + } + } + + return 1; +} + void cli_ac_free(struct cli_matcher *root) { - uint32_t i; - struct cli_ac_patt *patt; + uint32_t i = 0; + struct cli_ac_patt *patt = NULL; for (i = 0; i < root->ac_patterns; i++) { patt = root->ac_pattable[i]; @@ -655,45 +681,55 @@ void cli_ac_free(struct cli_matcher *root) TODO: never store the virname in the ac pattern and only store it per-signature, not per-pattern. */ MPOOL_FREE(root->mempool, patt->virname); } - if (patt->special) + if (patt->special) { mpool_ac_free_special(root->mempool, patt); + } MPOOL_FREE(root->mempool, patt); } - if (root->ac_pattable) + if (root->ac_pattable) { MPOOL_FREE(root->mempool, root->ac_pattable); + } - if (root->ac_reloff) + if (root->ac_reloff) { MPOOL_FREE(root->mempool, root->ac_reloff); + } /* Freeing trans nodes must be done before freeing table nodes! */ for (i = 0; i < root->ac_nodes; i++) { if (!IS_LEAF(root->ac_nodetable[i]) && - root->ac_nodetable[i]->fail && - root->ac_nodetable[i]->trans != root->ac_nodetable[i]->fail->trans) { - MPOOL_FREE(root->mempool, root->ac_nodetable[i]->trans); + root->ac_root->trans != root->ac_nodetable[i]->trans) { + + if (need_to_free_trans(root, i)) { + MPOOL_FREE(root->mempool, root->ac_nodetable[i]->trans); + } } } - for (i = 0; i < root->ac_lists; i++) + for (i = 0; i < root->ac_lists; i++) { MPOOL_FREE(root->mempool, root->ac_listtable[i]); + } - if (root->ac_listtable) + if (root->ac_listtable) { MPOOL_FREE(root->mempool, root->ac_listtable); + } - for (i = 0; i < root->ac_nodes; i++) + for (i = 0; i < root->ac_nodes; i++) { MPOOL_FREE(root->mempool, root->ac_nodetable[i]); + } - if (root->ac_nodetable) + if (root->ac_nodetable) { MPOOL_FREE(root->mempool, root->ac_nodetable); + } if (root->ac_root) { MPOOL_FREE(root->mempool, root->ac_root->trans); MPOOL_FREE(root->mempool, root->ac_root); } - if (root->filter) + if (root->filter) { MPOOL_FREE(root->mempool, root->filter); + } } /* diff --git a/libclamav/others.h b/libclamav/others.h index 38020a5d4c..a8dfa7c631 100644 --- a/libclamav/others.h +++ b/libclamav/others.h @@ -1227,6 +1227,19 @@ uint8_t cli_set_debug_flag(uint8_t debug_flag); } while (0) #endif +#ifndef CLI_STRDUP +#define CLI_STRDUP(buf, var, ...) \ + do { \ + var = cli_strdup(buf); \ + if (NULL == var) { \ + do { \ + __VA_ARGS__; \ + } while (0); \ + goto done; \ + } \ + } while (0) +#endif + #ifndef FREE #define FREE(var) \ do { \ @@ -1326,4 +1339,29 @@ uint8_t cli_set_debug_flag(uint8_t debug_flag); } while (0) #endif +/** + * @brief Wrapper around realloc that limits how much may be allocated to CLI_MAX_ALLOCATION. + * + * IMPORTANT: This differs from realloc() in that if size==0, it will NOT free the ptr. + * + * NOTE: cli_realloc() will NOT free ptr if size==0. It is safe to free ptr after `done:`. + * + * @param ptr + * @param size + * @return void* + */ +#ifndef CLI_REALLOC +#define CLI_REALLOC(ptr, size, ...) \ + do { \ + void *vTmp = cli_realloc(ptr, size); \ + if (NULL == vTmp) { \ + do { \ + __VA_ARGS__; \ + } while (0); \ + goto done; \ + } \ + ptr = vTmp; \ + } while (0) +#endif + #endif diff --git a/libclamav/regex_list.c b/libclamav/regex_list.c index 9bf5a8fd72..6fae820b76 100644 --- a/libclamav/regex_list.c +++ b/libclamav/regex_list.c @@ -39,7 +39,6 @@ #include #include -#include #include "regex/regex.h" @@ -162,13 +161,35 @@ cl_error_t regex_list_match(struct regex_matcher *matcher, char *real_url, const struct cli_ac_data mdata; struct cli_ac_result *res = NULL; - assert(matcher); - assert(real_url); - assert(display_url); + if (NULL == matcher) { + rc = CL_ENULLARG; + cli_errmsg("regex_list_match: matcher must be initialized\n"); + goto done; + } + + if (NULL == real_url) { + rc = CL_ENULLARG; + cli_errmsg("regex_list_match: real_url must be initialized\n"); + goto done; + } + + if (NULL == display_url) { + rc = CL_ENULLARG; + cli_errmsg("regex_list_match: display_url must be initialized\n"); + goto done; + } + *info = NULL; - if (!matcher->list_inited) - return CL_SUCCESS; - assert(matcher->list_built); + if (1 != matcher->list_inited) { + rc = CL_SUCCESS; + goto done; + } + if (0 == matcher->list_built) { + cli_errmsg("regex_list_match: matcher->list_built must be initialized\n"); + rc = CL_ENULLARG; + goto done; + } + /* skip initial '.' inserted by get_host */ if (real_url[0] == '.') real_url++; if (display_url[0] == '.') display_url++; @@ -279,6 +300,7 @@ cl_error_t regex_list_match(struct regex_matcher *matcher, char *real_url, const cli_dbgmsg("Lookup result: not in regex list\n"); else cli_dbgmsg("Lookup result: in regex list\n"); +done: return rc; } @@ -287,11 +309,25 @@ cl_error_t regex_list_match(struct regex_matcher *matcher, char *real_url, const cl_error_t init_regex_list(struct regex_matcher *matcher, uint8_t dconf_prefiltering) { #ifdef USE_MPOOL - mpool_t *mp = matcher->mempool; + mpool_t *mp = NULL; +#endif + cl_error_t rc = CL_SUCCESS; + + if (NULL == matcher) { + cli_errmsg("init_regex_list: matcher must be initialized\n"); + rc = CL_ENULLARG; + goto done; + } + +#ifdef USE_MPOOL + mp = matcher->mempool; + if (NULL == mp) { + cli_errmsg("init_regex_list: matcher->mempool must be initialized\n"); + rc = CL_ENULLARG; + goto done; + } #endif - cl_error_t rc; - assert(matcher); memset(matcher, 0, sizeof(*matcher)); matcher->list_inited = 1; @@ -301,23 +337,25 @@ cl_error_t init_regex_list(struct regex_matcher *matcher, uint8_t dconf_prefilte #ifdef USE_MPOOL matcher->mempool = mp; matcher->suffixes.mempool = mp; - assert(mp && "mempool must be initialized"); #endif + if ((rc = cli_ac_init(&matcher->suffixes, 2, 32, dconf_prefiltering))) { - return rc; + goto done; } #ifdef USE_MPOOL matcher->sha256_hashes.mempool = mp; matcher->hostkey_prefix.mempool = mp; #endif if ((rc = cli_bm_init(&matcher->sha256_hashes))) { - return rc; + goto done; } if ((rc = cli_bm_init(&matcher->hostkey_prefix))) { - return rc; + goto done; } filter_init(&matcher->filter); - return CL_SUCCESS; + +done: + return rc; } static int functionality_level_check(char *line) @@ -424,7 +462,10 @@ cl_error_t load_regex_matcher(struct cl_engine *engine, struct regex_matcher *ma int line = 0, entry = 0; char buffer[FILEBUFF]; - assert(matcher); + if (NULL == matcher) { + cli_errmsg("load_regex_matcher: matcher must be initialized\n"); + return CL_ENULLARG; + } if (matcher->list_inited == -1) return CL_EMALFDB; /* already failed to load */ @@ -510,8 +551,9 @@ cl_error_t load_regex_matcher(struct cl_engine *engine, struct regex_matcher *ma if ((buffer[0] == 'R' && !is_allow_list_lookup) || ((buffer[0] == 'X' || buffer[0] == 'Y') && is_allow_list_lookup)) { /* regex for hostname*/ - if ((rc = regex_list_add_pattern(matcher, pattern))) + if ((rc = regex_list_add_pattern(matcher, pattern))) { return rc == CL_EMEM ? CL_EMEM : CL_EMALFDB; + } } else if ((buffer[0] == 'H' && !is_allow_list_lookup) || (buffer[0] == 'M' && is_allow_list_lookup)) { /*matches displayed host*/ if ((rc = add_static_pattern(matcher, pattern))) @@ -564,7 +606,10 @@ cl_error_t cli_build_regex_list(struct regex_matcher *matcher) /* Done with this matcher, free resources */ void regex_list_done(struct regex_matcher *matcher) { - assert(matcher); + if (NULL == matcher) { + cli_errmsg("regex_list_done: matcher must be initialized\n"); + goto done; + } if (matcher->list_inited == 1) { size_t i; @@ -594,24 +639,55 @@ void regex_list_done(struct regex_matcher *matcher) cli_bm_free(&matcher->sha256_hashes); cli_bm_free(&matcher->hostkey_prefix); } + +done: + return; } int is_regex_ok(struct regex_matcher *matcher) { - assert(matcher); - return (!matcher->list_inited || matcher->list_inited != -1); /* either we don't have a regexlist, or we initialized it successfully */ + int ret = 0; + if (NULL == matcher) { + cli_errmsg("is_regex_ok: matcher must be initialized\n"); + } else { + ret = (!matcher->list_inited || matcher->list_inited != -1); /* either we don't have a regexlist, or we initialized it successfully */ + } + + return ret; } -static int add_newsuffix(struct regex_matcher *matcher, struct regex_list *info, const char *suffix, size_t len) +static cl_error_t add_newsuffix(struct regex_matcher *matcher, struct regex_list *info, const char *suffix, size_t len) { - struct cli_matcher *root = &matcher->suffixes; - struct cli_ac_patt *new = MPOOL_CALLOC(matcher->mempool, 1, sizeof(*new)); + struct cli_matcher *root = NULL; + struct cli_ac_patt *new = NULL; size_t i; - int ret; + cl_error_t ret = CL_SUCCESS; - if (!new) - return CL_EMEM; - assert(root && suffix); + if (NULL == matcher) { + cli_errmsg("add_newsuffix: matcher must be initialized\n"); + ret = CL_ENULLARG; + goto done; + } + + root = &matcher->suffixes; + if (NULL == root) { + cli_errmsg("add_newsuffix: root must be initialized\n"); + ret = CL_ENULLARG; + goto done; + } + + if (NULL == suffix) { + cli_errmsg("add_newsuffix: suffix must be initialized\n"); + ret = CL_ENULLARG; + goto done; + } + + new = MPOOL_CALLOC(matcher->mempool, 1, sizeof(*new)); + if (!new) { + cli_errmsg("add_newsuffix: Unable to allocate memory for new\n"); + ret = CL_EMEM; + goto done; + } new->rtype = 0; new->type = 0; @@ -629,22 +705,38 @@ static int add_newsuffix(struct regex_matcher *matcher, struct regex_list *info, new->pattern = MPOOL_MALLOC(matcher->mempool, sizeof(new->pattern[0]) * len); if (!new->pattern) { - MPOOL_FREE(matcher->mempool, new); cli_errmsg("add_newsuffix: Unable to allocate memory for new->pattern\n"); - return CL_EMEM; + ret = CL_EMEM; + goto done; } - for (i = 0; i < len; i++) + for (i = 0; i < len; i++) { new->pattern[i] = suffix[i]; /*new->pattern is short int* */ + } new->customdata = info; new->virname = NULL; if ((ret = cli_ac_addpatt(root, new))) { - MPOOL_FREE(matcher->mempool, new->pattern); - MPOOL_FREE(matcher->mempool, new); - return ret; + goto done; } - filter_add_static(&matcher->filter, (const unsigned char *)suffix, len, "regex"); - return CL_SUCCESS; + + if (filter_add_static(&matcher->filter, (const unsigned char *)suffix, len, "regex") < 0) { + cli_errmsg("add_newsuffix: Unable to add filter\n"); + ret = CL_ERROR; + goto done; + } + +done: + + if (CL_SUCCESS != ret) { + if (NULL != new) { + if (NULL != new->pattern) { + MPOOL_FREE(matcher->mempool, new->pattern); + } + MPOOL_FREE(matcher->mempool, new); + } + } + + return ret; } #define MODULE "regex_list: " @@ -663,45 +755,84 @@ static void list_add_tail(struct regex_list_ht *ht, struct regex_list *regex) static cl_error_t add_pattern_suffix(void *cbdata, const char *suffix, size_t suffix_len, const struct regex_list *iregex) { struct regex_matcher *matcher = cbdata; - struct regex_list *regex = cli_malloc(sizeof(*regex)); - const struct cli_element *el; - void *tmp_matcher; /* save original address if OOM occurs */ + struct regex_list *regex = NULL; + const struct cli_element *el = NULL; + void *tmp_matcher = NULL; /* save original address if OOM occurs */ + cl_error_t ret = CL_SUCCESS; - assert(matcher); - if (!regex) { - cli_errmsg("add_pattern_suffix: Unable to allocate memory for regex\n"); - return CL_EMEM; + if (NULL == matcher) { + cli_errmsg("add_pattern_suffix: matcher must be initialized\n"); + ret = CL_ENULLARG; + goto done; + } + if (NULL == suffix) { + cli_errmsg("add_pattern_suffix: suffix must be initialized\n"); + ret = CL_ENULLARG; + goto done; } - regex->pattern = iregex->pattern ? cli_strdup(iregex->pattern) : NULL; - regex->preg = iregex->preg; - regex->nxt = NULL; - el = cli_hashtab_find(&matcher->suffix_hash, suffix, suffix_len); + if (NULL == iregex) { + cli_errmsg("add_pattern_suffix: iregex must be initialized\n"); + ret = CL_ENULLARG; + goto done; + } + + CLI_MALLOC(regex, sizeof(*regex), + cli_errmsg("add_pattern_suffix: Unable to allocate memory for regex\n"); + ret = CL_EMEM); + + if (NULL == iregex->pattern) { + regex->pattern = NULL; + } else { + CLI_STRDUP(iregex->pattern, regex->pattern, + cli_errmsg("add_pattern_suffix: unable to strdup iregex->pattern"); + ret = CL_EMEM); + } + regex->preg = iregex->preg; + regex->nxt = NULL; + el = cli_hashtab_find(&matcher->suffix_hash, suffix, suffix_len); /* TODO: what if suffixes are prefixes of eachother and only one will * match? */ if (el) { /* existing suffix */ - assert((size_t)el->data < matcher->suffix_cnt); + if ((size_t)el->data >= matcher->suffix_cnt) { + cli_errmsg("add_pattern_suffix: el-> data too large"); + ret = CL_ERROR; + goto done; + } list_add_tail(&matcher->suffix_regexes[(size_t)el->data], regex); } else { /* new suffix */ size_t n = matcher->suffix_cnt; el = cli_hashtab_insert(&matcher->suffix_hash, suffix, suffix_len, (cli_element_data)n); tmp_matcher = matcher->suffix_regexes; /* save the current value before cli_realloc() */ - tmp_matcher = cli_realloc(matcher->suffix_regexes, (n + 1) * sizeof(*matcher->suffix_regexes)); - if (!tmp_matcher) { - FREE(regex->pattern); - free(regex); - return CL_EMEM; - } - matcher->suffix_regexes = tmp_matcher; /* success, point at new memory location */ + CLI_REALLOC(matcher->suffix_regexes, + (n + 1) * sizeof(*matcher->suffix_regexes), + cli_errmsg("add_pattern_suffix: Unable to reallocate memory for matcher->suffix_regexes\n"); + ret = CL_EMEM); matcher->suffix_regexes[n].tail = regex; matcher->suffix_regexes[n].head = regex; - matcher->suffix_cnt++; - if (suffix[0] == '/' && suffix[1] == '\0') + if (suffix[0] == '/' && suffix[1] == '\0') { matcher->root_regex_idx = n; - add_newsuffix(matcher, regex, suffix, suffix_len); + } + + ret = add_newsuffix(matcher, regex, suffix, suffix_len); + + if (CL_SUCCESS != ret) { + cli_hashtab_delete(&matcher->suffix_hash, suffix, suffix_len); + /*shrink the size back to what it was.*/ + CLI_REALLOC(matcher->suffix_regexes, n * sizeof(*matcher->suffix_regexes)); + } else { + matcher->suffix_cnt++; + } } - return CL_SUCCESS; + +done: + if (CL_SUCCESS != ret) { + FREE(regex->pattern); + FREE(regex); + } + + return ret; } static size_t reverse_string(char *pattern) @@ -737,14 +868,17 @@ static cl_error_t add_static_pattern(struct regex_matcher *matcher, char *patter { size_t len; struct regex_list regex; - cl_error_t rc; - - len = reverse_string(pattern); - regex.nxt = NULL; - regex.pattern = cli_strdup(pattern); - regex.preg = NULL; - rc = add_pattern_suffix(matcher, pattern, len, ®ex); - free(regex.pattern); + cl_error_t rc = CL_EMEM; + + len = reverse_string(pattern); + regex.nxt = NULL; + CLI_STRDUP(pattern, regex.pattern, + cli_errmsg("add_static_pattern: Cannot allocate memory for regex.pattern\n"); + rc = CL_EMEM); + regex.preg = NULL; + rc = add_pattern_suffix(matcher, pattern, len, ®ex); +done: + FREE(regex.pattern); return rc; } diff --git a/libclamav/regex_suffix.c b/libclamav/regex_suffix.c index 0ca3fa784b..863a862e52 100644 --- a/libclamav/regex_suffix.c +++ b/libclamav/regex_suffix.c @@ -27,7 +27,6 @@ #include #include #include -#include #include "clamav.h" #include "others.h" @@ -180,7 +179,7 @@ static void destroy_tree(struct node *n) free(n); } -static uint8_t *parse_char_class(const char *pat, size_t *pos) +static uint8_t *parse_char_class(const uint8_t *pat, size_t *pos) { unsigned char range_start = 0; int hasprev = 0; @@ -200,7 +199,10 @@ static uint8_t *parse_char_class(const char *pat, size_t *pos) /* it is a range*/ unsigned char range_end; unsigned int c; - assert(range_start); + if (0 == range_start) { + cli_errmsg("parse_char_class: range_start not initialized"); + return NULL; + } ++*pos; if (pat[*pos] == '[') if (pat[*pos + 1] == '.') { @@ -238,7 +240,7 @@ static uint8_t *parse_char_class(const char *pat, size_t *pos) return bitmap; } -static struct node *parse_regex(const char *p, size_t *last) +static struct node *parse_regex(const uint8_t *p, size_t *last) { struct node *v = NULL; struct node *right; @@ -426,14 +428,15 @@ static cl_error_t build_suffixtree_descend(struct node *n, struct text_buffer *b cl_error_t cli_regex2suffix(const char *pattern, regex_t *preg, suffix_callback cb, void *cbdata) { - struct regex_list regex; - struct text_buffer buf; - struct node root_node; - struct node *n; - size_t last = 0; + struct regex_list regex = {0}; + struct text_buffer buf = {0}; + struct node root_node = {0}; + struct node *n = NULL; + size_t last = 0; int rc; - assert(pattern); + VERIFY_POINTER(pattern, cli_errmsg("cli_regex2suffix: pattern must be initialized"); rc = CL_ENULLARG); + VERIFY_POINTER(preg, cli_errmsg("cli_regex2suffix: preg must be initialized"); rc = CL_ENULLARG); regex.preg = preg; rc = cli_regcomp(regex.preg, pattern, REG_EXTENDED); @@ -449,10 +452,10 @@ cl_error_t cli_regex2suffix(const char *pattern, regex_t *preg, suffix_callback } return rc; } - regex.nxt = NULL; - regex.pattern = cli_strdup(pattern); + regex.nxt = NULL; + CLI_STRDUP(pattern, regex.pattern, cli_errmsg("cli_regex2suffix: Unable to duplicate pattern"); rc = CL_EMEM); - n = parse_regex(pattern, &last); + n = parse_regex(((const uint8_t *)pattern), &last); if (!n) return REG_ESPACE; memset(&buf, 0, sizeof(buf)); @@ -460,8 +463,11 @@ cl_error_t cli_regex2suffix(const char *pattern, regex_t *preg, suffix_callback n->parent = &root_node; rc = build_suffixtree_descend(n, &buf, cb, cbdata, ®ex); - free(regex.pattern); - free(buf.data); + +done: + + FREE(regex.pattern); + FREE(buf.data); destroy_tree(n); return rc; } diff --git a/libclamav/unzip.c b/libclamav/unzip.c index b8b4a15dcc..91c9e37acc 100644 --- a/libclamav/unzip.c +++ b/libclamav/unzip.c @@ -113,8 +113,8 @@ static cl_error_t unz( zip_cb zcb, const char *original_filename) { - char obuf[BUFSIZ]; - char *tempfile = NULL; + char obuf[BUFSIZ] = {0}; + char *tempfile = NULL; int out_file, ret = CL_CLEAN; int res = 1; size_t written = 0; diff --git a/unit_tests/check_regex.c b/unit_tests/check_regex.c index 6a3d2cd42f..6e4159b2c6 100644 --- a/unit_tests/check_regex.c +++ b/unit_tests/check_regex.c @@ -219,7 +219,7 @@ static const struct rtest { {NULL, "http://key.com", "go to key.com", RTR_CLEAN}, {":.+\\.paypal\\.(com|de|fr|it)([/?].*)?:.+\\.ebay\\.(at|be|ca|ch|co\\.uk|de|es|fr|ie|in|it|nl|ph|pl|com(\\.(au|cn|hk|my|sg))?)([/?].*)?/", - "http://www.paypal.com", "pics.ebay.com", RTR_ALLOWED}, + "http://www.paypal.com", "pics.ebay.com", RTR_INVALID_REGEX}, {NULL, "http://somefakeurl.example.com", "someotherdomain-key.com", RTR_CLEAN}, {NULL, "http://somefakeurl.example.com", "someotherdomain.key.com", RTR_PHISH}, {NULL, "http://malware-test.example.com/something", "test", RTR_BLOCKED},