Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shared memory crash fixes #1306

Closed

Conversation

phantom-az
Copy link
Contributor

Fix cases when shared memory allocated by first instance of applicaion being
overwritten by another instance attaching same shared memory segments. It happens
in case when 'nginx' server is running, but configuration is updated and 'nginx -t'
is called in order to update configuration. Shared memory segment gets overriten by
second instance of nginx, and any call to LogFile/SharedFile::find_handler() will
cause crash.

With this fix, every initialized block is being marked with feature specific signature
assuming that block in use already, and can't be re-used yet. For second and later
application instances yet another ftok() code is adviced to use (via recursive call to
add_new_handler()).

…on being

overwritten by another instance attaching same shared memory segments.  It happens
in case when 'nginx' server is running, but configuration is updated and 'nginx -t'
is called in order to update configuration.  Shared memory segment gets overriten by
second instance of nginx, and any call to LogFile/SharedFile::find_handler() will
cause crash.

With this fix, every initialized block is being marked with feature specific signature
assuming that block in use already, and can't be re-used yet.  For second and later
application instances yet another ftok() code is adviced to use (via recursive call to
add_new_handler()).

There's a side effect possible when nginx (or other user of libmodsecurity) gets
crashed, shared memory block can be left in place with valid signature stored in there.
For this case it's advised to use ipcs(1)/ipcrm(1) to cleanup stale blocks.
@defanator
Copy link
Contributor

Spent some time in attempts to make it working, but everything I have so far is the following message:

# nginx
nginx: [emerg] "proxy_pass" directive Failed to allocate shared memory (1): Invalid argument in /etc/nginx/nginx.conf:80

Configuration is pretty straightforward (3 server blocks with 6 locations in total, modsecurity is enabled in 3 locations with generic OWASP CRS v3.0.0).

@defanator
Copy link
Contributor

strace(1) tail:

open("/var/log/modsec_audit.log", O_WRONLY|O_CREAT|O_APPEND, 0666) = 6
lseek(6, 0, SEEK_END)                   = 9612542
stat("/var/log/modsec_audit.log", {st_mode=S_IFREG|0644, st_size=9612542, ...}) = 0
stat("/var/log/modsec_audit.log", {st_mode=S_IFREG|0644, st_size=9612542, ...}) = 0
shmget(0x1001aa1, 96, IPC_CREAT|0666)   = -1 EINVAL (Invalid argument)
shmdt(0x56013b4b1ba0)                   = -1 EINVAL (Invalid argument)
close(6)                                = 0
gettid()                                = 5309
write(3, "2017/01/31 14:12:58 [emerg] 5309"..., 145) = 145
write(2, "nginx: [emerg] \"proxy_pass\" dire"..., 121nginx: [emerg] "proxy_pass" directive Failed to allocate shared memory (1): Invalid argument in /etc/nginx/nginx.conf:80) = 121

@zimmerle
Copy link
Contributor

Hi @phantom-az,

Thank you for the patch!

The main reason why we have this shared memory was to avoid the logs be over-written by different process. In the fork perspective it was convenient to also share the file descriptor and other data that may be useful while writing the logs. But, the real crucial information that most be shared among the different process (and threads) is the critical section locking. It means that in practice we can remove all the other data which is not about the file name and/or the locking structure.

I made this patch, that may solve the issue:
https://github.com/SpiderLabs/ModSecurity/blob/v3/dev/parser/src/utils/shared_files.cc

Tell me if you have any concern about it.

@zimmerle zimmerle closed this Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants