Skip to content

Commit 7526371

Browse files
vkrivopalovvkrivopalov
authored
Revert "Process request in background when in detection mode." (#108)
Let's give it more testing before rolling the fix out in production as we've been hit badly with a bug in a similar fix for Nginx. This reverts commit 2aa9412.
1 parent b3ed662 commit 7526371

File tree

3 files changed

+21
-144
lines changed

3 files changed

+21
-144
lines changed

‎iis/Makefile.win‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,7 @@ INCLUDES = -I. -I.. \
3737
-I..\apache2 \
3838
-I..\standalone \
3939
-I..\apache2\ag_mdb \
40-
-I..\apache2\waf_lock \
41-
-Iasio\asio\include
40+
-I..\apache2\waf_lock
4241

4342
# Enables support for SecRemoteRules and external resources.
4443
DEFS=$(DEFS) -DWITH_CURL -DWITH_REMOTE_RULES

‎iis/mymodule.cpp‎

Lines changed: 17 additions & 137 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
#include <string>
2020
#include <cctype>
2121
#include <memory>
22-
#include <future>
2322

2423
// IIS7 Server API header file
2524
#include <Windows.h>
@@ -36,9 +35,6 @@
3635

3736
#include "winsock2.h"
3837

39-
#include "asio/packaged_task.hpp"
40-
#include "asio/post.hpp"
41-
4238
// These helpers make sure that the underlying structures
4339
// are correctly released on any exit path due to RAII.
4440
using ConnRecPtr = std::shared_ptr<conn_rec>;
@@ -66,8 +62,6 @@ struct RequestStoredContext
6662
char* responseBuffer = nullptr;
6763
ULONGLONG responseLength = 0;
6864
ULONGLONG responsePosition = 0;
69-
bool responseProcessingEnabled = false;
70-
std::future<int> detectionProcessing;
7165

7266
void CleanupStoredContext() override
7367
{
@@ -506,97 +500,6 @@ static HRESULT SaveRequestBodyToRequestRec(RequestStoredContext* rsc)
506500
return S_OK;
507501
}
508502

509-
510-
static apr_status_t ReadBodyCallback(request_rec *r, char *buf, unsigned int length, unsigned int *readcnt, int *is_eos)
511-
{
512-
RequestStoredContext *rsc = RetrieveIISContext(r);
513-
514-
*readcnt = 0;
515-
516-
if (rsc == NULL)
517-
{
518-
*is_eos = 1;
519-
return APR_SUCCESS;
520-
}
521-
522-
IHttpContext *pHttpContext = rsc->httpContext;
523-
IHttpRequest *pRequest = pHttpContext->GetRequest();
524-
525-
if (pRequest->GetRemainingEntityBytes() == 0)
526-
{
527-
*is_eos = 1;
528-
return APR_SUCCESS;
529-
}
530-
531-
HRESULT hr = pRequest->ReadEntityBody(buf, length, false, (DWORD *)readcnt, NULL);
532-
533-
if (FAILED(hr))
534-
{
535-
// End of data is okay.
536-
if (ERROR_HANDLE_EOF != (hr & 0x0000FFFF))
537-
{
538-
// Set the error status.
539-
rsc->provider->SetErrorStatus(hr);
540-
}
541-
542-
*is_eos = 1;
543-
}
544-
545-
return APR_SUCCESS;
546-
}
547-
548-
static apr_status_t WriteBodyCallback(request_rec *r, char *buf, unsigned int length)
549-
{
550-
RequestStoredContext *rsc = RetrieveIISContext(r);
551-
552-
if (!rsc || !rsc->requestRec)
553-
return APR_SUCCESS;
554-
555-
IHttpContext *pHttpContext = rsc->httpContext;
556-
IHttpRequest *pHttpRequest = pHttpContext->GetRequest();
557-
558-
auto lengthStr = std::to_string(length);
559-
560-
// Remove/Modify Transfer-Encoding header if "chunked" Encoding is set in the request.
561-
// This is to avoid sending both Content-Length and Chunked Transfer-Encoding in the request header.
562-
563-
USHORT ctcch = 0;
564-
char *ct = (char *)pHttpRequest->GetHeader(HttpHeaderTransferEncoding, &ctcch);
565-
if (ct)
566-
{
567-
char *ctz = ZeroTerminate(ct, ctcch, r->pool);
568-
if (ctcch != 0)
569-
{
570-
if (0 == stricmp(ctz, "chunked"))
571-
{
572-
pHttpRequest->DeleteHeader(HttpHeaderTransferEncoding);
573-
}
574-
}
575-
}
576-
577-
HRESULT hr = pHttpRequest->SetHeader(
578-
HttpHeaderContentLength,
579-
lengthStr.c_str(),
580-
(USHORT)lengthStr.size(),
581-
TRUE);
582-
583-
if (FAILED(hr))
584-
{
585-
// possible, but there's nothing we can do
586-
}
587-
588-
// since we clean the APR pool at the end of OnSendRequest, we must get IIS-managed memory chunk
589-
//
590-
void *reqbuf = pHttpContext->AllocateRequestMemory(length);
591-
592-
memcpy(reqbuf, buf, length);
593-
594-
pHttpRequest->InsertEntityBody(reqbuf, length);
595-
596-
return APR_SUCCESS;
597-
}
598-
599-
600503
REQUEST_NOTIFICATION_STATUS
601504
CMyHttpModule::OnSendResponse(
602505
IN IHttpContext * pHttpContext,
@@ -606,18 +509,12 @@ CMyHttpModule::OnSendResponse(
606509
RequestStoredContext* rsc = dynamic_cast<RequestStoredContext*>(pHttpContext->GetModuleContextContainer()->GetModuleContext(g_pModuleContext));
607510

608511
CriticalSectionLock lock{cs};
609-
// here we must check if response body processing is enabled
610-
//
611-
if (!rsc || !rsc->requestRec || rsc->responseBuffer != nullptr || !rsc->responseProcessingEnabled)
612-
{
613-
goto Exit;
614-
}
615-
616-
// If we have offloaded request processing to the thread pool
617-
// we need to wait for its completion before we can proceed
618-
if (rsc->detectionProcessing.valid()) {
619-
rsc->detectionProcessing.get();
620-
}
512+
// here we must check if response body processing is enabled
513+
//
514+
if(!rsc || !rsc->requestRec || rsc->responseBuffer != nullptr || !modsecIsResponseBodyAccessEnabled(rsc->requestRec.get()))
515+
{
516+
goto Exit;
517+
}
621518

622519
HRESULT hr = S_OK;
623520
IHttpResponse *pHttpResponse = NULL;
@@ -968,8 +865,7 @@ CMyHttpModule::OnBeginRequest(IHttpContext* httpContext, IHttpEventProvider* pro
968865
rsc->connRec = connRec;
969866
rsc->requestRec = requestRec;
970867
rsc->httpContext = httpContext;
971-
rsc->provider = provider;
972-
rsc->responseProcessingEnabled = (config->config->resbody_access == 1);
868+
rsc->provider = provider;
973869
RequestStoredContext* context = rsc.get();
974870

975871
StoreIISContext(r, rsc.get());
@@ -1208,35 +1104,19 @@ CMyHttpModule::OnBeginRequest(IHttpContext* httpContext, IHttpEventProvider* pro
12081104
return RQ_NOTIFICATION_FINISH_REQUEST;
12091105
}
12101106

1211-
if (config->config->is_enabled == MODSEC_DETECTION_ONLY)
1212-
{
1213-
// We post the processing task to the thread pool to happen in the background.
1214-
// We store the future to track and wait for this processing in case if we also
1215-
// need to process the response because we need request processing to finish
1216-
// before we can start with the response.
1217-
context->detectionProcessing = asio::post(threadPool,
1218-
std::packaged_task<int()> {
1219-
[connRec, requestRec] {
1220-
return modsecProcessRequest(requestRec.get());
1221-
}
1222-
});
1223-
}
1224-
else
1225-
{
1226-
lock.unlock();
1227-
int status = modsecProcessRequest(r);
1228-
lock.lock();
1107+
lock.unlock();
1108+
int status = modsecProcessRequest(r);
1109+
lock.lock();
12291110

1230-
if (status != DECLINED)
1231-
{
1232-
httpContext->GetResponse()->SetStatus(status, "ModSecurity Action");
1233-
httpContext->SetRequestHandled();
1111+
if(status != DECLINED)
1112+
{
1113+
httpContext->GetResponse()->SetStatus(status, "ModSecurity Action");
1114+
httpContext->SetRequestHandled();
12341115

1235-
return RQ_NOTIFICATION_FINISH_REQUEST;
1236-
}
1237-
}
1116+
return RQ_NOTIFICATION_FINISH_REQUEST;
1117+
}
12381118

1239-
return RQ_NOTIFICATION_CONTINUE;
1119+
return RQ_NOTIFICATION_CONTINUE;
12401120
}
12411121

12421122
apr_status_t ReadResponseCallback(request_rec *r, char *buf, unsigned int length, unsigned int *readcnt, int *is_eos)

‎iis/mymodule.h‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,8 @@
1212
* directly using the email address [email protected].
1313
*/
1414

15-
#pragma once
16-
17-
#define ASIO_STANDALONE
18-
#include "asio/thread_pool.hpp"
15+
#ifndef __MY_MODULE_H__
16+
#define __MY_MODULE_H__
1917

2018
#include "critical_section.h"
2119
#include "event_logger.h"
@@ -48,8 +46,8 @@ class CMyHttpModule
4846
private:
4947
CriticalSection cs;
5048
EventLogger logger;
51-
asio::thread_pool threadPool;
5249
DWORD pageSize = 0;
5350
bool statusCallAlreadySent = false;
5451
};
5552

53+
#endif

0 commit comments

Comments
 (0)