diff mbox series

[v2] sunrpc: use snprintf instead of an implied length-guarantee on a format argument

Message ID 1606998152-21321-1-git-send-email-philipp.tomsich@vrull.eu
State Committed
Commit 66532a04aea77f379a165dbf55190f9452473eb9
Headers
Series [v2] sunrpc: use snprintf instead of an implied length-guarantee on a format argument |

Commit Message

Philipp Tomsich Dec. 3, 2020, 12:22 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=]

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 mbox series

Patch

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;