From philipp@mirapoint.com Tue Feb 23 15:14:11 1999 Return-Path: Received: from mail.mirapoint.com (mail.mirapoint.com [209.157.59.162]) by hub.freebsd.org (Postfix) with ESMTP id 96BF410EA2 for ; Tue, 23 Feb 1999 15:14:09 -0800 (PST) (envelope-from philipp@mirapoint.com) Received: from mirapoint.com (putois.mirapoint.com [192.168.0.96]) by mail.mirapoint.com (1.0.0/1.0.Beta1) with ESMTP id ADJ00733 Tue, 23 Feb 1999 15:14:08 -0800 (PST) Message-Id: <36D2E220.9A563E10@mirapoint.com> Date: Tue, 23 Feb 1999 09:15:12 -0800 From: "Philip A. Prindeville" Sender: philipp@mirapoint.com To: FreeBSD-gnats-submit@freebsd.org Subject: [PATCH] inet_addr() doesn't check for illegal values in input >Number: 10231 >Category: misc >Synopsis: inet_addr() doesn't check for illegal values in input >Confidential: no >Severity: non-critical >Priority: medium >Responsible: freebsd-bugs >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Tue Feb 23 15:20:00 PST 1999 >Closed-Date: Tue May 29 05:27:52 PDT 2001 >Last-Modified: Tue May 29 05:29:41 PDT 2001 >Originator: Philip A. Prindeville >Release: FreeBSD 2.2.8-RELEASE i386 >Organization: Mirapoint, Inc. >Environment: FreeBSD putois.mirapoint.com 2.2.8-RELEASE FreeBSD 2.2.8-RELEASE #0: Mon Nov 30 06:34:08 GMT 1998 jkh@time.cdrom.com:/usr/src/sys/compile/GENERIC i386 >Description: Input passed to inet_addr() is not correctly checked for validity. For instance, 437458475894848475 would be accepted, even though it will overflow a 32bit quantity. Likewise, on a four-part dotted-quad only the last integer is checked for correctness. >How-To-Repeat: call inet_addr("3493748787895789475489") and it won't return INADDR_NONE. Similarly, inet_addr("257.0.0.10") will return 0x0100000a... (on non-intel machines, anyway) >Fix: The following patch ensures that 32bits are never overflowed, and that the higher-order quads in a tuple, triple, or quadruple don't exceed 8 bits. --------------817090D209D8472FD395DE10 Content-Type: text/plain; charset=us-ascii; name="patches" Content-Disposition: inline; filename="patches" Content-Transfer-Encoding: 7bit *** inet_addr.c# Wed Feb 3 10:18:21 1999 --- inet_addr.c Tue Feb 23 07:57:16 1999 *************** *** 115,123 **** --- 115,127 ---- } for (;;) { if (isascii(c) && isdigit(c)) { + if (val >= (ULONG_MAX) / base) + return (0); val = (val * base) + (c - '0'); c = *++cp; } else if (base == 16 && isascii(c) && isxdigit(c)) { + if (val >= (ULONG_MAX) / base) + return (0); val = (val << 4) | (c + 10 - (islower(c) ? 'a' : 'A')); c = *++cp; *************** *** 157,175 **** break; case 2: /* a.b -- 8.24 bits */ ! if (val > 0xffffff) return (0); val |= parts[0] << 24; break; case 3: /* a.b.c -- 8.8.16 bits */ ! if (val > 0xffff) return (0); val |= (parts[0] << 24) | (parts[1] << 16); break; case 4: /* a.b.c.d -- 8.8.8.8 bits */ ! if (val > 0xff) return (0); val |= (parts[0] << 24) | (parts[1] << 16) | (parts[2] << 8); break; --- 161,180 ---- break; case 2: /* a.b -- 8.24 bits */ ! if (parts[0] > 0xff || val > 0xffffff) return (0); val |= parts[0] << 24; break; case 3: /* a.b.c -- 8.8.16 bits */ ! if (parts[0] > 0xff || parts[1] || val > 0xffff) return (0); val |= (parts[0] << 24) | (parts[1] << 16); break; case 4: /* a.b.c.d -- 8.8.8.8 bits */ ! if (parts[0] > 0xff || parts[1] > 0xff || parts[2] > 0xff ! || val > 0xff) return (0); val |= (parts[0] << 24) | (parts[1] << 16) | (parts[2] << 8); break; --------------817090D209D8472FD395DE10-- >Release-Note: >Audit-Trail: State-Changed-From-To: open->suspended State-Changed-By: n_hibma State-Changed-When: Fri Jul 16 02:25:39 PDT 1999 State-Changed-Why: awaiting committer From: Nick Hibma To: freebsd-gnats-submit@FreeBSD.org, wpaul@FreeBSD.org, jdp@FreeBSD.org Cc: philipp@mirapoint.com Subject: Re: misc/10231: inet_addr() doesn't check for illegal values in input Date: Fri, 16 Jul 1999 11:30:12 +0200 Paul, John, Any comments on this PR? Synopsis: Input passed to inet_addr() is not correctly checked for validity. For instance, 437458475894848475 would be accepted, even though it will overflow a 32bit quantity. Likewise, on a four-part dotted-quad only the last integer is checked for correctness. Cheers, Nick -- Paranoid: perl -e 'use strict;' -e ... ISIS/STA, T.P.270, Joint Research Centre, 21020 Ispra, Italy From: John Polstra To: Nick Hibma Cc: philipp@mirapoint.com, jdp@FreeBSD.org, wpaul@FreeBSD.org, freebsd-gnats-submit@FreeBSD.org Subject: Re: misc/10231: inet_addr() doesn't check for illegal values in Date: Fri, 16 Jul 1999 08:59:53 -0700 (PDT) Nick Hibma wrote: > Any comments on this PR? > > > Synopsis: > > Input passed to inet_addr() is not correctly checked for > validity. For instance, 437458475894848475 would be accepted, > even though it will overflow a 32bit quantity. > > Likewise, on a four-part dotted-quad only the last integer > is checked for correctness. Yes, it's a bug. The patch has the right idea but it isn't quite right in all the details. For example, in the first chunk of the patch: *** 115,123 **** --- 115,127 ---- } for (;;) { if (isascii(c) && isdigit(c)) { + if (val >= (ULONG_MAX) / base) + return (0); val = (val * base) + (c - '0'); c = *++cp; } else if (base == 16 && isascii(c) && isxdigit(c)) { + if (val >= (ULONG_MAX) / base) + return (0); val = (val << 4) | (c + 10 - (islower(c) ? 'a' : 'A')); c = *++cp; overflow won't be detected if (val == ULONG_MAX / base) and c is not '0'. A simpler and more reliable check would be this: u_int32_t val; /* Notice the new type of this variable */ u_int32_t oldval; ... if (isascii(c) && isdigit(c)) { oldval = val; val = val * base + c - '0'; if (val < oldval) return (0); c = *++cp; } ... and so forth. Also, in the second chunk of the patch, under case 3, there's a bug. The line should read: if (parts[0] > 0xff || parts[1] > 0xff || val > 0xffff) John From: Garrett Wollman To: John Polstra Cc: freebsd-gnats-submit@FreeBSD.ORG Subject: Re: misc/10231: inet_addr() doesn't check for illegal values in Date: Fri, 16 Jul 1999 12:17:34 -0400 (EDT) < said: > Yes, it's a bug. The patch has the right idea but it isn't quite > right in all the details. For example, in the first chunk of the > patch: > Also, in the second chunk of the patch, under case 3, there's a bug. > The line should read: Actually, the whole function should be rewritten to use strtoul() rather than hand-rolling Yet Another integer parser. -GAWollman -- Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same wollman@lcs.mit.edu | O Siem / The fires of freedom Opinions not those of| Dance in the burning flame MIT, LCS, CRS, or NSA| - Susan Aglukark and Chad Irschick From: Adrian Chadd To: freebsd-gnats-submit@freebsd.org Cc: Subject: Re: misc/10231 inet_addr() not checking for illegal values in input Date: Sun, 8 Aug 1999 02:13:15 +0800 (WST) There's a diff and rewritten src/lib/libc/net/inet_addr.c file with a strtoul'ified routine. I hear rumours that the new bind has a rewritten inet_addr(), but I thought I might as well try to close another PR. Adrian From: Adrian Chadd To: freebsd-gnats-submit@freebsd.org Cc: Subject: Re: misc/10231 inet_addr() not checking for illegal values in input Date: Sun, 8 Aug 1999 03:06:13 +0800 (WST) > > There's a diff and rewritten src/lib/libc/net/inet_addr.c file with a > strtoul'ified routine. I hear rumours that the new bind has a rewritten > inet_addr(), but I thought I might as well try to close another PR. > I did it again. http://www.freebsd.org/~adrian/inet_addr/ Adrian State-Changed-From-To: suspended->analyzed State-Changed-By: green State-Changed-When: Sat Oct 30 21:08:41 PDT 1999 State-Changed-Why: This behavior has been corrected in -CURRENT with Adrian's code; MFC to -STABLE should be forthcoming soon. State-Changed-From-To: analyzed->closed State-Changed-By: des State-Changed-When: Tue May 29 05:27:52 PDT 2001 State-Changed-Why: Stale PR, bug fixed on all supported branches. http://www.FreeBSD.org/cgi/query-pr.cgi?pr=10231 >Unformatted: