netfilter: nf_nat_snmp_basic: use asn1 decoder library

PATCH In review Taehee Yoo 2 participants 3 messages Nov 7, 2017 – Nov 18, 2017

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Taehee Yoo

author

2017-11-07 14:58 UTC #0 lore raw

PATCH v2 1/1 netfilter: nf_nat_snmp_basic: use asn1 decoder library

The basic SNMP ALG parse snmp ASN.1 payload
however, since 2012 linux kernel provide ASN.1 decoder library. If we use ASN.1 decoder in the /lib/asn1_decoder.c, we can remove about 1000 line of ASN.1 parsing routine.

To use asn1_decoder.c, we should write mib file(nf_nat_snmp_basic.asn1) then /script/asn1_compiler.c makes *-asn1.c and *-asn1.h file at the compiletime.(nf_nat_snmp_basic-asn1.c, nf_nat_snmp_basic-asn1.h) The nf_nat_snmp_basic.asn1 is made by RFC1155, RFC1157, RFC1902, RFC1905, RFC2578, RFC3416. of course that mib file supports only the basic SNMP ALG.

Previous SNMP ALG mangles only first octet of IPv4 address. but after this patch, the SNMP ALG mangles whole IPv4 Address. And SNMPv3 is not supported.

I tested with snmp commands such ans snmpd, snmpwalk, snmptrap.

Signed-off-by Taehee Yoo <[email protected]>

---

v2:
- Add missing nf_nat_snmp_basic.asn1 file

v1:
- Initial version

 net/ipv4/netfilter/Kconfig                |    1 +
 net/ipv4/netfilter/Makefile               |    4 +-
 net/ipv4/netfilter/nf_nat_snmp_basic.asn1 |  198 +++++
 net/ipv4/netfilter/nf_nat_snmp_basic.c    | 1172 ++---------------------------
 4 files changed, 262 insertions(+), 1113 deletions(-)
 create mode 100644 net/ipv4/netfilter/nf_nat_snmp_basic.asn1

net/ipv4/netfilter/Kconfig +1 -0

index c11eb17..346c474 100644
@@ -148,6 +148,7 @@ config NF_NAT_SNMP_BASIC
 	depends on NF_CONNTRACK_SNMP
 	depends on NETFILTER_ADVANCED
 	default NF_NAT && NF_CONNTRACK_SNMP
+	select ASN1
 	---help---
 
 	  This module implements an Application Layer Gateway (ALG) for

net/ipv4/netfilter/Makefile +3 -1

index f462fee..16c509c 100644
@@ -26,8 +26,10 @@ obj-$(CONFIG_NF_REJECT_IPV4) += nf_reject_ipv4.o
 # NAT helpers (nf_conntrack)
 obj-$(CONFIG_NF_NAT_H323) += nf_nat_h323.o
 obj-$(CONFIG_NF_NAT_PPTP) += nf_nat_pptp.o
-obj-$(CONFIG_NF_NAT_SNMP_BASIC) += nf_nat_snmp_basic.o
+nf_nat_snmp_basic-asn1.o := nf_nat_snmp_basic-asn1.c nf_nat_snmp_basic-asn1.h
+obj-$(CONFIG_NF_NAT_SNMP_BASIC) += nf_nat_snmp_basic.o nf_nat_snmp_basic-asn1.o
 obj-$(CONFIG_NF_NAT_MASQUERADE_IPV4) += nf_nat_masquerade_ipv4.o
+clean-files := nf_nat_snmp_basic-asn1.c nf_nat_snmp_basic-asn1.h
 
 # NAT protocols (nf_nat)
 obj-$(CONFIG_NF_NAT_PROTO_GRE) += nf_nat_proto_gre.o

net/ipv4/netfilter/nf_nat_snmp_basic.asn1 new file +198 -0

198 lines not on this page · raw message

net/ipv4/netfilter/nf_nat_snmp_basic.c +60 ~~-1112~~

1,247 lines not on this page · raw message

Signature · 1 line

2.9.3

Pablo Neira Ayuso

maintainer

2017-11-13 13:50 UTC #1 lore raw

Hi Taehee,

Quoted 11 lines · Taehee Yoo

