NetBSD Problem Report #36782

From Wolfgang.Stukenbrock@nagler-company.com  Tue Aug 14 16:43:47 2007
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id DC44A63B8EA
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 14 Aug 2007 16:43:47 +0000 (UTC)
Message-Id: <200708141643.l7EGhg5U015359@test-s0.nagler-company.com>
Date: Tue, 14 Aug 2007 18:43:42 +0200 (CEST)
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@NetBSD.org
Subject: inconsistent packet handling in IPSEC_NAT_T
X-Send-Pr-Version: 3.95

>Number:         36782
>Category:       kern
>Synopsis:       inconsistent packet handling in IPSEC_NAT_T
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Aug 14 16:45:00 +0000 2007
>Closed-Date:    Sat Feb 10 08:23:16 +0000 2018
>Last-Modified:  Sat Feb 10 08:23:16 +0000 2018
>Originator:     Wolfgang Stukenbrock
>Release:        NetBSD 3.1
>Organization:
Dr. Nagler & Company GmbH

>Environment:


System: NetBSD test-s0 3.1 NetBSD 3.1 (test-s0) #0: Tue Apr 3 11:33:43 CEST 2007 root@test-s0:/usr/src/sys/arch/i386/compile/test-s0 i386
Architecture: i386
Machine: i386
>Description:
	if IPSEC_NAT_T is compiled in the kernel UPD packets mayget checked for ESP inUDP encapsulation if the
	corresponding socket is setup in the INP_ESPINUDP or INP_ESPINUDP_NON_IKE mode.
	The whole thing is located in /usr/src/sys/netinet/udp_usrreq.c.
	The routine that does the removal stuff "udp4_espinudp()" is called around line 760. The code there expects,
	that a return value of -1 means hard error, 1 it was an ESP packet and 0 (or default) no ESP, continue UDP processing.

	But udp4_espinudp() will modify the mbuf chain, so that the value off gets invalid and udp4_sendup() will be called
	with nonsence.
	On the other hand, udp4_espinudp() assumes that it is an ecapsulated packet all the time - so normal UDP procession
	doesn't make realy sence, if udp4_espinudp() will fail for any reason. The port is used for NAT_T or not, but never
	for both at the same time.
	So it would be better to stop UDP-processing for that socket for all packets if the INP_ESP... option is set.
	Therefore after calling udp4_espinudp() there should no call to udp4_sendup() in any case anymore.
>How-To-Repeat:
	not relevant - source code study
>Fix:
	Handle 0 and default in the same way as return value 1 from udp4_espinudp().
	Perhaps drive some statistics dependend on correct ESPinUDP packets and faults.

>Release-Note:

>Audit-Trail:
From: "Maxime Villard" <maxv@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36782 CVS commit: src/sys/netinet
Date: Sat, 10 Feb 2018 08:17:00 +0000

 Module Name:	src
 Committed By:	maxv
 Date:		Sat Feb 10 08:17:00 UTC 2018

 Modified Files:
 	src/sys/netinet: udp_usrreq.c

 Log Message:
 If the socket wants a ESP-over-UDP packet, and the packet is incorrect,
 stop processing it instead of giving it to udp4_sendup. It just doesn't
 make any sense not to drop it.

 I was already telling myself this the other day when I visited this place,
 but I just saw PR/36782 (11 years old) that suggests the exact same thing,
 so fix it.

 Now, udp4_espinudp always frees the mbuf, and is made void. The packet is
 not processed any further afterwards.


 To generate a diff of this commit:
 cvs rdiff -u -r1.239 -r1.240 src/sys/netinet/udp_usrreq.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Sat, 10 Feb 2018 08:23:16 +0000
State-Changed-Why:
The original issue you reported (that there was a path where the mbuf was
modified but 'off' was not updated accordingly) was fixed a long time ago.

Your additional comment, about the fact that an incorrect ESP-over-UDP
packet should be dropped, is correct. I've just committed a fix.

I'm closing this PR. Thanks for the report.


>Unformatted:

Home	PR Database Search