From 6a5d2cb40d1446da5dc26ed83bf1d959c510bf98 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 6 Dec 2010 15:53:38 +0100 Subject: [PATCH] Fix potential locking issue in out of memory conditions in the http_header, http_raw_header code. Fix other potential small issues in http_ code. --- src/detect-engine-hcbd.c | 29 ++++++++++++++++++--------- src/detect-engine-hhd.c | 43 +++++++++++++++++++++++++--------------- src/detect-engine-hrhd.c | 28 +++++++++++++++++--------- src/detect-engine-uri.c | 12 ++++++++--- src/detect.c | 2 -- 5 files changed, 75 insertions(+), 39 deletions(-) diff --git a/src/detect-engine-hcbd.c b/src/detect-engine-hcbd.c index 45387ae13a..a3e011debc 100644 --- a/src/detect-engine-hcbd.c +++ b/src/detect-engine-hcbd.c @@ -203,7 +203,12 @@ static int DoInspectHttpClientBody(DetectEngineCtx *de_ctx, goto match; } - BUG_ON(sm->next == NULL); + /* bail out if we have no next match. Technically this is an + * error, as the current cd has the DETECT_CONTENT_RELATIVE_NEXT + * flag set. */ + if (sm->next == NULL) { + SCReturnInt(0); + } /* see if the next payload keywords match. If not, we will * search for another occurence of this http client body content @@ -253,6 +258,8 @@ match: * * \retval cnt The match count from the mpm call. If call_mpm is 0, the retval * is ignored. + * + * \warning Make sure flow is locked. */ static uint32_t DetectEngineInspectHttpClientBodyMpmInspect(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, @@ -318,11 +325,12 @@ static uint32_t DetectEngineInspectHttpClientBodyMpmInspect(DetectEngineCtx *de_ * should change to handling chunks statefully */ if (chunks_buffer_len > de_ctx->hcbd_buffer_limit) break; + chunks_buffer_len += cur->len; if ( (chunks_buffer = SCRealloc(chunks_buffer, chunks_buffer_len)) == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); - exit(EXIT_FAILURE); + goto end; } + memcpy(chunks_buffer + chunks_buffer_len - cur->len, cur->data, cur->len); cur = cur->next; } @@ -336,6 +344,7 @@ static uint32_t DetectEngineInspectHttpClientBodyMpmInspect(DetectEngineCtx *de_ } /* else - if (htud->body.nchunks == 0) */ } /* for (idx = AppLayerTransactionGetInspectId(f); .. */ +end: SCReturnUInt(cnt); } @@ -371,8 +380,8 @@ int DetectEngineInspectHttpClientBody(DetectEngineCtx *de_ctx, /* locking the flow, we will inspect the htp state */ SCMutexLock(&f->m); - if (htp_state->connp == NULL) { - SCLogDebug("HTP state has no connp"); + if (htp_state->connp == NULL || htp_state->connp->conn == NULL) { + SCLogDebug("HTP state has no conn(p)"); goto end; } @@ -395,14 +404,14 @@ int DetectEngineInspectHttpClientBody(DetectEngineCtx *de_ctx, /* assign space to hold buffers. Each per transaction */ det_ctx->hcbd_buffers = SCMalloc(det_ctx->hcbd_buffers_list_len * sizeof(uint8_t *)); if (det_ctx->hcbd_buffers == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); + r = 0; goto end; } memset(det_ctx->hcbd_buffers, 0, det_ctx->hcbd_buffers_list_len * sizeof(uint8_t *)); det_ctx->hcbd_buffers_len = SCMalloc(det_ctx->hcbd_buffers_list_len * sizeof(uint32_t)); if (det_ctx->hcbd_buffers_len == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); + r = 0; goto end; } memset(det_ctx->hcbd_buffers_len, 0, det_ctx->hcbd_buffers_list_len * sizeof(uint32_t)); @@ -470,8 +479,10 @@ void DetectEngineCleanHCBDBuffers(DetectEngineThreadCtx *det_ctx) if (det_ctx->hcbd_buffers[i] != NULL) SCFree(det_ctx->hcbd_buffers[i]); } - SCFree(det_ctx->hcbd_buffers); - det_ctx->hcbd_buffers = NULL; + if (det_ctx->hcbd_buffers != NULL) { + SCFree(det_ctx->hcbd_buffers); + det_ctx->hcbd_buffers = NULL; + } det_ctx->hcbd_buffers_list_len = 0; } diff --git a/src/detect-engine-hhd.c b/src/detect-engine-hhd.c index 1d0354c8c2..72e29e0267 100644 --- a/src/detect-engine-hhd.c +++ b/src/detect-engine-hhd.c @@ -205,7 +205,12 @@ static int DoInspectHttpHeader(DetectEngineCtx *de_ctx, goto match; } - BUG_ON(sm->next == NULL); + /* bail out if we have no next match. Technically this is an + * error, as the current cd has the DETECT_CONTENT_RELATIVE_NEXT + * flag set. */ + if (sm->next == NULL) { + SCReturnInt(0); + } /* see if the next payload keywords match. If not, we will * search for another occurence of this http header content and @@ -251,10 +256,12 @@ match: * buffers. * * \param det_ctx Detection engine thread ctx. - * \param f Pointer to the flow. + * \param f Pointer to the locked flow. * \param htp_state http state. * * \retval cnt The match count from the mpm call. + * + * \warning Make sure flow is locked. */ static uint32_t DetectEngineInspectHttpHeaderMpmInspect(DetectEngineThreadCtx *det_ctx, Signature *s, Flow *f, @@ -284,17 +291,19 @@ static uint32_t DetectEngineInspectHttpHeaderMpmInspect(DetectEngineThreadCtx *d htp_header_t *h = NULL; uint8_t *headers_buffer = NULL; - uint32_t headers_buffer_len = 0; + size_t headers_buffer_len = 0; + table_iterator_reset(tx->request_headers); while (table_iterator_next(tx->request_headers, (void **)&h) != NULL) { - int size1 = bstr_size(h->name); - int size2 = bstr_size(h->value); + size_t size1 = bstr_size(h->name); + size_t size2 = bstr_size(h->value); + /* the extra 4 bytes if for ": " and "\r\n" */ - headers_buffer = realloc(headers_buffer, headers_buffer_len + size1 + size2 + 4); + headers_buffer = SCRealloc(headers_buffer, headers_buffer_len + size1 + size2 + 4); if (headers_buffer == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory\n"); - exit(EXIT_FAILURE); + continue; } + memcpy(headers_buffer + headers_buffer_len, bstr_ptr(h->name), size1); headers_buffer_len += size1; headers_buffer[headers_buffer_len] = ':'; @@ -352,8 +361,8 @@ int DetectEngineInspectHttpHeader(DetectEngineCtx *de_ctx, /* locking the flow, we will inspect the htp state */ SCMutexLock(&f->m); - if (htp_state->connp == NULL) { - SCLogDebug("HTP state has no connp"); + if (htp_state->connp == NULL || htp_state->connp->conn == NULL) { + SCLogDebug("HTP state has no conn(p)"); goto end; } @@ -376,15 +385,15 @@ int DetectEngineInspectHttpHeader(DetectEngineCtx *de_ctx, /* assign space to hold buffers. Each per transaction */ det_ctx->hhd_buffers = SCMalloc(det_ctx->hhd_buffers_list_len * sizeof(uint8_t *)); if (det_ctx->hhd_buffers == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); - return 0; + r = 0; + goto end; } memset(det_ctx->hhd_buffers, 0, det_ctx->hhd_buffers_list_len * sizeof(uint8_t *)); det_ctx->hhd_buffers_len = SCMalloc(det_ctx->hhd_buffers_list_len * sizeof(uint32_t)); if (det_ctx->hhd_buffers_len == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); - return 0; + r = 0; + goto end; } memset(det_ctx->hhd_buffers_len, 0, det_ctx->hhd_buffers_list_len * sizeof(uint32_t)); } /* if (det_ctx->hhd_buffers_list_len == 0) */ @@ -449,8 +458,10 @@ void DetectEngineCleanHHDBuffers(DetectEngineThreadCtx *det_ctx) if (det_ctx->hhd_buffers[i] != NULL) SCFree(det_ctx->hhd_buffers[i]); } - SCFree(det_ctx->hhd_buffers); - det_ctx->hhd_buffers = NULL; + if (det_ctx->hhd_buffers != NULL) { + SCFree(det_ctx->hhd_buffers); + det_ctx->hhd_buffers = NULL; + } det_ctx->hhd_buffers_list_len = 0; } diff --git a/src/detect-engine-hrhd.c b/src/detect-engine-hrhd.c index b60531f866..f9fd1d987a 100644 --- a/src/detect-engine-hrhd.c +++ b/src/detect-engine-hrhd.c @@ -205,7 +205,12 @@ static int DoInspectHttpRawHeader(DetectEngineCtx *de_ctx, goto match; } - BUG_ON(sm->next == NULL); + /* bail out if we have no next match. Technically this is an + * error, as the current cd has the DETECT_CONTENT_RELATIVE_NEXT + * flag set. */ + if (sm->next == NULL) { + SCReturnInt(0); + } /* see if the next payload keywords match. If not, we will * search for another occurence of this http raw header content @@ -255,6 +260,8 @@ match: * \param htp_state http state. * * \retval cnt The match count from the mpm call. + * + * \warning Make sure the flow is locked. */ static uint32_t DetectEngineInspectHttpRawHeaderMpmInspect(DetectEngineThreadCtx *det_ctx, Signature *s, Flow *f, @@ -285,6 +292,7 @@ static uint32_t DetectEngineInspectHttpRawHeaderMpmInspect(DetectEngineThreadCtx bstr *raw_headers = htp_tx_get_request_headers_raw(tx); if (raw_headers == NULL) continue; + /* store the buffers. We will need it for further inspection */ det_ctx->hrhd_buffers[i] = (uint8_t *)bstr_ptr(raw_headers); det_ctx->hrhd_buffers_len[i] = bstr_len(raw_headers); @@ -330,8 +338,8 @@ int DetectEngineInspectHttpRawHeader(DetectEngineCtx *de_ctx, /* locking the flow, we will inspect the htp state */ SCMutexLock(&f->m); - if (htp_state->connp == NULL) { - SCLogDebug("HTP state has no connp"); + if (htp_state->connp == NULL || htp_state->connp->conn == NULL) { + SCLogDebug("HTP state has no conn(p)"); goto end; } @@ -354,15 +362,15 @@ int DetectEngineInspectHttpRawHeader(DetectEngineCtx *de_ctx, /* assign space to hold buffers. Each per transaction */ det_ctx->hrhd_buffers = SCMalloc(det_ctx->hrhd_buffers_list_len * sizeof(uint8_t *)); if (det_ctx->hrhd_buffers == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); - return 0; + r = 0; + goto end; } memset(det_ctx->hrhd_buffers, 0, det_ctx->hrhd_buffers_list_len * sizeof(uint8_t *)); det_ctx->hrhd_buffers_len = SCMalloc(det_ctx->hrhd_buffers_list_len * sizeof(uint32_t)); if (det_ctx->hrhd_buffers_len == NULL) { - SCLogError(SC_ERR_MEM_ALLOC, "Error allocating memory"); - return 0; + r = 0; + goto end; } memset(det_ctx->hrhd_buffers_len, 0, det_ctx->hrhd_buffers_list_len * sizeof(uint32_t)); } /* if (det_ctx->hrhd_buffers_list_len == 0) */ @@ -422,8 +430,10 @@ end: void DetectEngineCleanHRHDBuffers(DetectEngineThreadCtx *det_ctx) { if (det_ctx->hrhd_buffers_list_len != 0) { - SCFree(det_ctx->hrhd_buffers); - det_ctx->hrhd_buffers = NULL; + if (det_ctx->hrhd_buffers != NULL) { + SCFree(det_ctx->hrhd_buffers); + det_ctx->hrhd_buffers = NULL; + } det_ctx->hrhd_buffers_list_len = 0; } diff --git a/src/detect-engine-uri.c b/src/detect-engine-uri.c index dedaa0a9c4..c90c4edcce 100644 --- a/src/detect-engine-uri.c +++ b/src/detect-engine-uri.c @@ -227,7 +227,13 @@ static int DoInspectPacketUri(DetectEngineCtx *de_ctx, goto match; } - BUG_ON(sm->next == NULL); + /* bail out if we have no next match. Technically this is an + * error, as the current cd has the DETECT_CONTENT_RELATIVE_NEXT + * flag set. */ + if (sm->next == NULL) { + SCReturnInt(0); + } + SCLogDebug("uricontent %"PRIu32, ud->id); /* see if the next payload keywords match. If not, we will @@ -366,8 +372,8 @@ int DetectEngineInspectPacketUris(DetectEngineCtx *de_ctx, /* locking the flow, we will inspect the htp state */ SCMutexLock(&f->m); - if (htp_state->connp == NULL) { - SCLogDebug("HTP state has no connp"); + if (htp_state->connp == NULL || htp_state->connp->conn == NULL) { + SCLogDebug("HTP state has no conn(p)"); goto end; } diff --git a/src/detect.c b/src/detect.c index 56a559d668..d0f32dcfae 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1329,9 +1329,7 @@ end: } /* cleanup pkt specific part of the patternmatcher */ - //if (sms_runflags & SMS_USED_PM) { PacketPatternCleanup(th_v, det_ctx); - //} DetectEngineCleanHCBDBuffers(det_ctx); DetectEngineCleanHHDBuffers(det_ctx);