From: Theo Buehler <tb@theobuehler.org>
Subject: Re: [PATCH] LibreSSL: check for NULL data pointer in bio_mem_read_ptr()
To: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
Cc: tech@openbsd.org, jsing@openbsd.org
Date: Sat, 3 May 2025 10:49:28 +0200

On Sun, Apr 13, 2025 at 11:49:24AM +0200, Theo Buehler wrote:
> On Sat, Apr 12, 2025 at 12:15:42PM +0900, Kenjiro Nakayama wrote:
> > When bm->buf->data is NULL, calling bio_mem_read_ptr() triggers a runtime
> > error under UndefinedBehaviorSanitizer:
> 
> What version are you using? I couldn't reproduce with gcc 14 and clang 19.
> 
> > 
> >     $ ./tests/bio_dump
> >     /dev/portable/crypto/bio/bss_mem.c:87:10: runtime error: applying zero offset to null pointer
> >     SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /dev/portable/crypto/bio/bss_mem.c:87:10 in
> > 
> > This patch adds an explicit NULL check to avoid applying an offset
> > to a NULL pointer, which is undefined behavior. The function now
> > safely returns NULL if the buffer is uninitialized.
> 
> I think bio_mem_read_ptr() should only be called if we know that there's
> data available for reading. This is checked to be the case in all callers
> with one exception.
> 
> This exception is triggered by the corner case I added to the bio_dump
> test to check the output when no data is dumped. In this case the mem
> bio does not set up a data buffer because nothing is written to it and
> therefore the data pointer is NULL.
> 
> I'm confused why your fix is enough. It seems ubsan doesn't complain
> about the strncmp(..., NULL, 0) that then follows, which I'm pretty
> sure is also UB.
> 
> Anyway, while your fix would be one way of doing it, it also adds this
> check when it isn't needed. I think it is better to do something like
> this instead: to my mind the check for ->max is the most sensible one
> here, but we could also check directly for ->data != NULL.
> 

I'd like to commit this diff.

Index: lib/libcrypto/bio/bss_mem.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/bio/bss_mem.c,v
diff -u -p -r1.22 bss_mem.c
--- lib/libcrypto/bio/bss_mem.c	5 Jul 2023 21:23:37 -0000	1.22
+++ lib/libcrypto/bio/bss_mem.c	12 Apr 2025 17:05:15 -0000
@@ -283,7 +283,9 @@ mem_ctrl(BIO *bio, int cmd, long num, vo
 	case BIO_CTRL_INFO:
 		if (ptr != NULL) {
 			pptr = (void **)ptr;
-			*pptr = bio_mem_read_ptr(bm);
+			*pptr = NULL;
+			if (bm->buf->max > 0)
+				*pptr = bio_mem_read_ptr(bm);
 		}
 		ret = (long)bio_mem_pending(bm);
 		break;
Index: regress/lib/libcrypto/bio/bio_dump.c
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/bio/bio_dump.c,v
diff -u -p -r1.4 bio_dump.c
--- regress/lib/libcrypto/bio/bio_dump.c	9 Feb 2024 12:48:32 -0000	1.4
+++ regress/lib/libcrypto/bio/bio_dump.c	13 Apr 2025 09:35:54 -0000
@@ -809,7 +809,7 @@ bio_dump_test(const struct bio_dump_test
 		    tc->indent, ret, got_len, strlen(tc->output));
 		goto err;
 	}
-	if (strncmp(tc->output, got, got_len) != 0) {
+	if (got_len > 0 && strncmp(tc->output, got, got_len) != 0) {
 		fprintf(stderr, "%d: mismatch\n", tc->indent);
 		goto err;
 	}