Feature #488
closedIPS mode delayed rule initialization
Added by Victor Julien over 12 years ago. Updated about 12 years ago.
Description
Now that live ruleswaps are supported, in IPS mode we can delay the rules init. Reason is to reduce the time that traffic is not flowing. As the traffic won't be inspected this behaviour needs to be optional.
Logic would be:
1. start IPS mode with empty ruleset
2. after init trigger a rule reload immediately
Complication: starting with an empty ruleset will make sure htp callbacks are not registered, which a ruleswap currently can't change.
Please create a high level design for discussion first.
Files
0001-Delay-Detect-threads-initialization.patch (10.6 KB) 0001-Delay-Detect-threads-initialization.patch | Eric Leblond, 08/11/2012 06:35 AM | ||
0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch (4.24 KB) 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch | Eric Leblond, 08/15/2012 07:07 AM | ||
0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch (4.77 KB) 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch | Eric Leblond, 08/15/2012 10:52 AM | ||
0002-Convert-to-atomic-and-disable-check-on-HTP-config-ch.patch (5.61 KB) 0002-Convert-to-atomic-and-disable-check-on-HTP-config-ch.patch | Eric Leblond, 08/15/2012 10:52 AM |
Updated by Eric Leblond over 12 years ago
What about starting the suricata without the detect thread (or with a dummy detect function)?
This way, we will be able to activate the detection by atomically setting the detect function to the correct value. An other advantage is that all output module (pcap logging for instance) will work too.
Updated by Victor Julien over 12 years ago
Don't think what you're saying is any different from my suggestion.
The problem with this approach is that the detection engine controls some htp callback registration. We can't do those at runtime right now, unless we find a way to do so in a thread safe way. Any delayed detection engine initialisation will require us to do the callback registration at packet runtime.
Updated by Eric Leblond over 12 years ago
My first comment was not very clear. Just to be sure we talk about the same thing, there's the pseudo code version.
main() {
...
RunningModeDetectFunc = DummyFunc;
StartRunningMode();
StartDetectSystem();
RunningModeDetectFunc = RealDetectFunc;
...
}
If I get your point Detect is setting up some callback in the application layer and this causes problem if application layer is currently running.
In this case we could also use a contionnal test/dummy function pointer for application layer parsing when Detect is not yet initialised:
main() {
...
UseAppLayer = False;
RunningModeDetectFunc = DummyFunc;
StartRunningMode();
StartDetectSystem();
RunningModeDetectFunc = RealDetectFunc;
UseAppLayer = True;
...
}
One advantage of this technique is that may be possible to do a rule swap without using double memory.
Updated by Victor Julien over 12 years ago
Unless the runmode we use when starting up doesn't involve the stream+applayer, this won't solve the callback issue.
Also, the double memory issue isn't an issue here I think. The initial startup would be without rules, so no double memory there.
Maybe the easiest solution would be to just have a little runmode that simply accepts all packets when the detect engine is initializing. However if we use the full runmode we will have things like logging of http requests and such.
Updated by Eric Leblond about 12 years ago
I'm still thinking this will be over complicated for the benefits.
We've got two targets here:- Linux
- FreeBSD
This feature is not necessary for Netfilter, as we can use the queue-bypass option of NFQUEUE.
For FreeBSD and for old Linux, people can script it by adding the corresponding rules after suricata start.
Updated by Victor Julien about 12 years ago
Would the queue bypass scenario require us to:
- open the queue first, set bypass option, don't process packets -- packets will pass through bypass
- init suricata detect engine
- when init done, unset bypass opt on queue
- process packets
Updated by Eric Leblond about 12 years ago
No, queue-bypass is an NFQUEUE option: if there is nobody listening to the queue, the packet are simply accepted.
It can use with a iptables rule like the following:
iptables -I FORWARD -j NFQUEUE --queue-num 1 --queue-bypass
Updated by Victor Julien about 12 years ago
Hmm ok. This solution will be quite hard to deploy then as it depends on a lot of external settings/configuration. Thinking a solution internal to Suricata would be preferable.
Updated by Eric Leblond about 12 years ago
OK, trying to find/code something and submitting the POC as soon as I've got it.
Updated by Eric Leblond about 12 years ago
- File 0001-Delay-Detect-threads-initialization.patch 0001-Delay-Detect-threads-initialization.patch added
The attached patch is an implementation proposal. Basically a dummy function is used during the signature init. When this is done, the detect thread are initialised and the dummy function is switched to the real one.
Updated by Victor Julien about 12 years ago
So the detect threads now use "TmSlotSetFuncAppendDelayed" that keeps a list of thread modules that need some work after detect init is done. Then when detect init is complete, TmThreadActivateDummySlot runs the thread init functions and sets the s->slot_data atomically.
So while detect init is running, the detect threads run w/o any thread local data (==NULL). Correct?
I wonder if the thread init functions are safe for this use at packet runtime (same question applies to thread restarts I think)?
Updated by Eric Leblond about 12 years ago
Victor Julien wrote:
So the detect threads now use "TmSlotSetFuncAppendDelayed" that keeps a list of thread modules that need some work after detect init is done. Then when detect init is complete, TmThreadActivateDummySlot runs the thread init functions and sets the s->slot_data atomically.
Exactly and it also set the function to a the real one when thread init is done.
So while detect init is running, the detect threads run w/o any thread local data (==NULL). Correct?
Yes, but it is a dummy function whic is used (we do not use a conditional to exit of the standard function).
I wonder if the thread init functions are safe for this use at packet runtime (same question applies to thread restarts I think)?
Yes, it is the same issue with restart. If restart occurs before the init, we reinit the thread with a NULL thread init function (should be no issue here). If it is done after, then the thread structure it completely normal.
For now, I've just looked at the Detect module. The function is building a det_ctx based on the current de_ctx. I don't think there could be some problems here.
Updated by Anoop Saldanha about 12 years ago
looks good to me.
We need to set a dummy slot for stream as well. We will have to update the live swap function to register the htp callbacks later on, which is why we don't want the stream layer running in delayed mode.
Updated by Eric Leblond about 12 years ago
- File 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch added
The attached patch is a proposal. It solves the issue at the cost of a function call and a test.
Updated by Victor Julien about 12 years ago
Eric Leblond wrote:
The attached patch is a proposal. It solves the issue at the cost of a function call and a test.
Good idea. I think we need to use atomic flags here. We can replace the various need_* globals with a single atomic flags var that is checked and set instead.
Updated by Eric Leblond about 12 years ago
- File 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch 0001-Get-rid-of-AppLayerHtpRegisterExtraCallbacks.patch added
- File 0002-Convert-to-atomic-and-disable-check-on-HTP-config-ch.patch 0002-Convert-to-atomic-and-disable-check-on-HTP-config-ch.patch added
I update this patchset. First patch has been fixed because I've forgot to add the callbacks to the root element. Second patch uses an atomic and should fix #522.
Updated by Eric Leblond about 12 years ago
Pull request in github: https://github.com/inliniac/suricata/pull/4
Updated by Victor Julien about 12 years ago
- Status changed from Assigned to Closed
Merged, thanks Eric!