From: Victor Julien Date: Thu, 1 Nov 2012 14:17:50 +0000 (+0100) Subject: libhtp: harden code against malloc failures. Bug #587. X-Git-Tag: suricata-1.4beta3~45 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=76f0838a9fa6d868ee85fb5f9282da132e554c0d;p=thirdparty%2Fsuricata.git libhtp: harden code against malloc failures. Bug #587. --- diff --git a/libhtp/htp/dslib.c b/libhtp/htp/dslib.c index b4b87c9b4d..01606db107 100644 --- a/libhtp/htp/dslib.c +++ b/libhtp/htp/dslib.c @@ -457,6 +457,8 @@ static void *table_get_internal(table_t *table, bstr *key) { * @return table element, or NULL if not found */ void *table_getc(table_t *table, char *cstr) { + if (table == NULL||cstr == NULL) + return NULL; // TODO This is very inefficient bstr *key = bstr_cstrdup(cstr); if (key == NULL) @@ -475,7 +477,7 @@ void *table_getc(table_t *table, char *cstr) { * @return table element, or NULL if not found */ void *table_get(table_t *table, bstr *key) { - if (table == NULL) + if (table == NULL||key == NULL) return NULL; // TODO This is very inefficient bstr *lkey = bstr_dup_lower(key); diff --git a/libhtp/htp/htp_connection.c b/libhtp/htp/htp_connection.c index f1055cdf0e..cd2a9a616e 100644 --- a/libhtp/htp/htp_connection.c +++ b/libhtp/htp/htp_connection.c @@ -58,25 +58,29 @@ void htp_conn_destroy(htp_conn_t *conn) { // using the iterator does not work here because some of the // list element may be NULL (and with the iterator it is impossible // to distinguish a NULL element from the end of the list). - size_t i; - for (i = 0; i < list_size(conn->transactions); i++) { - htp_tx_t *tx = (htp_tx_t *)list_get(conn->transactions, i); - if (tx != NULL) { - htp_tx_destroy(tx); + if (conn->transactions != NULL) { + size_t i; + for (i = 0; i < list_size(conn->transactions); i++) { + htp_tx_t *tx = (htp_tx_t *)list_get(conn->transactions, i); + if (tx != NULL) { + htp_tx_destroy(tx); + } } + + list_destroy(conn->transactions); } - - list_destroy(conn->transactions); // Destroy individual messages - htp_log_t *l = NULL; - list_iterator_reset(conn->messages); - while ((l = list_iterator_next(conn->messages)) != NULL) { - free((void *)l->msg); - free(l); - } + if (conn->messages != NULL) { + htp_log_t *l = NULL; + list_iterator_reset(conn->messages); + while ((l = list_iterator_next(conn->messages)) != NULL) { + free((void *)l->msg); + free(l); + } - list_destroy(conn->messages); + list_destroy(conn->messages); + } if (conn->local_addr != NULL) { free(conn->local_addr); @@ -101,7 +105,7 @@ void htp_conn_destroy(htp_conn_t *conn) { * @return 1 if transaction was removed or 0 if it wasn't found */ int htp_conn_remove_tx(htp_conn_t *conn, htp_tx_t *tx) { - if ((tx == NULL)||(conn == NULL)) return 0; + if ((tx == NULL)||(conn == NULL)||(conn->transactions == NULL)) return 0; unsigned int i = 0; for (i = 0; i < list_size(conn->transactions); i++) { diff --git a/libhtp/htp/htp_connection_parser.c b/libhtp/htp/htp_connection_parser.c index 5bd6c36e83..b349837386 100644 --- a/libhtp/htp/htp_connection_parser.c +++ b/libhtp/htp/htp_connection_parser.c @@ -135,6 +135,11 @@ htp_connp_t *htp_connp_create_copycfg(htp_cfg_t *cfg) { if (connp == NULL) return NULL; connp->cfg = htp_config_copy(cfg); + if (connp->cfg == NULL) { + htp_connp_destroy(connp); + return NULL; + } + connp->is_cfg_private = 1; return connp; @@ -160,7 +165,8 @@ void htp_connp_destroy(htp_connp_t *connp) { free(connp->in_header_line); } - free(connp->in_line); + if (connp->in_line != NULL) + free(connp->in_line); if (connp->out_header_line != NULL) { if (connp->out_header_line->line != NULL) { @@ -170,12 +176,14 @@ void htp_connp_destroy(htp_connp_t *connp) { free(connp->out_header_line); } - free(connp->out_line); + if (connp->out_line != NULL) + free(connp->out_line); // Destroy the configuration structure, but only // if it is our private copy if (connp->is_cfg_private) { - htp_config_destroy(connp->cfg); + if (connp->cfg != NULL) + htp_config_destroy(connp->cfg); } free(connp); @@ -188,15 +196,12 @@ void htp_connp_destroy(htp_connp_t *connp) { * @param connp */ void htp_connp_destroy_all(htp_connp_t *connp) { - if (connp->conn == NULL) { - fprintf(stderr, "HTP: htp_connp_destroy_all was invoked, but conn is NULL\n"); - return; + if (connp->conn != NULL) { + // Destroy connection + htp_conn_destroy(connp->conn); + connp->conn = NULL; } - // Destroy connection - htp_conn_destroy(connp->conn); - connp->conn = NULL; - // Destroy everything else htp_connp_destroy(connp); } diff --git a/libhtp/htp/htp_parsers.c b/libhtp/htp/htp_parsers.c index 5c4f3dd4f7..5d9bdc88ea 100644 --- a/libhtp/htp/htp_parsers.c +++ b/libhtp/htp/htp_parsers.c @@ -23,7 +23,7 @@ * @return Protocol version or PROTOCOL_UKNOWN. */ int htp_parse_protocol(bstr *protocol) { - if (bstr_len(protocol) == 8) { + if (protocol != NULL && bstr_len(protocol) == 8) { char *ptr = bstr_ptr(protocol); if ((ptr[0] == 'H') && (ptr[1] == 'T') && (ptr[2] == 'T') && (ptr[3] == 'P') && (ptr[4] == '/') && (ptr[6] == '.')) { diff --git a/libhtp/htp/htp_request.c b/libhtp/htp/htp_request.c index aed300f569..7de9f61fc1 100644 --- a/libhtp/htp/htp_request.c +++ b/libhtp/htp/htp_request.c @@ -266,7 +266,7 @@ int htp_connp_REQ_BODY_DETERMINE(htp_connp_t *connp) { // Check for the Transfer-Encoding header, which // would indicate a chunked request body - if (te != NULL) { + if (te != NULL && te->value != NULL) { // Make sure it contains "chunked" only if (bstr_cmpc(te->value, "chunked") != 0) { // Invalid T-E header value @@ -299,7 +299,7 @@ int htp_connp_REQ_BODY_DETERMINE(htp_connp_t *connp) { connp->in_tx->progress = TX_PROGRESS_REQ_BODY; } else // Next check for the presence of the Content-Length header - if (cl != NULL) { + if (cl != NULL && cl->value != NULL) { // It seems that we have a request body. connp->in_tx->request_transfer_coding = IDENTITY; diff --git a/libhtp/htp/htp_request_apache_2_2.c b/libhtp/htp/htp_request_apache_2_2.c index 9d053e60cb..c06f1b02cc 100644 --- a/libhtp/htp/htp_request_apache_2_2.c +++ b/libhtp/htp/htp_request_apache_2_2.c @@ -52,6 +52,13 @@ int htp_process_request_header_apache_2_2(htp_connp_t *connp) { for (i = connp->in_header_line_index; i < connp->in_header_line_counter; i++) { htp_header_line_t *hl = list_get(connp->in_tx->request_header_lines, i); + if (hl == NULL) { + // Internal error + htp_log(connp, HTP_LOG_MARK, HTP_LOG_ERROR, 0, + "Process request header (Apache 2.2): Internal error"); + free(h); + return HTP_ERROR; + } len += bstr_len(hl->line); } @@ -65,6 +72,14 @@ int htp_process_request_header_apache_2_2(htp_connp_t *connp) { for (i = connp->in_header_line_index; i < connp->in_header_line_counter; i++) { htp_header_line_t *hl = list_get(connp->in_tx->request_header_lines, i); + if (hl == NULL) { + // Internal error + htp_log(connp, HTP_LOG_MARK, HTP_LOG_ERROR, 0, + "Process request header (Apache 2.2): Internal error"); + bstr_free(tempstr); + free(h); + return HTP_ERROR; + } char *data = bstr_ptr(hl->line); size_t len = bstr_len(hl->line); htp_chomp((unsigned char *)data, &len); @@ -98,8 +113,10 @@ int htp_process_request_header_apache_2_2(htp_connp_t *connp) { bstr_add_str_noex(h_existing->value, h->value); // The header is no longer needed - free(h->name); - free(h->value); + if (h->name != NULL) + free(h->name); + if (h->value != NULL) + free(h->value); free(h); // Keep track of same-name headers @@ -276,6 +293,9 @@ int htp_parse_request_line_apache_2_2(htp_connp_t *connp) { } tx->request_uri = bstr_memdup((char *) data + start, pos - start); + if (tx->request_uri == NULL) { + return HTP_ERROR; + } #ifdef HTP_DEBUG fprint_raw_data(stderr, __FUNCTION__, (unsigned char *)bstr_ptr(tx->request_uri), bstr_len(tx->request_uri)); diff --git a/libhtp/htp/htp_transaction.c b/libhtp/htp/htp_transaction.c index 1de7a1b5d3..3b33891cfb 100644 --- a/libhtp/htp/htp_transaction.c +++ b/libhtp/htp/htp_transaction.c @@ -86,27 +86,31 @@ void htp_tx_destroy(htp_tx_t *tx) { // Destroy request_header_lines htp_header_line_t *hl = NULL; - list_iterator_reset(tx->request_header_lines); - while ((hl = list_iterator_next(tx->request_header_lines)) != NULL) { - bstr_free(hl->line); - bstr_free(hl->terminators); - // No need to destroy hl->header because - // htp_header_line_t does not own it. - free(hl); - } + if (tx->request_header_lines != NULL) { + list_iterator_reset(tx->request_header_lines); + while ((hl = list_iterator_next(tx->request_header_lines)) != NULL) { + bstr_free(hl->line); + bstr_free(hl->terminators); + // No need to destroy hl->header because + // htp_header_line_t does not own it. + free(hl); + } - list_destroy(tx->request_header_lines); + list_destroy(tx->request_header_lines); + } // Destroy request_headers htp_header_t *h = NULL; - table_iterator_reset(tx->request_headers); - while (table_iterator_next(tx->request_headers, (void **) & h) != NULL) { - bstr_free(h->name); - bstr_free(h->value); - free(h); - } + if (tx->request_headers != NULL) { + table_iterator_reset(tx->request_headers); + while (table_iterator_next(tx->request_headers, (void **) & h) != NULL) { + bstr_free(h->name); + bstr_free(h->value); + free(h); + } - table_destroy(tx->request_headers); + table_destroy(tx->request_headers); + } if (tx->request_headers_raw != NULL) { bstr_free(tx->request_headers_raw); @@ -119,25 +123,29 @@ void htp_tx_destroy(htp_tx_t *tx) { // Destroy response_header_lines hl = NULL; - list_iterator_reset(tx->response_header_lines); - while ((hl = list_iterator_next(tx->response_header_lines)) != NULL) { - bstr_free(hl->line); - bstr_free(hl->terminators); - // No need to destroy hl->header because - // htp_header_line_t does not own it. - free(hl); + if (tx->response_header_lines != NULL) { + list_iterator_reset(tx->response_header_lines); + while ((hl = list_iterator_next(tx->response_header_lines)) != NULL) { + bstr_free(hl->line); + bstr_free(hl->terminators); + // No need to destroy hl->header because + // htp_header_line_t does not own it. + free(hl); + } + list_destroy(tx->response_header_lines); } - list_destroy(tx->response_header_lines); // Destroy response headers h = NULL; - table_iterator_reset(tx->response_headers); - while (table_iterator_next(tx->response_headers, (void **) & h) != NULL) { - bstr_free(h->name); - bstr_free(h->value); - free(h); + if (tx->response_headers) { + table_iterator_reset(tx->response_headers); + while (table_iterator_next(tx->response_headers, (void **) & h) != NULL) { + bstr_free(h->name); + bstr_free(h->value); + free(h); + } + table_destroy(tx->response_headers); } - table_destroy(tx->response_headers); // Tell the connection to remove this transaction // from the list diff --git a/libhtp/htp/htp_util.c b/libhtp/htp/htp_util.c index 8e2864ce50..f2849221fd 100644 --- a/libhtp/htp/htp_util.c +++ b/libhtp/htp/htp_util.c @@ -147,6 +147,7 @@ int htp_is_space(int c) { * @return Method number of M_UNKNOWN */ int htp_convert_method_to_number(bstr *method) { + if (method == NULL) return M_UNKNOWN; // TODO Optimize using parallel matching, or something if (bstr_cmpc(method, "GET") == 0) return M_GET; if (bstr_cmpc(method, "PUT") == 0) return M_PUT; @@ -446,6 +447,8 @@ int htp_parse_authority(htp_connp_t *connp, bstr *authority, htp_uri_t **uri) { * @return HTP_ERROR on memory allocation failure, HTP_OK otherwise */ int htp_parse_uri(bstr *input, htp_uri_t **uri) { + if (input == NULL) + return HTP_ERROR; char *data = bstr_ptr(input); size_t len = bstr_len(input); size_t start, pos; @@ -920,6 +923,9 @@ int decode_u_encoding(htp_cfg_t *cfg, htp_tx_t *tx, unsigned char *data) { * @param path */ int htp_decode_path_inplace(htp_cfg_t *cfg, htp_tx_t *tx, bstr *path) { + if (path == NULL) + return -1; + unsigned char *data = (unsigned char *) bstr_ptr(path); if (data == NULL) { return -1; @@ -1228,22 +1234,23 @@ int htp_normalize_parsed_uri(htp_connp_t *connp, htp_uri_t *incomplete, htp_uri_ if (incomplete->path != NULL) { // Make a copy of the path, on which we can work on normalized->path = bstr_strdup(incomplete->path); + if (normalized->path != NULL) { + // Decode URL-encoded (and %u-encoded) characters, as well as lowercase, + // compress separators and convert backslashes. + htp_decode_path_inplace(connp->cfg, connp->in_tx, normalized->path); + + // Handle UTF-8 in path + if (connp->cfg->path_convert_utf8) { + // Decode Unicode characters into a single-byte stream, using best-fit mapping + htp_utf8_decode_path_inplace(connp->cfg, connp->in_tx, normalized->path); + } else { + // Only validate path as a UTF-8 stream + htp_utf8_validate_path(connp->in_tx, normalized->path); + } - // Decode URL-encoded (and %u-encoded) characters, as well as lowercase, - // compress separators and convert backslashes. - htp_decode_path_inplace(connp->cfg, connp->in_tx, normalized->path); - - // Handle UTF-8 in path - if (connp->cfg->path_convert_utf8) { - // Decode Unicode characters into a single-byte stream, using best-fit mapping - htp_utf8_decode_path_inplace(connp->cfg, connp->in_tx, normalized->path); - } else { - // Only validate path as a UTF-8 stream - htp_utf8_validate_path(connp->in_tx, normalized->path); + // RFC normalization + htp_normalize_uri_path_inplace(normalized->path); } - - // RFC normalization - htp_normalize_uri_path_inplace(normalized->path); } // Query