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

Fix stack overflow when parsing variables #1831

Closed
wants to merge 1 commit into from
Closed

Fix stack overflow when parsing variables #1831

wants to merge 1 commit into from

Conversation

gozzy
Copy link
Contributor

@gozzy gozzy commented Feb 3, 2016

For instance, a crash occured when HOME_NET is specified as $HOME_NET

For instance, a crash occured when HOME_NET is specified as $HOME_NET
@inliniac
Copy link
Contributor

inliniac commented Feb 3, 2016

Can you explain the problem in a bit more detail? Create some example rules that crash for you?

@gozzy
Copy link
Contributor Author

gozzy commented Feb 3, 2016

Actually it occurs in case of misconfiguration. For instance, if in yaml-file we specify the following:

vars:
HOME_NET: "!$HOME_NET"

It's definitely wrong configuration, but suricata recursively tries to resolve a variable in DetectAddressParse2() until stack is exhausted and segmentation fault occurs. Valgrind shows this:

==11392== Stack overflow in thread #1: can't grow stack to 0xffe801000
==11392==
==11392== Process terminating with default action of signal 11 (SIGSEGV)

I'm not sure if it's the best way to handle this misconfiguration... Furthermore, we can do something like this:

vars:
HOME_NET: "$EXTERNAL_NET"
EXTERNAL_NET: "$HOME_NET"

This is more complicated case, and it also leads to segmentation fault.

I've tried to handle this just to inform a user that something goes wrong. Because getting SIGSEGV without any details is not that good.

@inliniac
Copy link
Contributor

inliniac commented Feb 3, 2016

I see, thanks. Could you open a redmine ticket for this bug as well? Thanks!

@gozzy
Copy link
Contributor Author

gozzy commented Feb 3, 2016

Done: https://redmine.openinfosecfoundation.org/issues/1689

I'll try to look at the second case (when A is defined as B and vice versa).

@gozzy
Copy link
Contributor Author

gozzy commented Feb 3, 2016

@inliniac probably we should just limit the recursion depth without tricky strings comparison.

@gozzy
Copy link
Contributor Author

gozzy commented Feb 3, 2016

Got an idea about possible 'loop detection', gonna check it.

@gozzy gozzy closed this Feb 3, 2016
@gozzy gozzy deleted the recursive_var_fix2 branch February 3, 2016 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants