Page MenuHomeFreeBSD

netgraph/ng_nat: Prevent crash by malformated packets
ClosedPublic

Authored by donner on Jan 8 2020, 5:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 3:30 AM
Unknown Object (File)
Sat, Mar 9, 3:30 AM
Unknown Object (File)
Sat, Mar 9, 3:30 AM
Unknown Object (File)
Sat, Mar 9, 3:30 AM
Unknown Object (File)
Feb 5 2024, 4:09 AM
Unknown Object (File)
Feb 4 2024, 10:18 PM
Unknown Object (File)
Jan 2 2024, 8:10 AM
Unknown Object (File)
Dec 12 2023, 7:33 AM

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj added inline comments.
sys/netgraph/ng_nat.c
816 ↗(On Diff #66507)

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

Checking available space before accessing it.

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 marked an inline comment as done and an inline comment as not done.Jan 10 2020, 10:54 AM
donner added inline comments.
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.

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.

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 revision now requires changes to proceed.Jan 13 2020, 11:18 AM
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.

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.

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.

In D23091#511942, @lutz_donnerhacke.de 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.

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.

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.

In D23091#518544, @lutz_donnerhacke.de wrote:

@eugen_grosbein.net are your concerns handled?

Yes.

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