From e0795b90a14feae0d67ef474f1b315006b4d2895 Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Tue, 12 Mar 2019 16:50:53 -0700 Subject: [PATCH 1/3] Only set APR request body if non-empty. If we set EOS bucket to the brigade and assign this brigade to APR request, it causes an error code returned from modsecProcessRequest(). This does not affect detection-only mode because we dismiss the code but needs to be taken into account for prevention mode. Signed-off-by: Vladimir Krivopalov --- iis/mymodule.cpp | 90 +++++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 43 deletions(-) diff --git a/iis/mymodule.cpp b/iis/mymodule.cpp index fddf37f8b..b7dd3c8ea 100644 --- a/iis/mymodule.cpp +++ b/iis/mymodule.cpp @@ -448,58 +448,62 @@ static HRESULT SaveRequestBodyToRequestRec(RequestStoredContext* rsc) APR_BRIGADE_INSERT_TAIL(brigade, bucket); } - apr_bucket* e = apr_bucket_eos_create(conn->bucket_alloc); - if (e == nullptr) - { - return E_OUTOFMEMORY; - } - APR_BRIGADE_INSERT_TAIL(brigade, e); + if (!APR_BRIGADE_EMPTY(brigade)) { + apr_bucket* e = apr_bucket_eos_create(conn->bucket_alloc); + if (e == nullptr) + { + return E_OUTOFMEMORY; + } + APR_BRIGADE_INSERT_TAIL(brigade, e); - modsecSetBodyBrigade(aprRequest, brigade); + modsecSetBodyBrigade(aprRequest, brigade); - apr_off_t contentLength = 0; - apr_status_t status = apr_brigade_length(brigade, FALSE, &contentLength); - if (status != APR_SUCCESS) - { - return E_FAIL; - } + apr_off_t contentLength = 0; + apr_status_t status = apr_brigade_length(brigade, FALSE, &contentLength); + if (status != APR_SUCCESS) + { + return E_FAIL; + } - // Remove/Modify Transfer-Encoding header if "chunked" Encoding is set in the request. - // This is to avoid sending both Content-Length and Chunked Transfer-Encoding in the request header. - static const std::string CHUNKED = "chunked"; - USHORT encodingLength = 0; - const char* transferEncoding = iisRequest->GetHeader(HttpHeaderTransferEncoding, &encodingLength); - if (transferEncoding) - { - if (CHUNKED.size() == encodingLength && - std::equal(CHUNKED.cbegin(), CHUNKED.cend(), transferEncoding, - [](char lhs, char rhs) { return std::tolower(lhs) == std::tolower(rhs); })) + // Remove/Modify Transfer-Encoding header if "chunked" Encoding is set in the request. + // This is to avoid sending both Content-Length and Chunked Transfer-Encoding in the request header. + static const std::string CHUNKED = "chunked"; + USHORT encodingLength = 0; + const char* transferEncoding = iisRequest->GetHeader(HttpHeaderTransferEncoding, &encodingLength); + if (transferEncoding) { - iisRequest->DeleteHeader(HttpHeaderTransferEncoding); + if (encodingLength == CHUNKED.size() && + std::equal(CHUNKED.cbegin(), CHUNKED.cend(), transferEncoding, + [](char lhs, char rhs) { return std::tolower(lhs) == std::tolower(rhs); })) + { + iisRequest->DeleteHeader(HttpHeaderTransferEncoding); + } } - } - auto contentLengthStr = std::to_string(contentLength); - HRESULT hr = iisRequest->SetHeader( - HttpHeaderContentLength, - contentLengthStr.c_str(), - contentLengthStr.size(), - TRUE); + auto contentLengthStr = std::to_string(contentLength); + HRESULT hr = iisRequest->SetHeader( + HttpHeaderContentLength, + contentLengthStr.c_str(), + contentLengthStr.size(), + TRUE); - if (FAILED(hr)) - { - return hr; - } + if (FAILED(hr)) + { + return hr; + } - // since we clean the APR pool at the end of OnSendRequest, we must get IIS-managed memory chunk - // - char* requestBuffer = static_cast(rsc->httpContext->AllocateRequestMemory(contentLength)); - status = apr_brigade_flatten(brigade, requestBuffer, reinterpret_cast(&contentLength)); - if (status != APR_SUCCESS) - { - return E_FAIL; + // since we clean the APR pool at the end of OnSendRequest, we must get IIS-managed memory chunk + // + char* requestBuffer = static_cast(rsc->httpContext->AllocateRequestMemory(contentLength)); + status = apr_brigade_flatten(brigade, requestBuffer, reinterpret_cast(&contentLength)); + if (status != APR_SUCCESS) + { + return E_FAIL; + } + return iisRequest->InsertEntityBody(requestBuffer, contentLength); } - return iisRequest->InsertEntityBody(requestBuffer, contentLength); + + return S_OK; } From 3fa2caceb958eef57661a06120a2741272adf5ea Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Tue, 12 Mar 2019 16:55:00 -0700 Subject: [PATCH 2/3] Explicitly copy request body into Apache structures for any mode. Use the same helper that has been previously only called for detection-only mode when prevention mode is enabled. Signed-off-by: Vladimir Krivopalov --- iis/mymodule.cpp | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/iis/mymodule.cpp b/iis/mymodule.cpp index b7dd3c8ea..1bb2c400d 100644 --- a/iis/mymodule.cpp +++ b/iis/mymodule.cpp @@ -957,17 +957,6 @@ CMyHttpModule::OnBeginRequest(IHttpContext* httpContext, IHttpEventProvider* pro delete apppath; delete path; - - if (config->config->is_enabled == MODSEC_DETECTION_ONLY) - { - modsecSetReadBody(nullptr); - modsecSetWriteBody(nullptr); - } - else - { - modsecSetReadBody(ReadBodyCallback); - modsecSetWriteBody(WriteBodyCallback); - } } ConnRecPtr connRec = MakeConnReq(); @@ -1218,13 +1207,14 @@ CMyHttpModule::OnBeginRequest(IHttpContext* httpContext, IHttpEventProvider* pro #endif c->remote_host = NULL; + hr = SaveRequestBodyToRequestRec(context); + if (FAILED(hr)) { + context->provider->SetErrorStatus(hr); + return RQ_NOTIFICATION_FINISH_REQUEST; + } + if (config->config->is_enabled == MODSEC_DETECTION_ONLY) { - hr = SaveRequestBodyToRequestRec(context); - if (FAILED(hr)) { - context->provider->SetErrorStatus(hr); - return RQ_NOTIFICATION_FINISH_REQUEST; - } // We post the processing task to the thread pool to happen in the background. // We store the future to track and wait for this processing in case if we also // need to process the response because we need request processing to finish From 87bd3ad69cdcc3b1bba650f885d4277fe3fbe861 Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Tue, 12 Mar 2019 16:56:36 -0700 Subject: [PATCH 3/3] Do not enforce stream in-body inspection flag for IIS. Signed-off-by: Vladimir Krivopalov --- iis/mymodule.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/iis/mymodule.cpp b/iis/mymodule.cpp index 1bb2c400d..071a6ec91 100644 --- a/iis/mymodule.cpp +++ b/iis/mymodule.cpp @@ -972,11 +972,6 @@ CMyHttpModule::OnBeginRequest(IHttpContext* httpContext, IHttpEventProvider* pro rsc->responseProcessingEnabled = (config->config->resbody_access == 1); RequestStoredContext* context = rsc.get(); - // on IIS we force input stream inspection flag, because its absence does not add any performance gain - // it's because on IIS request body must be restored each time it was read - // - modsecSetConfigForIISRequestBody(r); - StoreIISContext(r, rsc.get()); httpContext->GetModuleContextContainer()->SetModuleContext(rsc.release(), g_pModuleContext);