Message ID | 1606935851-19711-1-git-send-email-philipp.tomsich@vrull.eu |
---|---|
State | Superseded |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E7D19396D81D; Wed, 2 Dec 2020 19:04:19 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-lj1-x243.google.com (mail-lj1-x243.google.com [IPv6:2a00:1450:4864:20::243]) by sourceware.org (Postfix) with ESMTPS id A19A8395CCD8 for <libc-alpha@sourceware.org>; Wed, 2 Dec 2020 19:04:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A19A8395CCD8 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=vrull.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=philipp.tomsich@vrull.eu Received: by mail-lj1-x243.google.com with SMTP id y10so5052964ljc.7 for <libc-alpha@sourceware.org>; Wed, 02 Dec 2020 11:04:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull-eu.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=WjjOiFTAHN4yup0zsjvbkQANZ4a5HGDt3GXSo94VZww=; b=sviSxDgCSqj6Tzal3hCs8IiymCanluvr8unpY2DPsLzWNNJC4IxVPjuWaHSqisPWpD 8lXjH78eBQNYw7GgSCaK1w8f9PnEf+t178mn/FlltnFM36IE8L2ScU7T4o8FC/8v0Fjw P/x2kx1M6kRneqvZygVKFzKY+wERggtlIcxxcAU+928+Kf6YvuJDnrMGbPjUUigsRcwK Cf8MYBP3c4JRqgpxbvQsroGyrJjwc3CNgCKaBzXsRkQFcnDtrZ/aTSxjt8AtawGHOtd1 hZwBpPwQTLO97SY767nInlpJeHgV/V9VTzwnpyC7qoOu9aFFQb4/DHb9k57/NzPx1cSU JP1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=WjjOiFTAHN4yup0zsjvbkQANZ4a5HGDt3GXSo94VZww=; b=gqKPEIoKiHvAZwydL9P55Mdb0SUUFDE39rX1HsfpoxE9YSDRudg65JtEdHWuSn10MQ YFQVUMRyX8uvIhopUCHMRmOlPwDWacJOJQvpGPiIrPG76or1Kf+rdiscAyL4/hyVo2zP JyTWApdd+DZUdMJY6rr+uh7PS/6xFvnaCindetV9lkjUob+CPHpwBi6G2T1akfXWGEd5 nGG4Q1HV1scpJlaf1EdDIAZ4VxGFVltExWX+fjFLAxBmOIAhpuL1dQHwk2XwLThb7KkY oQSCbWbSHz8EMc1MAq7d4YTo9+Ls34A3CSWtXeUZJtR5ZSNfT9l8sbbxVwaVEsB0XpYU t6Ug== X-Gm-Message-State: AOAM533eYliJPQsqZVxoZv8rLAYt7RFDStHigYaCCCV4KoQGuEu+2TVF REMOOm9OguhO0N0+C8PyaBjQo1z6l5nTZA== X-Google-Smtp-Source: ABdhPJyfVwz08aUvqTYRchOuJx6yqMEcIVZTiKbZoFPag2Q2E4eb2vqHxWZkRhIxwEt43fHKnCc9bw== X-Received: by 2002:a05:651c:3d0:: with SMTP id f16mr1793193ljp.109.1606935854178; Wed, 02 Dec 2020 11:04:14 -0800 (PST) Received: from centos7.localdomain (helsinki-01.engr.vrull.eu. [135.181.61.214]) by smtp.gmail.com with ESMTPSA id d25sm676534lfm.17.2020.12.02.11.04.13 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Dec 2020 11:04:13 -0800 (PST) From: Philipp Tomsich <philipp.tomsich@vrull.eu> To: libc-alpha@sourceware.org Subject: [PATCH] sunrpc: use snprintf to guard against buffer overflow Date: Wed, 2 Dec 2020 20:04:11 +0100 Message-Id: <1606935851-19711-1-git-send-email-philipp.tomsich@vrull.eu> X-Mailer: git-send-email 1.8.3.1 X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
sunrpc: use snprintf to guard against buffer overflow
|
|
Commit Message
Philipp Tomsich
Dec. 2, 2020, 7:04 p.m. UTC
GCC11 has improved detection of buffer overflows detectable through the analysis of format strings and parameters, which identifies the following issue: netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region of size between 239 and 249 [-Werror=format-overflow=] This rewrites user2netname() to use snprintf to guard against overflows. --- sunrpc/netname.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
* Philipp Tomsich: > GCC11 has improved detection of buffer overflows detectable through the analysis > of format strings and parameters, which identifies the following issue: > netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region > of size between 239 and 249 [-Werror=format-overflow=] > > This rewrites user2netname() to use snprintf to guard against overflows. > > --- > > sunrpc/netname.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/sunrpc/netname.c b/sunrpc/netname.c > index 24ee519..62e644f 100644 > --- a/sunrpc/netname.c > +++ b/sunrpc/netname.c > @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, > if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN) > return 0; > > - sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom); > - i = strlen (netname); > + i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom); > + if (i > (size_t) MAXNETNAMELEN) > + return 0; > + There's a strlen check right above the sprintf, and it looks like it would catch cases where we'd right too much into the buffer. So this looks like a GCC 11 false positive to me. Am I missing something? The switch to snprintf is reasonable (with the caveat that this code is in very, very deep maintenance mode), but I think you should replace the strlen check and also check for negative i. Thanks, Florian
On Thu, 3 Dec 2020 at 11:09, Florian Weimer <fweimer@redhat.com> wrote: > > * Philipp Tomsich: > > > GCC11 has improved detection of buffer overflows detectable through the analysis > > of format strings and parameters, which identifies the following issue: > > netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region > > of size between 239 and 249 [-Werror=format-overflow=] > > > > This rewrites user2netname() to use snprintf to guard against overflows. > > > > --- > > > > sunrpc/netname.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/sunrpc/netname.c b/sunrpc/netname.c > > index 24ee519..62e644f 100644 > > --- a/sunrpc/netname.c > > +++ b/sunrpc/netname.c > > @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, > > if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN) > > return 0; > > > > - sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom); > > - i = strlen (netname); > > + i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom); > > + if (i > (size_t) MAXNETNAMELEN) > > + return 0; > > + > > There's a strlen check right above the sprintf, and it looks like it > would catch cases where we'd right too much into the buffer. So this > looks like a GCC 11 false positive to me. Am I missing something? I should maybe reword the commit message to clearly state that this does not mitigate an exploitable overflow, but just addresses a language statement that the static analyzer in GCC chokes on (for the obvious reason that format-string is not checked against a ranger — although the "implied guarantee" on the length of the dfltdom-string will not be easily understandable to a compiler). > The switch to snprintf is reasonable (with the caveat that this code is > in very, very deep maintenance mode), but I think you should replace the > strlen check and also check for negative i. I'll provide a V2. Thanks, Philipp.
* Philipp Tomsich: >> The switch to snprintf is reasonable (with the caveat that this code is >> in very, very deep maintenance mode), but I think you should replace the >> strlen check and also check for negative i. > > I'll provide a V2. Looking forward to it. You can probably expand the definition of OPSYS and remove the now-unused macros, too. Thanks, Florian
On 12/3/20 3:09 AM, Florian Weimer via Libc-alpha wrote: > * Philipp Tomsich: > >> GCC11 has improved detection of buffer overflows detectable through the analysis >> of format strings and parameters, which identifies the following issue: >> netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region >> of size between 239 and 249 [-Werror=format-overflow=] >> >> This rewrites user2netname() to use snprintf to guard against overflows. >> >> --- >> >> sunrpc/netname.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/sunrpc/netname.c b/sunrpc/netname.c >> index 24ee519..62e644f 100644 >> --- a/sunrpc/netname.c >> +++ b/sunrpc/netname.c >> @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, >> if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN) >> return 0; >> >> - sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom); >> - i = strlen (netname); >> + i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom); >> + if (i > (size_t) MAXNETNAMELEN) >> + return 0; >> + > > There's a strlen check right above the sprintf, and it looks like it > would catch cases where we'd right too much into the buffer. So this > looks like a GCC 11 false positive to me. Am I missing something? There has been a change to the warning but I don't see it in my x86_64 build with the latest GCC/Glibc. Compiling with -ftree-dump-strlen=/dev/stdout prints the details for each directive. That should help explain how the warning came up with the number. My result looks like this: netname.c:52: sprintf: objsize = 256, fmtstr = "%s.%d@%s" Directive 1 at offset 0: "%s" Result: 4, 4, 4, 4 (4, 4, 4, 4) Directive 2 at offset 2: ".", length = 1 Result: 1, 1, 1, 1 (5, 5, 5, 5) Directive 3 at offset 3: "%d" Result: 1, 1, 11, 11 (6, 6, 16, 16) Directive 4 at offset 5: "@", length = 1 Result: 1, 1, 1, 1 (7, 7, 17, 17) Directive 5 at offset 6: "%s" Result: 0, 237, 237, 237 (7, 244, 254, 254) Directive 6 at offset 8: "", length = 1 If you see the warning on a target other that x86_64 I'll be happy to look into it if you can send me the translation unit (and the target) so I can easily reproduce it on my end. Martin > > The switch to snprintf is reasonable (with the caveat that this code is > in very, very deep maintenance mode), but I think you should replace the > strlen check and also check for negative i. > > Thanks, > Florian >
diff --git a/sunrpc/netname.c b/sunrpc/netname.c index 24ee519..62e644f 100644 --- a/sunrpc/netname.c +++ b/sunrpc/netname.c @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN) return 0; - sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom); - i = strlen (netname); + i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom); + if (i > (size_t) MAXNETNAMELEN) + return 0; + if (netname[i - 1] == '.') netname[i - 1] = '\0'; return 1;