From 83f732437f0b3687aac59bb2f3a65b760475cff5 Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Thu, 17 Nov 2022 14:10:02 +0100 Subject: [PATCH 01/10] don't reuse buffer --- src/vmod_sec.c | 52 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index 1c2764d..bdf0326 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -9,6 +9,7 @@ typedef struct Rules_t RulesSet; #endif #include +#define VMOD_SEC_DEBUG 1 #include #include @@ -136,12 +137,16 @@ VCL_VOID v_matchproto_(td_sec_sec__init) int error; (void)vcl_name; + + if (ctx->method != VCL_MET_INIT) { + VRT_fail(ctx, "[vmodsec] - init can only be called from vcl_init{}"); + } /* Sanity check */ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); AN(vpp); AZ(*vpp); - VSL(SLT_Error, 0, "[vmodsec] - object [%s] initialized using modsecurity %s", + VSL(SLT_Debug, 0, "[vmodsec] - object [%s] initialized using modsecurity %s", vcl_name, MODSECURITY_VERSION); modsec = msc_init(); @@ -188,6 +193,11 @@ VCL_INT v_matchproto_(td_sec_sec_add_rule) const char *error = NULL; VSL(SLT_Debug, 0, "[vmodsec] - [%s] - VCL provided rule", rule); CHECK_OBJ_NOTNULL(vp, VMOD_SEC_SEC_MAGIC_BITS); + + if (ctx->method != VCL_MET_INIT) { + VRT_fail(ctx, "[vmodsec] - .add_rule can only be called from vcl_init{}"); + } + ret = msc_rules_add(vp->rules_set, rule, &error); if (ret < 0) { @@ -208,6 +218,10 @@ VCL_INT v_matchproto_(td_sec_sec_add_rules) { int ret; const char *error = NULL; + if (ctx->method != VCL_MET_INIT) { + VRT_fail(ctx, "[vmodsec] - .add_rules can only be called from vcl_init{}"); + } + VSL(SLT_Debug, 0, "[vmodsec] - [%s] - Try to load the rules", args->rules_path); CHECK_OBJ_NOTNULL(vp, VMOD_SEC_SEC_MAGIC_BITS); @@ -271,6 +285,10 @@ VCL_INT v_matchproto_(td_sec_sec_new_conn) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); CHECK_OBJ_NOTNULL(vp, VMOD_SEC_SEC_MAGIC_BITS); + if (ctx->method != VCL_MET_RECV) { + VRT_fail(ctx, "[vmodsec] - new_conn can only be called from vcl_recv{}"); + } + struct vmod_priv *p; if (args->arg1 == NULL) { return 0; @@ -337,6 +355,7 @@ VCL_INT v_matchproto_(td_sec_sec_process_url) VSL(SLT_Error, ctx->sp->vxid, "[vmodsec] - connection has not been started, closing"); return -1; } + struct vmod_sec_struct_trans_int *transInt = (struct vmod_sec_struct_trans_int *)priv->priv; /* This will be used to Initialise the original URL */ msc_process_uri(transInt->trans, req_url, protocol, http_version); @@ -356,12 +375,13 @@ VCL_INT v_matchproto_(td_sec_sec_process_url) #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Found %d headers, Start at %d, need to ingest %d headers", hp->nhd, HTTP_HDR_FIRST, hp->nhd - HTTP_HDR_FIRST); #endif - // Freed after loop - char *headerName = malloc(8192); - char *headerValue = malloc(8192); + int headerCount = hp->nhd - HTTP_HDR_FIRST; + char **headersNames = (char **)malloc(sizeof(char *) * headerCount); + char **headersValues = (char **)malloc(sizeof(char *) * headerCount); for (u = HTTP_HDR_FIRST; u < hp->nhd; u++) { + int headerPos = u-HTTP_HDR_FIRST; Tcheck(hp->hd[u]); const char *header = hp->hd[u].b; long int hlen = strlen(header); @@ -372,24 +392,32 @@ VCL_INT v_matchproto_(td_sec_sec_process_url) continue; } /* Copy headers */ - strncpy(headerName, header, pos); - headerName[pos] = '\0'; + headersNames[headerPos] = (char *)malloc(pos+1); + strncpy(headersNames[headerPos], header, pos); + headersNames[headerPos][pos] = '\0'; // Find spaces pos += 1 /* : */ + strspn(&header[pos + 1], " \r\n\t"); // LWS = [CRLF] 1*( SP | HT ) chr(9,10,13,32) - strncpy(headerValue, &header[pos], hlen - pos); - headerValue[hlen - pos] = '\0'; - msc_add_request_header(transInt->trans, headerName, headerValue); + // Copy value + headersValues[headerPos] = (char *)malloc(hlen - pos +1); + strncpy(headersValues[headerPos], &header[pos], hlen - pos); + headersValues[headerPos][hlen - pos] = '\0'; + // FIXME : use msc_add_n_request_header + msc_add_request_header(transInt->trans, headersNames[headerPos], headersValues[headerPos]); #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, - "[vmodsec] - Additional header provided %s: %s", headerName, headerValue); + "[vmodsec] - Additional header provided %s: %s", headersNames[headerPos], headersValues[headerPos]); #endif } - free(headerName); - free(headerValue); #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Processing Request Headers"); #endif msc_process_request_headers(transInt->trans); + for (u = 0; u Date: Thu, 17 Nov 2022 14:30:09 +0100 Subject: [PATCH 02/10] fix: test, as add_rules should be in init --- src/vtc/vmod_sec_load_remote_rule.vtc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/vtc/vmod_sec_load_remote_rule.vtc b/src/vtc/vmod_sec_load_remote_rule.vtc index 7263a57..95e71ca 100644 --- a/src/vtc/vmod_sec_load_remote_rule.vtc +++ b/src/vtc/vmod_sec_load_remote_rule.vtc @@ -1,25 +1,25 @@ varnishtest "Check if we got the version string" server s1 { - rxreq - txresp + rxreq + txresp } -start varnish v1 -vcl+backend { import sec; - sub vcl_init { - new xsec = sec.sec(); - - } + sub vcl_init { + new xsec = sec.sec(); + xsec.add_rules("https://www.modsecurity.org/modsecurity-regression-test-secremoterules.txt", "test"); + } sub vcl_deliver { - set resp.http.X-ModSec-RulesCouns = xsec.add_rules("https://www.modsecurity.org/modsecurity-regression-test-secremoterules.txt", "test"); + set resp.http.X-ModSec-RulesCount = 50; } } -start client c1 { - txreq - rxresp - expect resp.status == 200 - expect resp.http.X-ModSec-RulesCouns ~ ^[0-9]+$ + txreq + rxresp + expect resp.status == 200 + expect resp.http.X-ModSec-RulesCount ~ ^[0-9]+$ } -run From 63f075590105530210c76de00a3e38e7026b697a Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Thu, 17 Nov 2022 15:33:45 +0100 Subject: [PATCH 03/10] fix correct response headers --- src/vmod_sec.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index bdf0326..add061a 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -509,12 +509,14 @@ VCL_INT v_matchproto_(td_sec_sec_process_response) VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Found %d headers, Start at %d, need to ingest %d headers", hp->nhd, HTTP_HDR_FIRST, hp->nhd - HTTP_HDR_FIRST); #endif + int headerCount = hp->nhd - HTTP_HDR_FIRST; // freed after loop - char *headerName = malloc(8192); - char *headerValue = malloc(8192); + char **headersNames = (char **)malloc(sizeof(char *) * headerCount); + char **headersValues = (char **)malloc(sizeof(char *) * headerCount);; for (u = HTTP_HDR_FIRST; u < hp->nhd; u++) { + int headerPos = u-HTTP_HDR_FIRST; Tcheck(hp->hd[u]); const char *header = hp->hd[u].b; long int hlen = strlen(header); @@ -525,21 +527,27 @@ VCL_INT v_matchproto_(td_sec_sec_process_response) continue; } /* Copy headers */ - strncpy(headerName, header, pos); - headerName[pos] = '\0'; + headersNames[headerPos] = malloc(pos+1); + strncpy(headersNames[headerPos], header, pos); + headersNames[headerPos][pos] = '\0'; // Find spaces pos += 1 /* : */ + strspn(&header[pos + 1], " \r\n\t"); // LWS = [CRLF] 1*( SP | HT ) chr(9,10,13,32) - strncpy(headerValue, &header[pos], hlen - pos); - headerValue[hlen - pos] = '\0'; - msc_add_response_header(transInt->trans, headerName, headerValue); + headersValues[headerPos] = (char *)malloc(hlen - pos +1); + strncpy(headersValues[headerPos], &header[pos], hlen - pos); + headersValues[headerPos][hlen - pos] = '\0'; + msc_add_response_header(transInt->trans, headersNames[headerPos], headersValues[headerPos]); #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Additional response header provided %s: %s", - headerName, headerValue); + headersNames[headerPos], headersValues[headerPos]); #endif } - free(headerName); - free(headerValue); msc_process_response_headers(transInt->trans, ctx->req->resp->status, protocol); + for (u = 0; u Date: Fri, 18 Nov 2022 10:19:54 +0100 Subject: [PATCH 04/10] fix: use the response struct not the request --- src/vmod_sec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index add061a..67eef63 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -371,7 +371,7 @@ VCL_INT v_matchproto_(td_sec_sec_process_url) /* Handling headers */ unsigned u; - const struct http *hp = ctx->req->http; + const struct http *hp = ctx->http_resp; #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Found %d headers, Start at %d, need to ingest %d headers", hp->nhd, HTTP_HDR_FIRST, hp->nhd - HTTP_HDR_FIRST); #endif From 23f43684ed01a7961ea8b0e73e81cd6961156458 Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Fri, 18 Nov 2022 10:26:42 +0100 Subject: [PATCH 05/10] fix contexts -_-" --- src/vmod_sec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index 67eef63..ab7bcc8 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -371,7 +371,7 @@ VCL_INT v_matchproto_(td_sec_sec_process_url) /* Handling headers */ unsigned u; - const struct http *hp = ctx->http_resp; + const struct http *hp = ctx->http_req; #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Found %d headers, Start at %d, need to ingest %d headers", hp->nhd, HTTP_HDR_FIRST, hp->nhd - HTTP_HDR_FIRST); #endif @@ -503,7 +503,7 @@ VCL_INT v_matchproto_(td_sec_sec_process_response) /* Handling headers */ unsigned u; - const struct http *hp = ctx->req->resp; + const struct http *hp = ctx->http_resp; #ifdef VMOD_SEC_DEBUG VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Processing Response Headers"); VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Found %d headers, Start at %d, need to ingest %d headers", From c44112684f0bc12d207a013507809f6ab619871e Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Mon, 21 Nov 2022 14:30:15 +0100 Subject: [PATCH 06/10] process body if body == none --- src/vmod_sec.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index ab7bcc8..153b864 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -459,6 +459,11 @@ VCL_INT v_matchproto_(td_sec_sec_do_process_request_body) if (capture_body == 1) { const struct http *hp = ctx->req->http; + if (ctx->req->req_body_status == BS_NONE) + { + msc_process_request_body(transInt->trans); + return process_intervention(transInt); + } if (ctx->req->req_body_status != BS_CACHED) { VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Unbuffered req.body"); @@ -764,3 +769,8 @@ VCL_STRING v_matchproto_(td_sec_sec_version) vmod_sec_version(VRT_CTX, struct VPFX(sec_sec) *vp){ return MODSECURITY_VERSION; } + + +// Handle Varnish requests on stream + +// VRT_AddFilter(ctx, &td_sec_vfp_modsec, &td_sec_vdp_modsec); \ No newline at end of file From a4a0f29c90eee6a6d26d225f87aae162cd8dcb07 Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Mon, 21 Nov 2022 14:32:40 +0100 Subject: [PATCH 07/10] force processing of body in all cases --- src/vmod_sec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index 153b864..cffeb58 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -467,7 +467,8 @@ VCL_INT v_matchproto_(td_sec_sec_do_process_request_body) if (ctx->req->req_body_status != BS_CACHED) { VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Unbuffered req.body"); - return -1; + msc_process_request_body(transInt->trans); + return process_intervention(transInt); } int ret; @@ -480,8 +481,8 @@ VCL_INT v_matchproto_(td_sec_sec_do_process_request_body) { VSL(SLT_Error, ctx->sp->vxid, "[vmodsec] - Iteration on req.body didn't succeed. %d", ret); - - return -1; + msc_process_request_body(transInt->trans); + return process_intervention(transInt); } VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Processing Request Body"); From fb81d821ce89d37a0e141966718435e3cabeec00 Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Mon, 21 Nov 2022 14:52:20 +0100 Subject: [PATCH 08/10] correct handling for response body processing --- src/vmod_sec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index cffeb58..935455f 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -604,7 +604,8 @@ VCL_INT v_matchproto_(td_sec_sec_do_process_response_body) VSL(SLT_Error, ctx->sp->vxid, "[vmodsec] - Iteration on resp.body didn't succeed. %d", ret); - return -1; + msc_process_response_body(transInt->trans); + return process_intervention(transInt); } VSL(SLT_Debug, ctx->sp->vxid, "[vmodsec] - Processing Response Body"); From 8a9358452d08deadb95c42433d50186a3b4f8486 Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Mon, 21 Nov 2022 16:14:59 +0100 Subject: [PATCH 09/10] debug: remove the hardcoded debug mode --- src/vmod_sec.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vmod_sec.c b/src/vmod_sec.c index 935455f..2f8b5ec 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -9,8 +9,6 @@ typedef struct Rules_t RulesSet; #endif #include -#define VMOD_SEC_DEBUG 1 - #include #include #include From 74382f7b28e35f616ee14f5665b1d1aeaca1c9da Mon Sep 17 00:00:00 2001 From: Xavier De Cock Date: Tue, 22 Nov 2022 09:47:19 +0100 Subject: [PATCH 10/10] Prepare for 2.0.0 --- CHANGELOG.MD | 3 +++ TODO.txt | 2 ++ src/vmod_sec.c | 6 ------ 3 files changed, 5 insertions(+), 6 deletions(-) create mode 100644 CHANGELOG.MD create mode 100644 TODO.txt diff --git a/CHANGELOG.MD b/CHANGELOG.MD new file mode 100644 index 0000000..1711435 --- /dev/null +++ b/CHANGELOG.MD @@ -0,0 +1,3 @@ +2.0.0 - Make the binding functional - before updating, make sure you test your rules +1.0.1 - Update to compile with different modsecurity version, and different varnish versions +1.0.0 - Initial Proof of concept binding, some important problems occurs on this version, mostly not able to effectively act. diff --git a/TODO.txt b/TODO.txt new file mode 100644 index 0000000..31f155c --- /dev/null +++ b/TODO.txt @@ -0,0 +1,2 @@ +Handle Varnish requests on stream (request / deliver) +VRT_AddFilter(ctx, &td_sec_vfp_modsec, &td_sec_vdp_modsec); \ No newline at end of file diff --git a/src/vmod_sec.c b/src/vmod_sec.c index 2f8b5ec..ae16991 100644 --- a/src/vmod_sec.c +++ b/src/vmod_sec.c @@ -135,7 +135,6 @@ VCL_VOID v_matchproto_(td_sec_sec__init) int error; (void)vcl_name; - if (ctx->method != VCL_MET_INIT) { VRT_fail(ctx, "[vmodsec] - init can only be called from vcl_init{}"); } @@ -769,8 +768,3 @@ VCL_STRING v_matchproto_(td_sec_sec_version) vmod_sec_version(VRT_CTX, struct VPFX(sec_sec) *vp){ return MODSECURITY_VERSION; } - - -// Handle Varnish requests on stream - -// VRT_AddFilter(ctx, &td_sec_vfp_modsec, &td_sec_vdp_modsec); \ No newline at end of file