NetBSD Problem Report #36997

From martin@duskware.de  Mon Sep 17 10:14:44 2007
Return-Path: <martin@duskware.de>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 6B46763B8E9
	for <gnats-bugs@gnats.netbsd.org>; Mon, 17 Sep 2007 10:14:44 +0000 (UTC)
Message-Id: <20070917101241.CC27463B8E9@narn.NetBSD.org>
Date: Mon, 17 Sep 2007 10:12:41 +0000 (UTC)
From: zafer@aydogan.de
Reply-To: zafer@aydogan.de
To: netbsd-bugs-owner@NetBSD.org
Subject: ping doesn't verify limits
X-Send-Pr-Version: www-1.0

>Number:         36997
>Category:       bin
>Synopsis:       ping doesn't verify limits
>Confidential:   no
>Severity:       non-critical
>Priority:       high
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Sep 17 10:15:00 +0000 2007
>Closed-Date:    Thu Aug 17 18:40:39 +0000 2017
>Last-Modified:  Thu Aug 17 18:40:39 +0000 2017
>Originator:     Zafer Aydogan
>Release:        4.99.31
>Organization:
>Environment:
i386
>Description:
I think I found a bug in ping.

When playing around with ping, I noticed that there is no sufficient
verification for limits for the options -i, -l and -w.
For example, if you enter a really big interval, then poll() bails
out, since it can only handle  LONG_MAX (2147483647). But entering far
larger number lets ping hang.
More interesting and weird is that, if the interval is smaller than
0.000001 then ping hangs aswell. I thought everything less than
0.000001 should be treated like 0, but it isn't.

In my patch I've split the error messages to be more friendly and precise.
I multiply interval with 1000 to match LONG_MAX, which means the
largest interval will be 2147482 and the smallest will be 0.0001.
Smaller ones will be treated like 0.0001 (1000 Hz).

My patch may be incomplete or wrong.
Please review it at: http://www.aydogan.org/ping/ping.diff
(written against current).

When verified, a patch should be pulled up to 4.0 aswell.

Thanks, Zafer.
>How-To-Repeat:
ping -i 0.0000001 127.0.0.1
ping -i 9999999999 127.0.0.1

and

ping -i 99999999 127.0.0.1
>Fix:
--- ping.c.orig	2007-09-15 16:11:39.000000000 +0200
+++ ping.c	2007-09-15 18:36:32.000000000 +0200
@@ -283,8 +283,11 @@
 			break;
 		case 'c':
 			npackets = strtol(optarg, &p, 0);
-			if (*p != '\0' || npackets <= 0)
-				errx(1, "Bad/invalid number of packets");
+			if (*p != '\0') 
+				errx(1, "Invalid number of packets");
+
+			if (npackets <= 0 || npackets > INT_MAX)
+				errx(1, "Bad number of packets");
 			break;
 		case 'D':
 			pingflags |= F_DF;
@@ -300,13 +303,23 @@
 			break;
 		case 'i':		/* wait between sending packets */
 			interval = strtod(optarg, &p);
-			if (*p != '\0' || interval <= 0)
-				errx(1, "Bad/invalid interval %s", optarg);
+			if (*p != '\0')
+				errx(1, "Invalid timing interval %s", optarg);
+
+			if (interval <= 0 || interval * 1000 > INT_MAX)
+				errx(1, "Bad timing interval %s", optarg);
+			
+			if (interval < 0.0001) 
+				interval = 0.0001;
 			break;
 		case 'l':
 			preload = strtol(optarg, &p, 0);
-			if (*p != '\0' || preload < 0)
-				errx(1, "Bad/invalid preload value %s",
+			if (*p != '\0')
+				errx(1, "Invalid preload value %s",
+				     optarg);
+
+			if (preload < 0 || preload > INT_MAX)
+				errx(1, "Bad preload value %s",
 				     optarg);
 			break;
 		case 'n':
@@ -372,8 +385,11 @@
 			break;
 		case 'w':
 			maxwait = strtod(optarg, &p);
-			if (*p != '\0' || maxwait <= 0)
-				errx(1, "Bad/invalid maxwait time %s", optarg);
+			if (*p != '\0') 
+				errx(1, "Invalid maxwait time %s", optarg);
+
+			if ( maxwait <= 0 || maxwait > INT_MAX)
+				errx(1, "Bad maxwait time %s", optarg);
 			break;
 #ifdef IPSEC
 #ifdef IPSEC_POLICY_IPSEC

>Release-Note:

>Audit-Trail:
From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/36997: ping doesn't verify limits
Date: Mon, 17 Sep 2007 21:17:22 +0100

 On Mon, Sep 17, 2007 at 10:15:00AM +0000, zafer@aydogan.de wrote:
 > >Number:         36997
 > >Category:       bin
 > >Synopsis:       ping doesn't verify limits
 ...
 > I think I found a bug in ping.
 > 
 > When playing around with ping, I noticed that there is no sufficient
 > verification for limits for the options -i, -l and -w.
 ...

 There is a general statement in the SUS that command line utilities are
 not required to perform numeric overflow tests on command line arguments.
 There is probably a proviso that the erronous values don't have any
 nasty side effects (eg buffer overruns).

 Whether this should apply to ping is another matter.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: src/sbin/ping
Date: Sun, 18 Dec 2016 00:21:33 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Sun Dec 18 00:21:33 UTC 2016

 Modified Files:
 	src/sbin/ping: ping.c

 Log Message:
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.

 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.

 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.

 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.112 -r1.113 src/sbin/ping/ping.c

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

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: src/sbin/ping
Date: Sun, 18 Dec 2016 01:19:34 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Sun Dec 18 01:19:34 UTC 2016

 Modified Files:
 	src/sbin/ping: ping.c

 Log Message:
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.

 Reject packet intervals < 1 ns as they lead to infinite loops adding
 zero timespecs.

 Fix the behind-schedule behavior so it doesn't spend all its time in
 that loop adding very small timespecs. Try ping -c 500 -i 0.000000001
 to see this in action with the old ping.


 To generate a diff of this commit:
 cvs rdiff -u -r1.113 -r1.114 src/sbin/ping/ping.c

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

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: src/sbin/ping6
Date: Sun, 18 Dec 2016 01:30:54 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Sun Dec 18 01:30:54 UTC 2016

 Modified Files:
 	src/sbin/ping6: ping6.c

 Log Message:
 Merge the PR 36997 fixes into ping6. It already didn't accept too
 small or wildly too large intervals, but it did allow intervals that
 failed at poll(). Since that's signed integer overflow and thus UB,
 better not to.


 To generate a diff of this commit:
 cvs rdiff -u -r1.92 -r1.93 src/sbin/ping6/ping6.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->pending-pullups
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 18 Dec 2016 01:51:47 +0000
State-Changed-Why:
pullup-6 #1424, pullup-7 #1333
(for the first commit only, the other one doesn't seem critical)


From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/36997 (ping doesn't verify limits)
Date: Sun, 18 Dec 2016 01:58:05 +0000

 On Mon, Sep 17, 2007 at 10:15:00 +0000, Zafer Aydogan wrote:
  > +                       if (npackets <= 0 || npackets > INT_MAX)
  > +                               errx(1, "Bad number of packets");

 Unfortunately, it doesn't work too well to test an int for being
 greater than INT_MAX :-)

 The changes I made are mostly different, but should cover the same
 territory, except that I don't see any reason to limit the -w value;
 it's not an integer and it's not converted to one later on.

 -- 
 David A. Holland
 dholland@netbsd.org

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: [netbsd-7] src/sbin/ping
Date: Sun, 18 Dec 2016 08:22:29 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sun Dec 18 08:22:29 UTC 2016

 Modified Files:
 	src/sbin/ping [netbsd-7]: ping.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1333):
 	sbin/ping/ping.c: revision 1.113
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.
 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.
 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.
 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.107.4.1 -r1.107.4.2 src/sbin/ping/ping.c

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: [netbsd-7-0] src/sbin/ping
Date: Sun, 18 Dec 2016 08:40:54 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sun Dec 18 08:40:54 UTC 2016

 Modified Files:
 	src/sbin/ping [netbsd-7-0]: ping.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1333):
 	sbin/ping/ping.c: revision 1.113
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.
 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.
 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.
 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.107.4.1 -r1.107.4.1.2.1 src/sbin/ping/ping.c

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: [netbsd-6-0] src/sbin/ping
Date: Tue, 11 Jul 2017 21:15:23 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jul 11 21:15:23 UTC 2017

 Modified Files:
 	src/sbin/ping [netbsd-6-0]: ping.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1424):
 	sbin/ping/ping.c: revision 1.113 via patch
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.
 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.
 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.
 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.102 -r1.102.8.1 src/sbin/ping/ping.c

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: [netbsd-6-1] src/sbin/ping
Date: Tue, 11 Jul 2017 21:15:43 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jul 11 21:15:43 UTC 2017

 Modified Files:
 	src/sbin/ping [netbsd-6-1]: ping.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1424):
 	sbin/ping/ping.c: revision 1.113 via patch
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.
 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.
 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.
 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.102.2.1 -r1.102.2.1.2.1 src/sbin/ping/ping.c

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36997 CVS commit: [netbsd-6] src/sbin/ping
Date: Tue, 11 Jul 2017 21:16:08 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jul 11 21:16:07 UTC 2017

 Modified Files:
 	src/sbin/ping [netbsd-6]: ping.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1424):
 	sbin/ping/ping.c: revision 1.113 via patch
 PR bin/36997 Zafer Aydogan: ping doesn't validate numeric inputs enough.
 Check for values between INT_MAX and LONG_MAX (if they're different)
 when using strtol to get an int. This applies to the -c and -l options;
 the other uses were already checked.
 Also limit the inter-packet interval given with -i to values that
 don't cause integer overflow calling poll() with milliseconds.
 Really large intervals (the number is read as floating point) can
 produce positive poll() values but negative integers when converted to
 struct timespec; this produces behavior akin to using -l at first and
 could be construed as a local DoS vulnerability.


 To generate a diff of this commit:
 cvs rdiff -u -r1.102.2.1 -r1.102.2.2 src/sbin/ping/ping.c

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

State-Changed-From-To: pending-pullups->closed
State-Changed-By: jdolecek@NetBSD.org
State-Changed-When: Thu, 17 Aug 2017 18:40:39 +0000
State-Changed-Why:
Pullups to 6, 6-0, 6-1, 7, and 7-0 branches done.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.