ldx/python-iptables

Problem with target "NOTRACK"

jllorente opened this issue · 9 comments

Hi,

I think I've found a bug regarding target "NOTRACK"
I'm attempting to produce a test rule to untrack SCTP protocol

# iptables -t raw -A PREROUTING -p 132 -j NOTRACK

The problem is that the target is not handled properly and yields the following:

# iptables -t raw -S PREROUTING
-P PREROUTING ACCEPT
-P OUTPUT ACCEPT
-A PREROUTING -p sctp -j CT

Steps to reproduce:

import iptc

rule = iptc.Rule()
rule.protocol = "132"
rule.target = iptc.Target(rule, "NOTRACK")
chain = iptc.Chain(iptc.Table(iptc.Table.RAW), "PREROUTING")
chain.insert_rule(rule)

Any help would be appreciated!
Thanks!

I have done bit more testing. Turns out that I can properly retrieve a NOTRACK rule via iptc, but I cannot insert it back.

Steps to reproduce:

### First insert the rule via iptables
# iptables -t raw -A PREROUTING -p 132 -j NOTRACK


### Then fire up python(3)
import iptc

chain = iptc.Chain(iptc.Table(iptc.Table.RAW), "PREROUTING")
rule = chain.rules[0]

rule.target.name
>> NOTRACK

rule.target.standard_target
>>> CT
 
rule.target.get_all_parameters()
>>> {}

rule.target.__dict__
>>>
{'_revision': 2,
 '_ptr': <iptc.xtables.LP_xt_entry_target at 0x7f4c562b1ae8>,
 '_orig_parse': <CFunctionType object at 0x7f4c57169818>,
 '_orig_options': <iptc.xtables.LP_xt_option_entry at 0x7f4c562b1268>,
 '_target_buf': <iptc.ip4tc.LP_c_ubyte at 0x7f4c562b1488>,
 '_name': 'CT',
 '_buffer': <iptc.ip4tc._Buffer at 0x7f4c5627acc0>,
 '_xt': XTables for protocol 2,
 '_ptrptr': <iptc.xtables.LP_LP_xt_entry_target at 0x7f4c562b1d90>,
 '_module': <iptc.xtables._xtables_target_v10 at 0x7f4c562b18c8>,
 '_alias_module': <iptc.xtables._xtables_target_v10 at 0x7f4c562b1f28>,
 '_rule': <iptc.ip4tc.Rule at 0x7f4c5627aa20>}

Hope this sheds some light :)

I think the cause to this issue is that NOTRACK is actually an 'alias' to CT --notrack.

But as the Target was created without any parameters, the conversion to CT does not have space to record the notrack parameter.

Try targetting CT --notrack instead, and check if the rule is again malformed.

Thanks for pointing that out @pepoluan, you are absolutely right, NOTRACK is indeed an alias for the CT target.

I would like to point out that to perform that "NOTRACK" action, one could insert the rule in 2 different ways, however only the former seems to be properly processed by iptc.

# iptables -t raw -A PREROUTING -p 132 -j CT --notrack
# iptables -t raw -A PREROUTING -p 132 -j NOTRACK

The supported way then to handle the NOTRACK is as follows:

import iptc

rule = iptc.Rule()
rule.protocol = "132"
rule.target = iptc.Target(rule, "CT")
rule.target.notrack = ""
chain = iptc.Chain(iptc.Table(iptc.Table.RAW), "PREROUTING")
chain.insert_rule(rule)

Shall we then close this issue or is there any way of adding support for the alias processing?

Cheers!

Just a thought: Should we augment the Target class' __init__ to raise an exception for this case?

(IIRC, NOTRACK is deprecated anyways, so we can extend this behavior to all deprecated targets.)

ldx commented

@jllorente there might be a way to handle this better, but I'm kinda pressed on time right now, so not sure when I'll be able to look into this.

@pepoluan I usually don't really like adding target/match specific logic to the classes, but if you can elaborate or create a PR to show what you mean I'll be happy to review it.

Busy doing other things, so rather than submitting a half-baked PR, I'll just illustrate it in a diff against latest master:


diff --git a/iptc/ip4tc.py b/iptc/ip4tc.py
index 3a5791a..3fe4e0d 100644
--- a/iptc/ip4tc.py
+++ b/iptc/ip4tc.py
@@ -15,6 +15,8 @@ from .xtables import (XT_INV_PROTO, NFPROTO_IPV4, XTablesError, xtables,

 __all__ = ["Table", "Chain", "Rule", "Match", "Target", "Policy", "IPTCError"]

+DEPRECATED_TARGETS = {"NOTRACK"}
+
 try:
     load_kernel("ip_tables")
 except:
@@ -687,6 +689,9 @@ class Target(IPTCModule):
         self._orig_parse = None
         self._orig_options = None

+        if name in DEPRECATED_TARGETS:
+            raise DeprecationWarning('Target "{0}" is deprecated!'.format(name))
+
         # NOTE:
         # get_ip() returns the 'ip' structure that contains (1)the 'flags' field, and
         # (2)the value for the GOTO flag.

So, we have a global constant DEPRECATED_TARGETS, and during Target instantiation, a simple check would raise DeprecationWarning exception if the Target name is in that set.

ldx commented

Oh I see. Unfortunately there's more here than meets the eye: whether a target/match should be marked as "deprecated" also depends on the version. I'm also not a fan of baking this into class.

OTOH, it might be a good idea to update docs about this.

Fair enough. I too am not really too keen on enforcement of DeprecatedWarning like this.

Another idea would be to perform an (optional) auto-retrieval post-commit, and warn the user if the retrieved Target changes (e.g., from "NOTRACK" to "CT"). Not sure how much work to implement this kind of self-verification, though.

I will close this issue. IMO the better way is using CT target anyway.
I hope this serves at least for future reference :)