[PATCH v2 0/5] Small, assorted "hardening" fixes
All harmless issues as far as I can tell, but nice to fix. v2: - in 3/5: - keep 'skip' in write_remainder() unsigned, and check for unsigned overflow instead - refactor sadd_overflow() and ssub_overflow() to use built-ins with automatic types, take ssize_t arguments, and deal with different ssize_t type widths - in 4/5: - switch l2len in tap_handler_passt() to uint32_t, as it really is unsigned and 32-bit wide - return if the length descriptor mismatches, instead of trying to proceed to the next frame - add 5/5 Stefano Brivio (5): conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH tcp_splice: Check return value of setsockopt() for SO_RCVLOWAT util, lineread, tap: Overflow checks on long signed sums and subtractions tap: Discard guest data on length descriptor mismatch conf: Use the right maximum buffer size for c->sock_path conf.c | 4 ++-- lineread.c | 5 +++-- tap.c | 31 +++++++++++++++++++---------- tcp_splice.c | 15 +++++++++----- util.c | 5 +++++ util.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 19 deletions(-) -- 2.43.0
Spotted by Coverity just recently. Not that it really matters as
MAXDNSRCH always appears to be defined as 1025, while a full domain
name can have up to 253 characters: it would be a bit pointless to
have a longer search domain.
Signed-off-by: Stefano Brivio
Spotted by Coverity, harmless as we would consider that successful
and check on the socket later from the timer, but printing a debug
message in that case is definitely wise, should it ever happen.
Signed-off-by: Stefano Brivio
Potential sum and subtraction overflows were reported by Coverity in a
few places where we use size_t and ssize_t types.
Strictly speaking, those are not false positives even if I don't see a
way to trigger those: for instance, if we receive up to n bytes from a
socket up, the return value from recv() is already constrained and
can't overflow for the usage we make in tap_handler_passt().
In any case, take care of those by adding two functions that
explicitly check for overflows in sums and subtractions of long signed
values, and using them.
Signed-off-by: Stefano Brivio
This was reported by Matej a while ago, but we forgot to fix it. Even
if the hypervisor is necessarily trusted by passt, as it can in any
case terminate the guest or disrupt guest connectivity, it's a good
idea to be robust against possible issues.
Instead of resetting the connection to the hypervisor, just discard
the data we read with a single recv(), as we had a few cases where
QEMU would get the length descriptor wrong, in the past.
While at it, change l2len in tap_handler_passt() to uint32_t, as the
length descriptor is logically unsigned and 32-bit wide.
Reported-by: Matej Hrica
UNIX_SOCK_MAX is the maximum number we'll append to the socket path
if we generate it automatically. If it's given on the command line,
it can be up to UNIX_PATH_MAX (including the terminating character)
long.
UNIX_SOCK_MAX happened to kind of fit because it's 100 (instead of
108).
Commit ceddcac74a6e ("conf, tap: False "Buffer not null terminated"
positives, CWE-170") fixed the wrong problem: the right fix for the
problem at hand was actually commit cc287af173ca ("conf: Fix
incorrect bounds checking for sock_path parameter").
Fixes: ceddcac74a6e ("conf, tap: False "Buffer not null terminated" positives, CWE-170")
Signed-off-by: Stefano Brivio
On Thu, Jun 27, 2024 at 10:46:41PM +0200, Stefano Brivio wrote:
UNIX_SOCK_MAX is the maximum number we'll append to the socket path if we generate it automatically. If it's given on the command line, it can be up to UNIX_PATH_MAX (including the terminating character) long.
UNIX_SOCK_MAX happened to kind of fit because it's 100 (instead of 108).
Commit ceddcac74a6e ("conf, tap: False "Buffer not null terminated" positives, CWE-170") fixed the wrong problem: the right fix for the problem at hand was actually commit cc287af173ca ("conf: Fix incorrect bounds checking for sock_path parameter").
Fixes: ceddcac74a6e ("conf, tap: False "Buffer not null terminated" positives, CWE-170") Signed-off-by: Stefano Brivio
Reviewed-by: David Gibson
--- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/conf.c b/conf.c index 9e47e9a..3c38ceb 100644 --- a/conf.c +++ b/conf.c @@ -1398,7 +1398,7 @@ void conf(struct ctx *c, int argc, char **argv) c->foreground = 1; break; case 's': - ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s", + ret = snprintf(c->sock_path, sizeof(c->sock_path), "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) die("Invalid socket path: %s", optarg);
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, 27 Jun 2024 22:46:36 +0200
Stefano Brivio
All harmless issues as far as I can tell, but nice to fix.
v2: - in 3/5: - keep 'skip' in write_remainder() unsigned, and check for unsigned overflow instead - refactor sadd_overflow() and ssub_overflow() to use built-ins with automatic types, take ssize_t arguments, and deal with different ssize_t type widths - in 4/5: - switch l2len in tap_handler_passt() to uint32_t, as it really is unsigned and 32-bit wide - return if the length descriptor mismatches, instead of trying to proceed to the next frame - add 5/5
Stefano Brivio (5): conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH tcp_splice: Check return value of setsockopt() for SO_RCVLOWAT util, lineread, tap: Overflow checks on long signed sums and subtractions tap: Discard guest data on length descriptor mismatch conf: Use the right maximum buffer size for c->sock_path
I applied 1/5, 2/5, and 5/5. I'll post a new version of 4/5 without the "fix" for the integer overflow false positive soon, and I'll leave 3/5 alone for the moment. -- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio