Page MenuHomeFreeBSD

netgraph/ng_nat: Prevent crash by malformated packets
ClosedPublic

Authored by lutz_donnerhacke.de on Jan 8 2020, 5:38 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 28638
Build 26668: arc lint + arc unit

Event Timeline

markj added a subscriber: markj.Jan 8 2020, 5:44 PM
markj added inline comments.
sys/netgraph/ng_nat.c
819

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

Checking available space before accessing it.

lutz_donnerhacke.de marked an inline comment as done.Jan 9 2020, 8:07 AM
markj added a comment.Jan 9 2020, 3:31 PM

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

There should be a space following 'if'.

819

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.

Fixed spacing style.

lutz_donnerhacke.de marked an inline comment as done and an inline comment as not done.Jan 10 2020, 10:54 AM
lutz_donnerhacke.de added inline comments.
sys/netgraph/ng_nat.c
819

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.

Add an explanation to the man page.

share/man/man4/ng_nat.4
396 ↗(On Diff #66679)

Wrong reference.

409 ↗(On Diff #66679)

Missing reference.

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.

eugen_grosbein.net requested changes to this revision.Jan 13 2020, 11:18 AM
This revision now requires changes to proceed.Jan 13 2020, 11:18 AM

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.

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.

Leave the man page untouched.

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.

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 added a comment.Jan 24 2020, 4:09 PM

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.

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.

swills added a subscriber: swills.Thu, Feb 6, 6:39 PM
lutz_donnerhacke.de marked an inline comment as done.Sat, Feb 8, 8:22 AM
markj accepted this revision.Tue, Feb 11, 8:07 PM
markj added a comment.Tue, Feb 11, 8:12 PM

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.

Thank you. Somebody need to land this.

This revision is now accepted and ready to land.Wed, Feb 12, 12:05 AM