The basic SNMP ALG parse snmp ASN.1 payload
however, since 2012 linux kernel provide ASN.1 decoder library. If we use ASN.1 decoder in the /lib/asn1_decoder.c, we can remove about 1000 line of ASN.1 parsing routine.
To use asn1_decoder.c, we should write mib file(nf_nat_snmp_basic.asn1) then /script/asn1_compiler.c makes *-asn1.c and *-asn1.h file at the compiletime.(nf_nat_snmp_basic-asn1.c, nf_nat_snmp_basic-asn1.h) The nf_nat_snmp_basic.asn1 is made by RFC1155, RFC1157, RFC1902, RFC1905, RFC2578, RFC3416. of course that mib file supports only the basic SNMP ALG.

Quoted 3 lines

Previous SNMP ALG mangles only first octet of IPv4 address. but after this patch, the SNMP ALG mangles whole IPv4 Address. And SNMPv3 is not supported.

Was SNMPv3 supported before this patch?

Quoted 1 line

I tested with snmp commands such ans snmpd, snmpwalk, snmptrap.

If you can develop a bit more how you have tested this, I'd really appreciate.

This is removing quite a lot of code, which as I said is great since it's way less code to maintain.

My concern is that this patch is doing way many things in one go, and the lack of tests also make me feel concerned about regressions. If you could provide a more comprehensive description of what you have tested, I'd really appreciate.

Just to mention a few things that got introduced here in one go, that probably can be done incrementally:

