SecRequestBodyLimit does not get properly merged from upper contexts

[defanator]

Reproducible with the following test:

#!/usr/bin/perl

# (C) Andrei Belov

# Tests for ModSecurity-nginx connector (request body operations).

###############################################################################

use warnings;
use strict;

use Test::More;
use Socket qw/ CRLF /;

BEGIN { use FindBin; chdir($FindBin::Bin); }

use lib 'lib';
use Test::Nginx;

###############################################################################

select STDERR; $| = 1;
select STDOUT; $| = 1;

my $t = Test::Nginx->new()->has(qw/http auth_request/);

$t->write_file_expand('nginx.conf', <<'EOF');

%%TEST_GLOBALS%%

daemon off;

events {
}

http {
    %%TEST_GLOBALS_HTTP%%

    server {
        listen       127.0.0.1:8080;
        server_name  localhost;

        modsecurity on;
        modsecurity_rules '
            SecRuleEngine On
            SecRequestBodyAccess On
            SecRequestBodyLimit 128
            SecRequestBodyLimitAction Reject
            SecRule REQUEST_BODY "@rx BAD BODY" "id:31,phase:request,deny,log,status:403"
        ';

        client_header_buffer_size 1024;

        location /bodylimitreject {
#        modsecurity_rules '
#            SecRuleEngine On
#            SecRequestBodyAccess On
#            SecRequestBodyLimit 128
#            SecRequestBodyLimitAction Reject
#            SecRule REQUEST_BODY "@rx BAD BODY" "id:31,phase:request,deny,log,status:403"
#        ';
            proxy_pass http://127.0.0.1:8081;
        }
    }
}
EOF

$t->run_daemon(\&http_daemon);
$t->run()->waitforsocket('127.0.0.1:' . port(8081));

$t->plan(8);

###############################################################################

foreach my $method (('GET', 'POST', 'PUT', 'DELETE')) {
like(http_req_body($method, '/bodylimitreject', 'BODY' x 32), qr/TEST-OK-IF-YOU-SEE-THIS/, "$method request body limit reject, pass");
like(http_req_body($method, '/bodylimitreject', 'BODY' x 33), qr/403 Forbidden/, "$method request body limit reject, block");
}

###############################################################################

sub http_daemon {
	my $server = IO::Socket::INET->new(
		Proto => 'tcp',
		LocalHost => '127.0.0.1:' . port(8081),
		Listen => 5,
		Reuse => 1
	)
		or die "Can't create listening socket: $!\n";

	local $SIG{PIPE} = 'IGNORE';

	while (my $client = $server->accept()) {
		$client->autoflush(1);

		my $headers = '';
		my $uri = '';

		while (<$client>) {
			$headers .= $_;
			last if (/^\x0d?\x0a?$/);
		}

		$uri = $1 if $headers =~ /^\S+\s+([^ ]+)\s+HTTP/i;

		print $client <<'EOF';
HTTP/1.1 200 OK
Connection: close

EOF
		print $client "TEST-OK-IF-YOU-SEE-THIS"
			unless $headers =~ /^HEAD/i;

		close $client;
	}
}

sub http_req_body {
	my $method = shift;
	my $uri = shift;
	my $last = pop;
	return http( join '', (map {
		my $body = $_;
		"$method $uri HTTP/1.1" . CRLF
		. "Host: localhost" . CRLF
		. "Content-Length: " . (length $body) . CRLF . CRLF
		. $body
	} @_),
		"$method $uri HTTP/1.1" . CRLF
		. "Host: localhost" . CRLF
		. "Connection: close" . CRLF
		. "Content-Length: " . (length $last) . CRLF . CRLF
		. $last
	);
}

sub http_req_body_postargs {
	my $method = shift;
	my $uri = shift;
	my $last = pop;
	return http( join '', (map {
		my $body = $_;
		"$method $uri HTTP/1.1" . CRLF
		. "Host: localhost" . CRLF
		. "Content-Type: application/x-www-form-urlencoded" . CRLF
		. "Content-Length: " . (length "test=" . $body) . CRLF . CRLF
		. "test=" . $body
	} @_),
		"$method $uri HTTP/1.1" . CRLF
		. "Host: localhost" . CRLF
		. "Connection: close" . CRLF
		. "Content-Type: application/x-www-form-urlencoded" . CRLF
		. "Content-Length: " . (length "test=" . $last) . CRLF . CRLF
		. "test=" . $last
	);
}

