netgraph/ng_nat: Prevent crash by malformated packets

[donner]

Summary

Module is easy to crash with an unexpected packet from outside.
This patch weakens the checks from an assert to a silent skip.

PR: 243096

Test Plan

Bind module to an external interface as described in the bug report.
Wait.

[markj]

sys/netgraph/ng_nat.c

816 ↗

(On Diff #66507)

We also need to verify that the packet length is >= ipofs + sizeof(struct ip).

[donner]

Checking available space before accessing it.

[markj]

So this provides some basic validation, but it is not clear what assumptions libalias makes on input packets. Do we also need to verify the IP header checksum? For fragments, do we need to duplicate some of the validation implemented by ip_reass()?

sys/netgraph/ng_nat.c

818 ↗

(On Diff #66537)

There should be a space following 'if'.

819 ↗

(On Diff #66537)

I think this validation only needs to be performed for ethernet packets, i.e., in the DLT_EN10MB case above. We should be able to trust raw IP packets, so it is a waste to re-validate in that case.

[donner]

Fixed spacing style.

[donner]

sys/netgraph/ng_nat.c

819 ↗

(On Diff #66537)

Raw (IP) packets may come from other parts of the netgraph infrastructure which is not Ethernet based, i.e. PPP, HDLC etc.

So I'd keep this.

[donner]

Add an explanation to the man page.

[donner]

share/man/man4/ng_nat.4

396 ↗

(On Diff #66679)

Wrong reference.

409 ↗

(On Diff #66679)

Missing reference.

[eugen_grosbein.net]

In D23091#507447, @lutz_donnerhacke.de wrote:

Add an explanation to the man page.

I don't think we should make a habit documenting kernel panics in NEGRAPH code. Such bugs must be fixed, not documented.

[donner]

In D23091#507455, @eugen_grosbein.net wrote:

In D23091#507447, @lutz_donnerhacke.de wrote:

Add an explanation to the man page.

I don't think we should make a habit documenting kernel panics in NEGRAPH code. Such bugs must be fixed, not documented.

This patch fixes the (obvious) panic and (as requested in the bug report) documents, that there might be an unexplored rabbit hole of further bugs.
Please feel free to find the next panic.

[eugen_grosbein.net]

In D23091#507459, @lutz_donnerhacke.de wrote:

This patch fixes the (obvious) panic and (as requested in the bug report) documents, that there might be an unexplored rabbit hole of further bugs.
Please feel free to find the next panic.

If there is no known case leading to a panic, we should not scare users with manual. It's still bad wording IMO. If there is known vector that may lead to panic, another checks should be added to make sure panic does not happen.

[donner]

Leave the man page untouched.

[donner]

In D23091#507505, @eugen_grosbein.net wrote:

If there is no known case leading to a panic, we should not scare users with manual. It's still bad wording IMO. If there is known vector that may lead to panic, another checks should be added to make sure panic does not happen.

So the patch is reverted now.

[donner]

In D23091#506416, @markj wrote:

So this provides some basic validation, but it is not clear what assumptions libalias makes on input packets. Do we also need to verify the IP header checksum? For fragments, do we need to duplicate some of the validation implemented by ip_reass()?

I just look this issue up in sys/netinet/libalias/alias.c
The functions LibAliasIn and LibAliasOut do expect IPv4 packets in flat memory and do all the validation stuff (sufficient length, fragmentation etc.) themselves.

So the precondition for calling libalias is that the IP packet is pulled completely and has version 4 set
Relevant pieces of code are "if ((m = m_megapullup(m, m->m_pkthdr.len)) == NULL) {" and "if (ip->ip_v != IPVERSION) ..." which are both present.

Any further tests are unnecessary.

[markj]

In D23091#511942, @lutz_donnerhacke.de wrote:

In D23091#506416, @markj wrote:

So this provides some basic validation, but it is not clear what assumptions libalias makes on input packets. Do we also need to verify the IP header checksum? For fragments, do we need to duplicate some of the validation implemented by ip_reass()?

I just look this issue up in sys/netinet/libalias/alias.c
The functions LibAliasIn and LibAliasOut do expect IPv4 packets in flat memory and do all the validation stuff (sufficient length, fragmentation etc.) themselves.

So the precondition for calling libalias is that the IP packet is pulled completely and has version 4 set
Relevant pieces of code are "if ((m = m_megapullup(m, m->m_pkthdr.len)) == NULL) {" and "if (ip->ip_v != IPVERSION) ..." which are both present.

Any further tests are unnecessary.

I note also that it uses a differential checksum rather than recomputing the entire IP header checksum, so it should not silently hide corrupted packets.

[rob_sarcasticadmin.com]

I applied this patch and recompiled. I can confirm that I'm no longer seeing a panic for the example configuration in the ng_nat manpage.

Im also happy to hear that libalias has the necessary validations.

[markj]

I have no objection to this. I think the checks are cheap enough that they won't hurt, and after spending some time reading libalias I believe they are sufficient.

[donner]

Thank you. Somebody need to land this.

[donner]

@eugen_grosbein.net are your concerns handled?

[eugen_grosbein.net]

In D23091#518544, @lutz_donnerhacke.de wrote:

@eugen_grosbein.net are your concerns handled?

Yes.