net/ipv4/netfilter/nf_nat_snmp_basic.c static int help(struct sk_buff *skb, unsigned int protoff, · line 193

Quoted 12 lines

net/ipv4/netfilter/nf_nat_snmp_basic.c +4 -2

@@ -1232,14 +185,17 @@ static int help(struct sk_buff *skb, unsigned int protoff,
 	if (ntohs(udph->len) != skb->len - (iph->ihl << 2)) {
 		net_warn_ratelimited("SNMP: dropping malformed packet src=%pI4 dst=%pI4\n",
 				     &iph->saddr, &iph->daddr);
-		 return NF_DROP;
+		nf_ct_helper_log(skb, ct, "too short packet");
+		return NF_DROP;
 	}
 
-	if (!skb_make_writable(skb, skb->len))
+	if (!skb_make_writable(skb, skb->len)) {
+		nf_ct_helper_log(skb, ct, "cannot mangle packet");

This log line is new, probably good to add this in a separated initial patch.

net/ipv4/netfilter/nf_nat_snmp_basic.c static int help(struct sk_buff *skb, unsigned int protoff, · line 198

Quoted 6 lines

net/ipv4/netfilter/nf_nat_snmp_basic.c +2 -1

 		return NF_DROP;
+	}
 
 	spin_lock_bh(&snmp_lock);
-	ret = snmp_translate(ct, ctinfo, skb);
+	ret = snmp_translate(ct, dir, skb);

Conversion to use 'dir' could be done in a initial patch too I think.

net/ipv4/netfilter/nf_nat_snmp_basic.c static struct nf_conntrack_helper snmp_trap_helper __read_mostly = { · line 1267

Quoted 13 lines

net/ipv4/netfilter/nf_nat_snmp_basic.c +0 -6

 	spin_unlock_bh(&snmp_lock);
 	return ret;
 }

@@ -1259,12 +215,6 @@ static struct nf_conntrack_helper snmp_trap_helper __read_mostly = {
 	.tuple.dst.protonum	= IPPROTO_UDP,
 };
 
-/*****************************************************************************
- *
- * Module stuff.
- *
- *****************************************************************************/
-

This useless comment can also go away in an initial patch.

net/ipv4/netfilter/nf_nat_snmp_basic.c static void __exit nf_nat_snmp_basic_fini(void) · line 1286

Quoted 9 lines

net/ipv4/netfilter/nf_nat_snmp_basic.c +0 -2

 static int __init nf_nat_snmp_basic_init(void)
 {
 	BUG_ON(nf_nat_snmp_hook != NULL);

@@ -1282,5 +232,3 @@ static void __exit nf_nat_snmp_basic_fini(void)
 
 module_init(nf_nat_snmp_basic_init);
 module_exit(nf_nat_snmp_basic_fini);
-
-module_param(debug, int, 0600);

I'm fine with killing this old debug parameter, but better in an initial patch.

Thanks for your work!

Taehee Yoo

author

2017-11-18 07:59 UTC #2 lore raw

Hi Pablo

I apologize for late reply.

2017-11-13 22:50 GMT+09:00 Pablo Neira Ayuso <[email protected]>:

Quoted 14 lines

Hi Taehee,
Quoted 11 lines · Taehee Yoo
The basic SNMP ALG parse snmp ASN.1 payload
however, since 2012 linux kernel provide ASN.1 decoder library. If we use ASN.1 decoder in the /lib/asn1_decoder.c, we can remove about 1000 line of ASN.1 parsing routine.
To use asn1_decoder.c, we should write mib file(nf_nat_snmp_basic.asn1) then /script/asn1_compiler.c makes *-asn1.c and *-asn1.h file at the compiletime.(nf_nat_snmp_basic-asn1.c, nf_nat_snmp_basic-asn1.h) The nf_nat_snmp_basic.asn1 is made by RFC1155, RFC1157, RFC1902, RFC1905, RFC2578, RFC3416. of course that mib file supports only the basic SNMP ALG.

Quoted 6 lines

Quoted 3 lines
Previous SNMP ALG mangles only first octet of IPv4 address. but after this patch, the SNMP ALG mangles whole IPv4 Address. And SNMPv3 is not supported.
Was SNMPv3 supported before this patch?

Yes, original snmp helper do not support SNMPv3.

Quoted 5 lines

Quoted 1 line
I tested with snmp commands such ans snmpd, snmpwalk, snmptrap.
If you can develop a bit more how you have tested this, I'd really appreciate.

My test environment are below.

#Network
Client <-------------> Netfilter FW <-------------> SNMP Server
192.168.3.2 192.168.3.1 192.168.4.1 192.168.4.2

#FW commands
iptables -t raw -I PREROUTING -p udp -m multiport --dports 161,162
-j CT --helper snmp
echo 'file nf_nat_snmp_basic.c +p' > /sys/kernel/debug/dynamic_debug/control

#SNMP Server commands
sudo ip r a 192.168.3.2 via 192.168.4.1 dev enp2s0

To test basic snmp test, I used snmpwalk command because it is easy to use
snmpwalk -v <1 or 2c> -c public <ip address> OID
example)
snmpwalk -v 2c -c public 192.168.4.2 .1.3.6.1.2.1.4.21

so that we can see this message from dmesg.
"snmp_helper: 192.168.3.2 to 192.168.4.1"

And, to test snmp trap test, I used snmptrap command.
snmptrap -v 1 -c public 192.168.3.2 .1 192.168.4.2 0 0 0 .1 a 192.168.4.2 snmptrap -v 2c -c public 192.168.3.2 .1 .1 .1 a 192.168.4.2

SNMPv1 trap includes two ip address in payload. so we can see below message twice.
"snmp_helper: 192.168.4.2 to 192.168.3.1"

If you want to see asn1 decoder debug message, please use below command
echo 'file asn1_decoder.c +p' > /sys/kernel/debug/dynamic_debug/control

so that you can see below message

[ 4365.722398] next_op: pc=0/100 dp=0/46 C=0 J=0
[ 4365.722530] - match? 30 30 00
[ 4365.722578] - TAG: 30 44 CONS
[ 4365.722625] next_op: pc=2/100 dp=2/46 C=1 J=0
[ 4365.722665] - match? 02 02 00
[ 4365.722704] - TAG: 02 1
[ 4365.722739] - LEAF: 1
[ 4365.722808] next_op: pc=5/100 dp=5/46 C=1 J=0
[ 4365.722874] - match? 04 04 00
[ 4365.722915] - TAG: 04 6
[ 4365.722952] - LEAF: 6
[ 4365.723008] next_op: pc=7/100 dp=13/46 C=1 J=0
[ 4365.723075] - match? a4 a0 04
[ 4365.723137] next_op: pc=10/100 dp=13/46 C=1 J=0
[ 4365.723176] - match? a4 a1 05
[ 4365.723218] next_op: pc=13/100 dp=13/46 C=1 J=0
[ 4365.723256] - match? a4 a5 01
[ 4365.723297] next_op: pc=16/100 dp=13/46 C=1 J=0
[ 4365.723334] - match? a4 a2 06
[ 4365.723376] next_op: pc=19/100 dp=13/46 C=1 J=0
[ 4365.723413] - match? a4 a4 00
[ 4365.723452] - TAG: a4 31 CONS
[ 4365.723525] - MATCH_JUMP
[ 4365.723596] next_op: pc=48/100 dp=15/46 C=2 J=1
[ 4365.723653] - match? 06 06 00
[ 4365.723691] - TAG: 06 1
[ 4365.723726] - LEAF: 1
[ 4365.723767] next_op: pc=50/100 dp=18/46 C=2 J=1
[ 4365.723805] - match? 40 40 00
[ 4365.723842] - TAG: 40 4
[ 4365.723906] snmp_helper: 192.168.3.2 to 192.168.4.1
[ 4365.723944] - LEAF: 4
[ 4365.723988] next_op: pc=53/100 dp=24/46 C=2 J=1
[ 4365.724063] - match? 02 02 00
[ 4365.724127] - TAG: 02 1
[ 4365.724180] - LEAF: 1
[ 4365.724222] next_op: pc=55/100 dp=27/46 C=2 J=1
[ 4365.724259] - match? 02 02 00
[ 4365.724296] - TAG: 02 1
[ 4365.724330] - LEAF: 1
[ 4365.724372] next_op: pc=57/100 dp=30/46 C=2 J=1
[ 4365.724449] - match? 43 43 00
[ 4365.724505] - TAG: 43 1
[ 4365.724541] - LEAF: 1
[ 4365.724582] next_op: pc=59/100 dp=33/46 C=2 J=1
[ 4365.724621] - match? 30 30 00
[ 4365.724658] - TAG: 30 11 CONS
[ 4365.724703] - MATCH_JUMP
[ 4365.724774] next_op: pc=64/100 dp=35/46 C=3 J=2
[ 4365.724831] - match? 30 30 00
[ 4365.724870] - TAG: 30 9 CONS
[ 4365.724913] next_op: pc=66/100 dp=37/46 C=4 J=2
[ 4365.724951] - match? 06 06 00
[ 4365.724988] - TAG: 06 1
[ 4365.725023] - LEAF: 1
[ 4365.725065] next_op: pc=68/100 dp=40/46 C=4 J=2
[ 4365.725104] - match? 40 02 42
[ 4365.725146] next_op: pc=70/100 dp=40/46 C=4 J=2
[ 4365.725183] - match? 40 04 44
[ 4365.725224] next_op: pc=72/100 dp=40/46 C=4 J=2
[ 4365.725262] - match? 40 06 46
[ 4365.725347] next_op: pc=74/100 dp=40/46 C=4 J=2
[ 4365.725413] - match? 40 40 00
[ 4365.725478] - TAG: 40 4
[ 4365.725538] snmp_helper: 192.168.3.2 to 192.168.4.1
[ 4365.725575] - LEAF: 4
[ 4365.725617] next_op: pc=77/100 dp=46/46 C=4 J=2
[ 4365.725659] next_op: pc=79/100 dp=46/46 C=4 J=2
[ 4365.725702] next_op: pc=81/100 dp=46/46 C=4 J=2
[ 4365.725744] next_op: pc=83/100 dp=46/46 C=4 J=2
[ 4365.725804] next_op: pc=85/100 dp=46/46 C=4 J=2
[ 4365.725877] next_op: pc=87/100 dp=46/46 C=4 J=2
[ 4365.725920] next_op: pc=89/100 dp=46/46 C=4 J=2
[ 4365.725963] next_op: pc=91/100 dp=46/46 C=4 J=2
[ 4365.726005] next_op: pc=93/100 dp=46/46 C=4 J=2
[ 4365.726047] next_op: pc=95/100 dp=46/46 C=4 J=2
[ 4365.726128] next_op: pc=96/100 dp=46/46 C=4 J=2
[ 4365.726189] - end cons t=37 dp=46 l=46/46
[ 4365.726226] - cons len l=9 d=9
[ 4365.726267] next_op: pc=97/100 dp=46/46 C=3 J=2
[ 4365.726305] - end cons t=35 dp=46 l=46/46
[ 4365.726341] - cons len l=11 d=11
[ 4365.726382] next_op: pc=99/100 dp=46/46 C=2 J=2
[ 4365.726440] next_op: pc=62/100 dp=46/46 C=2 J=1
[ 4365.726510] - end cons t=15 dp=46 l=46/46
[ 4365.726567] - cons len l=31 d=31
[ 4365.726610] next_op: pc=63/100 dp=46/46 C=1 J=1
[ 4365.726652] next_op: pc=22/100 dp=46/46 C=1 J=0
[ 4365.726694] next_op: pc=25/100 dp=46/46 C=1 J=0
[ 4365.726737] next_op: pc=28/100 dp=46/46 C=1 J=0
[ 4365.726799] next_op: pc=31/100 dp=46/46 C=1 J=0
[ 4365.726872] next_op: pc=34/100 dp=46/46 C=1 J=0
[ 4365.726924] next_op: pc=35/100 dp=46/46 C=1 J=0
[ 4365.726964] - end cons t=2 dp=46 l=46/46
[ 4365.727002] - cons len l=44 d=44
[ 4365.727045] next_op: pc=36/100 dp=46/46 C=0 J=0

Quoted 64 lines

This is removing quite a lot of code, which as I said is great since it's way less code to maintain.
My concern is that this patch is doing way many things in one go, and the lack of tests also make me feel concerned about regressions. If you could provide a more comprehensive description of what you have tested, I'd really appreciate.
Just to mention a few things that got introduced here in one go, that probably can be done incrementally:
net/ipv4/netfilter/nf_nat_snmp_basic.c static int help(struct sk_buff *skb, unsigned int protoff, · line 193
Quoted 12 lines
net/ipv4/netfilter/nf_nat_snmp_basic.c +4 -2
@@ -1232,14 +185,17 @@ static int help(struct sk_buff *skb, unsigned int protoff,
      if (ntohs(udph->len) != skb->len - (iph->ihl << 2)) {
              net_warn_ratelimited("SNMP: dropping malformed packet src=%pI4 dst=%pI4\n",
                                   &iph->saddr, &iph->daddr);
-              return NF_DROP;
+             nf_ct_helper_log(skb, ct, "too short packet");
+             return NF_DROP;
      }
 
-     if (!skb_make_writable(skb, skb->len))
+     if (!skb_make_writable(skb, skb->len)) {
+             nf_ct_helper_log(skb, ct, "cannot mangle packet");
This log line is new, probably good to add this in a separated initial patch.
net/ipv4/netfilter/nf_nat_snmp_basic.c static int help(struct sk_buff *skb, unsigned int protoff, · line 198
Quoted 6 lines
net/ipv4/netfilter/nf_nat_snmp_basic.c +2 -1
              return NF_DROP;
+     }
 
      spin_lock_bh(&snmp_lock);
-     ret = snmp_translate(ct, ctinfo, skb);
+     ret = snmp_translate(ct, dir, skb);
Conversion to use 'dir' could be done in a initial patch too I think.
net/ipv4/netfilter/nf_nat_snmp_basic.c static struct nf_conntrack_helper snmp_trap_helper __read_mostly = { · line 1267
Quoted 13 lines
net/ipv4/netfilter/nf_nat_snmp_basic.c +0 -6
      spin_unlock_bh(&snmp_lock);
      return ret;
 }
@@ -1259,12 +215,6 @@ static struct nf_conntrack_helper snmp_trap_helper __read_mostly = {
      .tuple.dst.protonum     = IPPROTO_UDP,
 };
 
-/*****************************************************************************
- *
- * Module stuff.
- *
- *****************************************************************************/
-
This useless comment can also go away in an initial patch.
net/ipv4/netfilter/nf_nat_snmp_basic.c static void __exit nf_nat_snmp_basic_fini(void) · line 1286
Quoted 9 lines
net/ipv4/netfilter/nf_nat_snmp_basic.c +0 -2
 static int __init nf_nat_snmp_basic_init(void)
 {
      BUG_ON(nf_nat_snmp_hook != NULL);
@@ -1282,5 +232,3 @@ static void __exit nf_nat_snmp_basic_fini(void)
 
 module_init(nf_nat_snmp_basic_init);
 module_exit(nf_nat_snmp_basic_fini);
-
-module_param(debug, int, 0600);
I'm fine with killing this old debug parameter, but better in an initial patch.

Quoted 1 line

Thanks for your work!

I will resend v3 patch that are split several part.

Thank you so much for your review!