From patchwork Thu Dec 3 12:22:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philipp Tomsich X-Patchwork-Id: 41279 Return-Path: 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 94387385702C; Thu, 3 Dec 2020 12:22:43 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-lf1-x143.google.com (mail-lf1-x143.google.com [IPv6:2a00:1450:4864:20::143]) by sourceware.org (Postfix) with ESMTPS id 4BB8C3858024 for ; Thu, 3 Dec 2020 12:22:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4BB8C3858024 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-lf1-x143.google.com with SMTP id z21so2388621lfe.12 for ; Thu, 03 Dec 2020 04:22:40 -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=gYNrYNcGGUVZ6VH2SK682yyPxy0wOZRmzdroHKlHvr4=; b=EoqMXFv6D0f1VsNNohqSCFPtkUzXDhKPwbVQoaBvkEHUnw/9UwH7Rz+onqxtGCvfzG kUDBTSwX3rbBwjbIicvZEae17XTZKTAt9McSCLGlHnvpQtA7wD+9DwmsK1ZmxI0qUhdz bpeY+2r3d2yIA2vqsEUjqiGAw5HcWebBMAsFD56faY3rnoKpQu037nuXb93D2AhFNkvs VdMScFNFWj3m4Sy59eF8B5ZXu782Ra9J0cqNi+oFJP1s7hpyl3yHkEedVaoSWGJ2x8MO 4+mVDwRXHZut0z/CxVkq36rdG5T2FKhtBUWEMEUFvK7OMrdwnzSFqi3Cn5Qe/JkRJF9/ LucQ== 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=gYNrYNcGGUVZ6VH2SK682yyPxy0wOZRmzdroHKlHvr4=; b=OWtmOf0LEq71BNz76eyOfu6RcyaOmpd+umhNojtHrZaz97qfVah4XbgbN8bFG487Vu JeE49+s7++lV6qxaaEQ736S7jx6Nj0JuQJW8pckUE4ZxhLstOGRXOa9Rl+7etuzsY8L4 TiWp6pzfVex5AQIqzgZe3FGwRH54ffvr4tsy7/cWMRgfhGHuo3uHxnrxXJLN0+eMgR9Y Dbit6gHgL2dfH1UuLXcDyM4lzhb3rT+/qPJoYMamej2dgCclJ9w9guOijo/3gat0NkyM npyBiKKrEZdc6C7TMiJ/losEaOG27yM8khcYx/DPkE53EavpV4dye8/c1YxiLvjpwpRp AWSQ== X-Gm-Message-State: AOAM5321Lo5A2AV1GyUI07a+8xkuki/0r8fiI1ioOqFcFdahSMgFom8O 1ggEzXWxgcGLMsVMDKNmjX1/HzjoyDY5E+Tc5Tk= X-Google-Smtp-Source: ABdhPJwVx1LYV+MkXu04dPuynwdLe+/H4J54KXd38HadsJcB+ApMjXHVcEn1iLvtlkGfY2UvPWUFhQ== X-Received: by 2002:ac2:4844:: with SMTP id 4mr1106540lfy.64.1606998158953; Thu, 03 Dec 2020 04:22:38 -0800 (PST) Received: from centos7.localdomain (helsinki-01.engr.vrull.eu. [135.181.61.214]) by smtp.gmail.com with ESMTPSA id a26sm363118ljn.137.2020.12.03.04.22.38 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Dec 2020 04:22:38 -0800 (PST) From: Philipp Tomsich To: libc-alpha@sourceware.org Subject: [PATCH v2] sunrpc: use snprintf instead of an implied length-guarantee on a format argument Date: Thu, 3 Dec 2020 13:22:32 +0100 Message-Id: <1606998152-21321-1-git-send-email-philipp.tomsich@vrull.eu> X-Mailer: git-send-email 1.8.3.1 X-Spam-Status: No, score=-11.7 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Florian Weimer , Martin Sebor , Philipp Tomsich Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" 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=] While an if-check prior to the format-directive implies a guarantee on the length of the respective format argument, the compiler does not derive a range from that implied guarantuee and does not consider the range in performing the checking. This rewrites user2netname() to use snprintf to guard against overflows. In doing so, it removes the if-check and related macros. --- Changes in v2: - Use ssize_t to hold the return from snprintf and check for negative return values indicating an error. - Remove the if-statement that checks the length of the format arguments and provides an implied guarantuee. - Reword the commit message to clearly state that this does not address an exploitable overflow (as there is an implied guarantuee on the length of the string from a previous if-statement). sunrpc/netname.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sunrpc/netname.c b/sunrpc/netname.c index 24ee519..251ab94 100644 --- a/sunrpc/netname.c +++ b/sunrpc/netname.c @@ -24,8 +24,7 @@ #include "nsswitch.h" -#define OPSYS_LEN 4 -#define MAXIPRINT (11) /* max length of printed integer */ +#define OPSYS_LEN 4 static const char OPSYS[] = "unix"; int @@ -33,7 +32,7 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, const char *domain) { char dfltdom[MAXNETNAMELEN + 1]; - size_t i; + ssize_t i; if (domain == NULL) { @@ -46,11 +45,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid, dfltdom[MAXNETNAMELEN] = '\0'; } - if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN) + i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom); + if (i <= 0 || i > (ssize_t) MAXNETNAMELEN) return 0; - sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom); - i = strlen (netname); if (netname[i - 1] == '.') netname[i - 1] = '\0'; return 1;