Project

General

Profile

Feature #488

IPS mode delayed rule initialization

Added by Victor Julien about 7 years ago. Updated almost 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Effort:
Difficulty:
Label:

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


Subtasks

Feature #522: live rule reloads: support enabling/disabling libhtp callbacksClosedEric LeblondActions

History

#1

Updated by Eric Leblond about 7 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.

#2

Updated by Victor Julien about 7 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.

#3

Updated by Eric Leblond about 7 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.

#4

Updated by Victor Julien about 7 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.

#5

Updated by Eric Leblond almost 7 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.

#6

Updated by Victor Julien almost 7 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

#7

Updated by Eric Leblond almost 7 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

#8

Updated by Victor Julien almost 7 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.

#9

Updated by Eric Leblond almost 7 years ago

OK, trying to find/code something and submitting the POC as soon as I've got it.

#10

Updated by Eric Leblond almost 7 years ago

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.

#11

Updated by Eric Leblond almost 7 years ago

  • % Done changed from 0 to 80
#12

Updated by Victor Julien almost 7 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)?

#13

Updated by Eric Leblond almost 7 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.

#14

Updated by Anoop Saldanha almost 7 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.

#15

Updated by Eric Leblond almost 7 years ago

The attached patch is a proposal. It solves the issue at the cost of a function call and a test.

#16

Updated by Victor Julien almost 7 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.

#17

Updated by Eric Leblond almost 7 years ago

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.

#19

Updated by Victor Julien almost 7 years ago

  • Status changed from Assigned to Closed

Merged, thanks Eric!

Also available in: Atom PDF