From owner-svn-src-all@freebsd.org Thu Jul 25 01:57:00 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 0EB1BBFCF2; Thu, 25 Jul 2019 01:57:00 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id ECAED88DDA; Thu, 25 Jul 2019 01:56:57 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-io1-f67.google.com with SMTP id i10so93763576iol.13; Wed, 24 Jul 2019 18:56:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc; bh=mYhSleMh0kX4qcI4pHUXwHl0jbw4yu48cFnyNukh5Jo=; b=qy+3QFr2Tvbomx7iVQabDiaGVo8rmzV0q3at+hs0Pz6sZYL+U9bi7xLVbmsN459g/c l0MxG0NNsDXBi2//rkztJj6B2LuC9Y+qT8JPykIEhYVGKHnPK0IHxNEEDvj89KEET1Zm Y2rFAYL2CNZuPcK2+OX14NmNCZmJ4LdldyrB/DP5Mx8hmc/qyWl2t7/cXyrTNE0U3kmM n+uY+o1Espgusl8BKH2FvPlxk9JFF44vvXcvJ+ZoNqA0PuEJddWaoN+EA/4s1zfiib1A FoOEb6E7lIzfWARdL9VzH4EA9lm6waKPlCoqn+O3o7JuSbkeMAg2URL7BVdOQHdfCTlC SGfQ== X-Gm-Message-State: APjAAAVMgjj/QNs6We5PpvTRyWh9WMeQDvQdjdNjCGRK0+it6WBvbYkT LiZPLI9egEy2+BKDZq/aTKyKfEbO X-Google-Smtp-Source: APXvYqy3qZGqJ4XC0DckebikwJM1C7f4eaXF9U11FyTY5Gsp1+vFg0y4IqzV5XGYAI3jjUiTuAG2rw== X-Received: by 2002:a02:c550:: with SMTP id g16mr86349353jaj.49.1564019466723; Wed, 24 Jul 2019 18:51:06 -0700 (PDT) Received: from mail-io1-f47.google.com (mail-io1-f47.google.com. [209.85.166.47]) by smtp.gmail.com with ESMTPSA id u17sm45264149iob.57.2019.07.24.18.51.05 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 24 Jul 2019 18:51:06 -0700 (PDT) Received: by mail-io1-f47.google.com with SMTP id j6so18655940ioa.5; Wed, 24 Jul 2019 18:51:05 -0700 (PDT) X-Received: by 2002:a02:6d24:: with SMTP id m36mr89865171jac.87.1564019465491; Wed, 24 Jul 2019 18:51:05 -0700 (PDT) MIME-Version: 1.0 References: <201907101742.x6AHg4os016752@repo.freebsd.org> In-Reply-To: <201907101742.x6AHg4os016752@repo.freebsd.org> Reply-To: cem@freebsd.org From: Conrad Meyer Date: Wed, 24 Jul 2019 18:50:54 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r349890 - head/contrib/telnet/telnet To: Philip Paeps Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: ECAED88DDA X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; spf=pass (mx1.freebsd.org: domain of csecem@gmail.com designates 209.85.166.67 as permitted sender) smtp.mailfrom=csecem@gmail.com X-Spamd-Result: default: False [-4.33 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; HAS_REPLYTO(0.00)[cem@freebsd.org]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; REPLYTO_ADDR_EQ_FROM(0.00)[]; RCVD_COUNT_THREE(0.00)[4]; TO_DN_ALL(0.00)[]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; NEURAL_HAM_SHORT(-0.74)[-0.737,0]; FORGED_SENDER(0.30)[cem@freebsd.org,csecem@gmail.com]; MIME_TRACE(0.00)[0:+]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; TAGGED_FROM(0.00)[]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; FROM_NEQ_ENVFROM(0.00)[cem@freebsd.org,csecem@gmail.com]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[freebsd.org]; RCVD_TLS_LAST(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[67.166.85.209.list.dnswl.org : 127.0.5.0]; IP_SCORE(-1.59)[ip: (-2.01), ipnet: 209.85.128.0/17(-3.44), asn: 15169(-2.43), country: US(-0.05)] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jul 2019 01:57:00 -0000 Hi Philip, Sorry I'm late to the party. Unless I am mistaken, most of these are *not* overflows or vulnerabilities of any sort. On Wed, Jul 10, 2019 at 10:42 AM Philip Paeps wrote: > > Author: philip > Date: Wed Jul 10 17:42:04 2019 > New Revision: 349890 > URL: https://svnweb.freebsd.org/changeset/base/349890 > > Log: > telnet: fix a couple of snprintf() buffer overflows >... > --- head/contrib/telnet/telnet/commands.c Wed Jul 10 17:21:59 2019 (r349889) > +++ head/contrib/telnet/telnet/commands.c Wed Jul 10 17:42:04 2019 (r349890) > @@ -1655,10 +1655,11 @@ env_init(void) > char hbuf[256+1]; > char *cp2 = strchr((char *)ep->value, ':'); > > - gethostname(hbuf, 256); > - hbuf[256] = '\0'; > - cp = (char *)malloc(strlen(hbuf) + strlen(cp2) + 1); > - sprintf((char *)cp, "%s%s", hbuf, cp2); > + gethostname(hbuf, sizeof(hbuf)); > + hbuf[sizeof(hbuf)-1] = '\0'; > + unsigned int buflen = strlen(hbuf) + strlen(cp2) + 1; > + cp = (char *)malloc(sizeof(char)*buflen); > + snprintf((char *)cp, buflen, "%s%s", hbuf, cp2); This patch makes no functional change, except gethostname()'s 2nd parameter is now 257 instead of 256. It was not an overflow before and the formatted malloc contents are identical. > Modified: head/contrib/telnet/telnet/telnet.c > ============================================================================== > --- head/contrib/telnet/telnet/telnet.c Wed Jul 10 17:21:59 2019 (r349889) > +++ head/contrib/telnet/telnet/telnet.c Wed Jul 10 17:42:04 2019 (r349890) > @@ -785,7 +785,7 @@ suboption(void) > name = gettermname(); > len = strlen(name) + 4 + 2; > if (len < NETROOM()) { > - sprintf(temp, "%c%c%c%c%s%c%c", IAC, SB, TELOPT_TTYPE, > + snprintf(temp, sizeof(temp), "%c%c%c%c%s%c%c", IAC, SB, TELOPT_TTYPE, > TELQUAL_IS, name, IAC, SE); This one actually overflowed before. But the new behavior isn't much better. Truncating the formatted string arbitrarily is still wrong; it would be better to errx() or assert() or abort(). > @@ -807,7 +807,7 @@ suboption(void) > > TerminalSpeeds(&ispeed, &ospeed); > > - sprintf((char *)temp, "%c%c%c%c%ld,%ld%c%c", IAC, SB, TELOPT_TSPEED, > + snprintf((char *)temp, sizeof(temp), "%c%c%c%c%ld,%ld%c%c", IAC, SB, TELOPT_TSPEED, > TELQUAL_IS, ospeed, ispeed, IAC, SE); > len = strlen((char *)temp+4) + 4; /* temp[3] is 0 ... */ Unless I'm miscounting, this could not overflow (on any FreeBSD system with 64-bit or smaller long)... > Modified: head/contrib/telnet/telnet/utilities.c > ============================================================================== > --- head/contrib/telnet/telnet/utilities.c Wed Jul 10 17:21:59 2019 (r349889) > +++ head/contrib/telnet/telnet/utilities.c Wed Jul 10 17:42:04 2019 (r349890) > @@ -629,7 +629,7 @@ printsub(char direction, unsigned char *pointer, int l > } > { > char tbuf[64]; > - sprintf(tbuf, "%s%s%s%s%s", > + snprintf(tbuf, sizeof(tbuf), "%s%s%s%s%s", > pointer[2]&MODE_EDIT ? "|EDIT" : "", > pointer[2]&MODE_TRAPSIG ? "|TRAPSIG" : "", > pointer[2]&MODE_SOFT_TAB ? "|SOFT_TAB" : "", This one could not overflow before either. I think most of this change is an unnecessary regression, and the actual overflow should be fixed in a better way anyway. Thanks, Conrad