Hi,
This patch removes a dead-code overflow check in sshbuf_dup_string() and
replaces it with a correct guard.
What was wrong
- l is a size_t; the expression l > SIZE_MAX is always false, so the
protection never triggered.
- If l == SIZE_MAX, the expression l + 1 overflows to 0; allocating 0
bytes and then copying l bytes invokes undefined behavior
Alternative considered ? remove the length check entirely
The sshbuf layer already enforces the invariant len <= SSHBUF_SIZE_MAX,
so in normal operation l can never reach SIZE_MAX. In principle we could
therefore drop the overflow guard and keep only the s == NULL test.
The issue was found via static analysis.
The patch applies cleanly to current master and passes all CI tests.
GitHub mirror PR (with CI results):
https://github.com/openssh/openssh-portable/pull/573
Please review.
Thanks,
Boris
From 123429f33990652797799d97ca686f3a74c79f08 Mon Sep 17 00:00:00 2001
From: Boris Tonofa <b.tonofa at ideco.ru>
Date: Thu, 12 Jun 2025 18:57:16 +0300
Subject: [PATCH] fix incorrect overflow check
---
sshbuf-misc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sshbuf-misc.c b/sshbuf-misc.c
index adbf9903b..ad7398ad9 100644
--- a/sshbuf-misc.c
+++ b/sshbuf-misc.c
@@ -254,7 +254,7 @@ sshbuf_dup_string(struct sshbuf *buf)
size_t l = sshbuf_len(buf);
char *r;
- if (s == NULL || l > SIZE_MAX)
+ if (s == NULL || l == SIZE_MAX)
return NULL;
/* accept a nul only as the last character in the buffer */
if (l > 0 && (p = memchr(s, '\0', l)) != NULL) {
--
2.47.0