From: Nakayama Kenjiro Subject: Re: [PATCH] LibreSSL: check for NULL data pointer in bio_mem_read_ptr() To: Theo Buehler Cc: tech@openbsd.org, jsing@openbsd.org Date: Mon, 14 Apr 2025 08:35:29 +0900 > > 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. I added "-fsanitize=undefined" when testing. Sorry I haven't explained that. With the “-fsanitize=undefined” option enabled, I'm using GCC 14 and Clang 16. And both were able to reproduce the issue. I demonstrated the error using my own github repository below. https://github.com/nak3/portable/pull/1 The error occurs in bio_mem. (Although "make test" doesn’t show the detailed error message...) > 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. Yes, exactly. I confirmed that it only occurs in the first test case (no data). > 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. You’re absolutely right - it was also strange to me. With that said, I guessed that strncmp(*str1, *str2, num) might be implemented in such a way that it doesn't access the pointer when the third argument (=num) is zero. Or, it's possible that the sanitizer doesn't detect issues inside the libc implementation. > 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. > > 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; > } Thank you. You’re right. My fix does add the check even when it isn't needed. I reviewed your fix as well, and I think it’s a good solution. I also confirmed that it resolves the sanitizer error in my environment. Regards, Kenjiro On Sun, Apr 13, 2025 at 6:49 PM 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. > > 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; > } > -- Kenjiro NAKAYAMA GPG Key fingerprint = ED8F 049D E67A 727D 9A44 8E25 F44B E208 C946 5EB9