###############################################################################

Moving rules under the location context makes it work.

Currently investigating this one by digging in the mergeProperties() thing here:
https://github.com/SpiderLabs/ModSecurity/blob/v3/master/headers/modsecurity/rules_properties.h#L311

[defanator]

The proposed PR does not fix configurations like the following though:

http {
    %%TEST_GLOBALS_HTTP%%

    modsecurity on;
    modsecurity_rules '
        SecRuleEngine On
        SecRequestBodyAccess On
        SecRequestBodyLimit 128
        SecRequestBodyLimitAction Reject
        SecRule REQUEST_BODY "@rx BAD BODY" "id:31,phase:request,deny,log,status:403"
    ';

    server {
        listen       127.0.0.1:8080;
        server_name  localhost;

        client_header_buffer_size 1024;

        location /bodylimitreject {
            modsecurity_rules '
                SecRequestBodyLimit 512 
            ';
            proxy_pass http://127.0.0.1:8081;
        }
    }
}

I.e. in the above configuration location block will still use SecRequestBodyLimit from http level (128) instead of locally redefined (512).

cc/ @victorhora @zimmerle @lonerr

[defanator]

Using something like this in mergeProperties() allows to redefine values in configurations like
#142 (comment):

-        if (from->m_requestBodyLimit.m_set == true) {
+        if (from->m_requestBodyLimit.m_set == true && to->m_requestBodyLimit.m_set == false) {
             to->m_requestBodyLimit.m_value = from->m_requestBodyLimit.m_value;
+            to->m_requestBodyLimit.m_set = true;
         }

I'm happy to extend the PR (+ tests) if this is considered as desired behavior.

[defanator]

OOP-friendly approach (incomplete, just an example:

test@vagrant:~/ModSecurity$ git diff | tee
diff --git a/headers/modsecurity/rules_properties.h b/headers/modsecurity/rules_properties.h
index 171d87e..4964dcc 100644
--- a/headers/modsecurity/rules_properties.h
+++ b/headers/modsecurity/rules_properties.h
@@ -54,6 +54,15 @@ class ConfigInt {
     ConfigInt() : m_set(false), m_value(0) { }
     bool m_set;
     int m_value;
+
+    void merge(ConfigInt *from) {
+        if (m_set == true || from->m_set == false) {
+            return;
+        }
+        m_set = true;
+        m_value = from->m_value;
+        return;
+    }
 };
 
 
@@ -62,6 +71,15 @@ class ConfigDouble {
     ConfigDouble() : m_set(false), m_value(0) { }
     bool m_set;
     double m_value;
+
+    void merge(ConfigDouble *from) {
+        if (m_set == true || from->m_set == false) {
+            return;
+        }
+        m_set = true;
+        m_value = from->m_value;
+        return;
+    }
 };
 
 
@@ -341,13 +359,8 @@ class RulesProperties {
             to->m_tmpSaveUploadedFiles = from->m_tmpSaveUploadedFiles;
         }
 
-        if (from->m_requestBodyLimit.m_set == true) {
-            to->m_requestBodyLimit.m_value = from->m_requestBodyLimit.m_value;
-        }
-
-        if (from->m_responseBodyLimit.m_set == true) {
-            to->m_responseBodyLimit.m_value = from->m_responseBodyLimit.m_value;
-        }
+        to->m_requestBodyLimit.merge(&from->m_requestBodyLimit);
+        to->m_responseBodyLimit.merge(&from->m_responseBodyLimit);
 
         if (from->m_requestBodyLimitAction != PropertyNotSetBodyLimitAction) {
             to->m_requestBodyLimitAction = from->m_requestBodyLimitAction;
@@ -357,13 +370,8 @@ class RulesProperties {
             to->m_responseBodyLimitAction = from->m_responseBodyLimitAction;
         }
 
-        if (from->m_uploadFileLimit.m_set == true) {
-            to->m_uploadFileLimit.m_value = from->m_uploadFileLimit.m_value;
-        }
-
-        if (from->m_uploadFileMode.m_set == true) {
-            to->m_uploadFileMode.m_value = from->m_uploadFileMode.m_value;
-        }
+        to->m_uploadFileLimit.merge(&from->m_uploadFileLimit);
+        to->m_uploadFileMode.merge(&from->m_uploadFileMode);
 
         if (from->m_uploadDirectory.m_set == true) {
             to->m_uploadDirectory.m_value = from->m_uploadDirectory.m_value;