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:54:31 +0900 > The error occurs in bio_mem. (Although "make test" doesn’t show the detailed error message...) Just to clarify. I meant bio_dump test - "FAIL: bio_dump". On Mon, Apr 14, 2025 at 8:35 AM Nakayama Kenjiro 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. > > 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 > -- Kenjiro NAKAYAMA GPG Key fingerprint = ED8F 049D E67A 727D 9A44 8E25 F44B E208 C946 5EB9