Gentoo QA Notice for libressl-3.7.1

Question

Gentoo QA Notice for libressl-3.7.1

orbea opened this issue 2 years ago · 10 comments

On Gentoo when building libressl-3.7.1 with gcc-12.2.1_p20230121-r1 this warning is emitted .

QA Notice: Package triggers severe warnings which indicate that it may exhibit random runtime failures.

/var/tmp/portage/dev-libs/libressl-3.7.1/work/libressl-3.7.1/ssl/s3_cbc.c:529:9: warning: 'memcpy' forming offset 128 is out of the bounds [0, 128] of object 'hmac_pad' with type 'unsigned char[128]' [-Warray-bounds]

And the whole warning can be seen:

/var/tmp/portage/dev-libs/libressl-3.7.1/work/libressl-3.7.1/ssl/s3_cbc.c: In function 'ssl3_cbc_digest_record':
/var/tmp/portage/dev-libs/libressl-3.7.1/work/libressl-3.7.1/ssl/s3_cbc.c:529:9: warning: 'memcpy' forming offset 128 is out of the bounds [0, 128] of object 'hmac_pad' with type 'unsigned char[128]' [-Warray-bounds]
  529 |         memcpy(hmac_pad, mac_secret, mac_secret_length);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/var/tmp/portage/dev-libs/libressl-3.7.1/work/libressl-3.7.1/ssl/s3_cbc.c:403:23: note: 'hmac_pad' declared here
  403 |         unsigned char hmac_pad[MAX_HASH_BLOCK_SIZE];
      |                       ^~~~~~~~

Answer 1 · 2023-03-16T19:19:26.000Z

I've seen this warning and it didn't make any sense to me: ``` 528 OPENSSL_assert(mac_secret_length <= sizeof(hmac_pad)); 529 memcpy(hmac_pad, mac_secret, mac_secret_length); ``` So the line before asserts that there is enough room to copy mac_secret_length bytes into hmac_pad.

Answer 2 · 2023-04-28T21:34:59.000Z

The issue is that gcc can't see that OPENSSL_assert() will not return because it is in a different library. This patch avoids the warning: bc0a702

Answer 3 · 2023-04-30T02:55:08.000Z

Thanks that patch silences the compiler warning and QA notice, wouldn't it be better to be explicit with the noreturn attribute to make gcc (And Gentoo's QA) happy? Or is there some reason to not merge it? It might seem silly, but is it actually problematic?

Answer 4 · 2023-04-30T04:07:28.000Z

I only made the PR as a patch for portable to ensure that all platforms in CI are happy with it. Once I knew that was the case, I merged it directly into our main source tree: openbsd/src@a988a9f

Answer 5 · 2023-04-30T13:26:22.000Z

Oh, thanks for explaining, I didn't catch that earlier. I suppose this issue can be closed when the new release is out?

Answer 6 · 2023-04-30T16:39:39.000Z

Actually there is an issue with this approach, libvncserver-0.9.14 now fails.

samu: job failed: /usr/lib/ccache/bin/x86_64-gentoo-linux-musl-gcc -DLIBVNCSERVER_HAVE_LIBJPEG -DLIBVNCSERVER_HAVE_LIBPNG -DLIBVNCSERVER_HAVE_LIBZ -DLIBVNCSERVER_WITH_WEBSOCKETS -Dvncserver_EXPORTS -I/var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14 -I/var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14_build -I/var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14/libvncserver -I/var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14/common  -O2 -pipe -Werror=implicit-function-declaration -Werror=implicit-int -fPIC -std=gnu90 -MD -MT CMakeFiles/vncserver.dir/libvncserver/rfbssl_openssl.c.o -MF CMakeFiles/vncserver.dir/libvncserver/rfbssl_openssl.c.o.d -o CMakeFiles/vncserver.dir/libvncserver/rfbssl_openssl.c.o -c /var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14/libvncserver/rfbssl_openssl.c
In file included from /usr/include/openssl/bio.h:69,
                 from /usr/include/openssl/evp.h:67,
                 from /usr/include/openssl/hmac.h:67,
                 from /usr/include/openssl/ssl.h:150,
                 from /var/tmp/portage/net-libs/libvncserver-0.9.14/work/libvncserver-LibVNCServer-0.9.14/libvncserver/rfbssl_openssl.c:25:
/usr/include/openssl/crypto.h:491:7: error: expected ';' before 'void'
  491 | __dead void OpenSSLDie(const char *file, int line, const char *assertion);
      |       ^~~~~
      |       ;
samu: subcommand failed
 * ERROR: net-libs/libvncserver-0.9.14::gentoo failed (compile phase)

Answer 7 · 2023-04-30T16:44:22.000Z

Does it compile if you add '#include <sys/types.h>' at the top of crypto.h?

Answer 8 · 2023-04-30T17:06:37.000Z

Judging from the linux systems I currently have access to that won't work. We could add #ifndef __dead #define __dead #endif to crypto.h, but this goes a step too far in my opinion. We have identified this QA Notice as a non-issue in more than one way. I'd be inclined to ignore it.

Answer 9 · 2023-04-30T17:19:10.000Z

Yea, I can confirm that didn't work while your second suggestion does.

diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h
index 066f624..98a2b06 100644
--- a/include/openssl/crypto.h
+++ b/include/openssl/crypto.h
@@ -132,6 +132,10 @@
 extern "C" {
 #endif
 
+#ifndef __dead
+#define __dead
+#endif
+
 /* Backward compatibility to SSLeay */
 /* This is more to be used to check the correct DLL is being used
  * in the MS world. */
@@ -488,7 +492,7 @@ typedef int *CRYPTO_MEM_LEAK_CB(unsigned long, const char *, int, int, void *);
 int CRYPTO_mem_leaks_cb(CRYPTO_MEM_LEAK_CB *cb);
 
 /* die if we have to */
-void OpenSSLDie(const char *file, int line, const char *assertion);
+__dead void OpenSSLDie(const char *file, int line, const char *assertion);
 #define OPENSSL_assert(e)       (void)((e) ? 0 : (OpenSSLDie(__FILE__, __LINE__, #e),1))
 
 uint64_t OPENSSL_cpu_caps(void);

However I share your sentiment that it seems a bit silly, but I am not sure if its bad enough to not silence warnings even if they are false positives? I'll leave it the best judgement of the libressl developers, feel free to close this issue if you feel that is best.

Answer 10 · 2023-07-07T15:21:38.000Z

Fixed by libressl/openbsd@27333fb