3.8.2 gcc 13 warnings -Wstringop-overflow, -Wdangling-pointer

[vszakats]

In function 'make_kn',
    inlined from 'make_kn' at ./libressl/crypto/cmac/cmac.c:80:1,
    inlined from 'CMAC_Init' at ./libressl/crypto/cmac/cmac.c:191:3:
./libressl/crypto/cmac/cmac.c:92:28: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
   92 |                 k1[bl - 1] ^= bl == 16 ? 0x87 : 0x1b;
      |                 ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
./libressl/crypto/cmac/cmac.c: In function 'CMAC_Init':
./libressl/crypto/cmac/cmac.c:66:23: note: at offset [-2147483649, -1] into destination object 'k1' of size 32
   66 |         unsigned char k1[EVP_MAX_BLOCK_LENGTH];
      |                       ^~

./libressl/crypto/compat/timegm.c: In function '__year_to_secs':
./libressl/crypto/compat/timegm.c:91:45: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
   91 |         leaps += 97*cycles + 24*centuries - *is_leap;
      |                                             ^~~~~~~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
   62 |         if (!is_leap) is_leap = &(int){0};
      |                                       ^
./libressl/crypto/compat/timegm.c:70:26: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
   70 |                 *is_leap = 1;
      |                 ~~~~~~~~~^~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
   62 |         if (!is_leap) is_leap = &(int){0};
      |                                       ^
./libressl/crypto/compat/timegm.c:82:34: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
   82 |                         *is_leap = 0;
      |                         ~~~~~~~~~^~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
   62 |         if (!is_leap) is_leap = &(int){0};
      |                                       ^
./libressl/crypto/compat/timegm.c:87:34: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
   87 |                         *is_leap = !rem;
      |                         ~~~~~~~~~^~~~~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
   62 |         if (!is_leap) is_leap = &(int){0};
      |                                       ^

[botovq]

In function 'make_kn',
    inlined from 'make_kn' at ./libressl/crypto/cmac/cmac.c:80:1,
    inlined from 'CMAC_Init' at ./libressl/crypto/cmac/cmac.c:191:3:
./libressl/crypto/cmac/cmac.c:92:28: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
   92 |                 k1[bl - 1] ^= bl == 16 ? 0x87 : 0x1b;
      |                 ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
./libressl/crypto/cmac/cmac.c: In function 'CMAC_Init':
./libressl/crypto/cmac/cmac.c:66:23: note: at offset [-2147483649, -1] into destination object 'k1' of size 32
   66 |         unsigned char k1[EVP_MAX_BLOCK_LENGTH];
      |                       ^~

This assumes that bl <= 0 which should never happen with a block cipher (and if you do CMAC with a non-block cipher you get what you asked for). But there are various issues in this code...

I wonder if the warning goes away if we do this, but I'm not sure if gcc is smart enough:

diff --git a/lbressl/crypto/cmac/cmac.c b/libressl/crypto/cmac/cmac.c
index 257bd21ccad..75930fd3cfe 100644
--- a/libressl/crypto/cmac/cmac.c
+++ b/libressl/crypto/cmac/cmac.c
@@ -186,7 +186,8 @@ CMAC_Init(CMAC_CTX *ctx, const void *key, size_t keylen,
 			return 0;
 		if (!EVP_EncryptInit_ex(&ctx->cctx, NULL, NULL, key, zero_iv))
 			return 0;
-		bl = EVP_CIPHER_CTX_block_size(&ctx->cctx);
+		if ((bl = EVP_CIPHER_CTX_block_size(&ctx->cctx)) <= 0)
+			return 0;
 		if (!EVP_Cipher(&ctx->cctx, ctx->tbl, zero_iv, bl))
 			return 0;
 		make_kn(ctx->k1, ctx->tbl, bl);

The second pile of warnings could be "solved" by deleting or commenting out this line:

portable/crypto/compat/timegm.c

Line 62 in e454895

if (!is_leap) is_leap = &(int){0};

However, it should be noted that __year_to_secs() is never called with is_leap == NULL so all the whining is irrelevant in the first place. Moreover, I'm pretty sure this is perfectly fine use of an unnamed temporary object, which should have static storage duration.

Also note that this is a copy of musl code.

[vszakats]

I tried both of your suggestions and they do make these warnings
disappear with gcc 13.

[ Luckily, I'm not building musl from source :) ]

[botovq]

Thanks for testing. Then I'll land a few fixes in the vicinity of that particular problem zone in cmac.c. I think it would be fine to comment out the line in the timegm.c code and explicitly say why it is done.

[bob-beck]

Do we even need timegm() anymore? I thought I got rid of all that?

…

On Nov 3, 2023, at 3:53 PM, Theo Buehler ***@***.***> wrote: Thanks for testing. Then I'll land a few fixes in the vicinity of that particular problem zone in cmac.c. I think it would be fine to comment out the line in the timegm.c code and explicitly say why it is done. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[botovq]

@bob-beck there is one call left in x509_verify_asn1_time_to_time_t() in x509_verify.c:

https://github.com/openbsd/src/blob/17e3ddc9933714bd053ce70afd6d575a774aee60/lib/libcrypto/x509/x509_verify.c#L78

[bob-beck]

Yeah, that’s silly, I can make that go away ;)

…

On Nov 3, 2023, at 4:10 PM, Theo Buehler ***@***.***> wrote: @bob-beck there is one call left in x509_verify_asn1_time_to_time_t() in x509_verify.c: https://github.com/openbsd/src/blob/17e3ddc9933714bd053ce70afd6d575a774aee60/lib/libcrypto/x509/x509_verify.c#L78 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[botovq]

Unfortunately there are a few more in libtls which are a bit more annoying to fix.

[vszakats]

With the timegm.c source gone with 8f03828 #1056, the second part of this issue seems resolved. (I haven't re-tested though)

[botovq]

The warning in the CMAC code should also be gone with openbsd/src@8865b67

[vszakats]

@botovq Great, thanks! I was looking, but in the wrong repo (https://github.com/libressl-portable/openbsd).

Closing this as resolved.

[botovq]

It's also there. I usually link to commits the openbsd/src repo since the commit hashes there are guaranteed to be stable. This is not the case for libressl/openbsd due to limitations in the conversion tool.

[vszakats]

Found it indeed: libressl/openbsd@0423d36
I have no excuses, not sure how I missed it!