Memory leak if builded modsecurity with --enable-pcre-study

[weliu]

In apache2/msc_pcre.c line 68-74, we will get regex->pe from pcre_study() if we compile with --enable-pcre-study.

#ifdef WITH_PCRE_STUDY
#ifdef WITH_PCRE_JIT
pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr);
#else
pe = pcre_study(regex->re, 0, &errptr);
#endif
#endif

However we will use pcre_free() or free() to free it in msc_pcre_cleanup(), the right choice here is pcre_free_study().
if (regex->pe != NULL) {

if defined(VERSION_NGINX)

        pcre_free(regex->pe);

else

        free(regex->pe);

endif

        regex->pe = NULL;
    }

This will lead to memory leak, the memory that leaked was allocated use SLJIT_MALLOC in pcre_study().

This can be a big problem if we use Apache graceful restart.

[kipras]

Yes, this is a leak. In my case (ModSecurity under nginx) memory was ok (no leak) under Ubuntu 14.04, but when using under Ubuntu 16.04 - nginx would leak memory with every reload.

Applying this change fixed it. Thank you @weliu.

[weliu]

Glad it helped. @kipras

[marcstern]

Adding the following code, right before the "if (regex->pe != NULL)" block, should do the work:
#ifdef WITH_PCRE_STUDY
#ifdef WITH_PCRE_JIT
if (regex->pe != NULL) {
pcre_free_study(regex->pe);
regex->pe = NULL;
}
#endif
#endif

Correct?

[weliu]

I think the '#ifdef WITH_PCRE_JIT' should be removed. Only insert below code before "if (regex->pe != NULL)" block:

#ifdef WITH_PCRE_STUDY
if (regex->pe != NULL) {
pcre_free_study(regex->pe);
regex->pe = NULL;
}
#endif

Moreover, I think we should check the return value of pcre_study() in msc_pregcomp_ex():

#ifdef WITH_PCRE_STUDY
    #ifdef WITH_PCRE_JIT
            pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr);
    #else
            pe = pcre_study(regex->re, 0, &errptr);
    #endif
            /* check pe here */
#endif

[marcstern]

In case WITH_PCRE_JIT is not defined, pe is allocated with pcre_malloc() or malloc().
Freeing it with pcre_free_study() doesn't look safe...

Additional question: why using malloc() instead of pcre_malloc() under Apache?
Or should we use the memory pool instead?

[kipras]

Here's how the fixed block looks like in my case:

        if (regex->pe != NULL) {
#if defined(VERSION_NGINX)
#ifdef WITH_PCRE_STUDY
            pcre_free_study(regex->pe);
#else
            pcre_free(regex->pe);
#endif
#else
            free(regex->pe);
#endif
            regex->pe = NULL;
        }

The previous unpatched version that leaked memory was:

        if (regex->pe != NULL) {
#if defined(VERSION_NGINX)
            pcre_free(regex->pe);
#else
            free(regex->pe);
#endif
            regex->pe = NULL;
        }

Although, now that i look at it, it should probably be

        if (regex->pe != NULL) {
#ifdef WITH_PCRE_STUDY
            pcre_free_study(regex->pe);
#else
#if defined(VERSION_NGINX)
            pcre_free(regex->pe);
#else
            free(regex->pe);
#endif
#endif
            regex->pe = NULL;
        }

Or is pcre_free_study() only necessary in nginx and free() is fine under apache?

Checking pe after pcre_study() call is also a good idea. If it returns NULL we would call the wrong deallocator here.

[marcstern]

Checking pe after pcre_study() call is definitely a good idea

[weliu]

kipras's modification looks more reasonable to me.

[victorhora]

One thing worth mentioning is that the --enable-pcre-study configuration flag seems to be enabled by default on v2 since ModSec 2.5. The changelog entry from 2.5.12 also suggests it's the case.

My tests with Valgring suggests that the proposed patches actually makes the memory leak more noticeable.

My test environment was the following:

Linux debian 3.16.0-4-amd64 #1 SMP Debian 3.16.43-2+deb8u2 (2017-06-26) x86_64 GNU/Linux

Server version: Apache/2.4.10 (Debian)
Server built:   Mar 31 2018 09:39:03
Server's Module Magic Number: 20120211:37
Server loaded:  APR 1.5.1, APR-UTIL 1.5.4
Compiled using: APR 1.5.1, APR-UTIL 1.5.4
Architecture:   64-bit
Server MPM:     event
  threaded:     yes (fixed thread count)
    forked:     yes (variable process count)
Server compiled with....
 -D APR_HAS_SENDFILE
 -D APR_HAS_MMAP
 -D APR_HAVE_IPV6 (IPv4-mapped addresses enabled)
 -D APR_USE_SYSVSEM_SERIALIZE
 -D APR_USE_PTHREAD_SERIALIZE
 -D SINGLE_LISTEN_UNSERIALIZED_ACCEPT
 -D APR_HAS_OTHER_CHILD
 -D AP_HAVE_RELIABLE_PIPED_LOGS
 -D DYNAMIC_MODULE_LIMIT=256

ModSec flags:
./configure --with-yajl=no --with-ssdeep=no --with-lua=no --with-curl=no --enable-pcre-study --enable-pcre-jit

Apache/ModSec Configuration:

LoadModule security2_module /root/ModSecurity-v2/apache2/.libs/mod_security2.so
SecRuleEngine On
SecRequestBodyAccess On
SecStreamInBodyInspection On
SecRequestBodyLimit 13107200
SecRequestBodyNoFilesLimit 131072
SecRequestBodyInMemoryLimit 131072
SecRequestBodyLimitAction Reject
SecPcreMatchLimit 100000
SecPcreMatchLimitRecursion 100000
SecResponseBodyAccess On
SecResponseBodyMimeType text/plain text/html text/xml
SecResponseBodyLimit 524288
SecResponseBodyLimitAction ProcessPartial
SecTmpDir /tmp/
SecDataDir /tmp/
SecDebugLog /var/log/apache2/modsec_debug.log
SecDebugLogLevel 9
SecAuditEngine On
SecAuditLogParts ABCDEFHIJZ
SecAuditLogType Serial
SecAuditLog /var/log/apache2/modsec_audit.log
SecArgumentSeparator &
SecCookieFormat 0
SecStatusEngine On

IncludeOptional /root/owasp-modsecurity-crs-3.0/crs-setup.conf
IncludeOptional /root/owasp-modsecurity-crs-3.0/rules/*.conf

Request generated with Apache Benchmark:

ab -n 1000 -c 100 http://localhost/

Memory check:

valgrind -v --log-file=valgrind1-apache-modsec-attached-CRS-9-JIT-study-patch0-ab-1000.log --trace-children=yes --leak-check=full apache2 -k start

This is a snippet of the leak summaries I got when running Apache Benchmark + CRS with the patch applied:

==85695== LEAK SUMMARY:
==85695==    definitely lost: 461,298 bytes in 1,548 blocks
==85695==    indirectly lost: 30,992 bytes in 114 blocks
==85695==      possibly lost: 0 bytes in 0 blocks
==85695==    still reachable: 4,325 bytes in 11 blocks
==85695==         suppressed: 0 bytes in 0 blocks

And the same run without the patch:

==92740== LEAK SUMMARY:
==92740==    definitely lost: 74,304 bytes in 774 blocks
==92740==    indirectly lost: 30,992 bytes in 114 blocks
==92740==      possibly lost: 0 bytes in 0 blocks
==92740==    still reachable: 4,325 bytes in 11 blocks
==92740==         suppressed: 0 bytes in 0 blocks

Am I doing something wrong?