Fix shorten-64-to-32 #743

pabuhler · 2025-01-21T22:31:10Z

This takes the changes proposed in #742 and adds a ci build as well as fixing these warnings for all backends.

…yptUpdate EVP_CIPHER_CTX_ctrl EVP_EncryptUpdate

Add the warning when using clang. This error is also set by default when build building ios so add a ci build.

Add checks before casting.

paulej · 2025-02-17T22:30:38Z

crypto/cipher/aes_gcm_ossl.c

@@ -258,12 +258,16 @@ static srtp_err_status_t srtp_aes_gcm_openssl_set_aad(void *cv,
    debug_print(srtp_mod_aes_gcm, "setting AAD: %s",
                srtp_octet_string_hex_string(aad, aad_len));

+    if (aad_len > INT_MAX) {


Might want to move this check up a few lines since it's printed above before it's checked here.

The check is only mean to protect from the casting, using it in the print statement as a szie_t should be no problem or ?

I just think that if the parameter is considered bad, we ought to return that error before using it. If aad_len is bad, then I would guess aad is also bad. The printed result might be wrong. I'm just overly cautious.

paulej · 2025-02-17T22:32:19Z

crypto/cipher/aes_gcm_ossl.c

@@ -300,10 +304,14 @@ static srtp_err_status_t srtp_aes_gcm_openssl_encrypt(void *cv,
        return srtp_err_status_buffer_small;
    }

+    if (src_len > INT_MAX) {


Might want to move this up a few lines, as there is a comparison against it just above before this check.

That comparison should be fine as they are all size_t, this is check was only to protect from casting.

Yeah, this will not break since it is the same type. I just prefer to check values before using them. But, your call.

paulej · 2025-02-17T22:32:50Z

crypto/cipher/aes_gcm_ossl.c

@@ -357,10 +365,15 @@ static srtp_err_status_t srtp_aes_gcm_openssl_decrypt(void *cv,
        return srtp_err_status_buffer_small;
    }

+    if (src_len > INT_MAX) {


Same here. Used a couple of times above before this check.

paulej · 2025-02-17T22:35:40Z

crypto/cipher/aes_gcm_wssl.c

@@ -298,10 +300,16 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_set_aad(void *cv,
    memcpy(c->aad + c->aad_size, aad, aad_len);
    c->aad_size += aad_len;
 #else
+    if (aad_len > INT_MAX) {


Seems like this should be moved just above the log statement, as I assume this should run regardless of the conditional compilation statements.

paulej · 2025-02-17T22:36:35Z

crypto/cipher/aes_gcm_wssl.c

@@ -338,6 +346,10 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_encrypt(void *cv,
        return srtp_err_status_buffer_small;
    }

+    if (src_len > INT_MAX) {


This one should be moved up a bit, since the value is compared just before this check.

paulej · 2025-02-18T00:12:19Z

crypto/hash/hmac_nss.c

@@ -247,7 +259,12 @@ static srtp_err_status_t srtp_hmac_compute(void *statev,
        return srtp_err_status_bad_param;
    }

-    if (PK11_DigestOp(hmac->ctx, message, msg_octets) != SECSuccess) {
+    if (msg_octets > UINT_MAX) {


You might want to move this above the debug print above that uses msg_octets.

paulej · 2025-02-18T00:13:47Z

crypto/hash/hmac_ossl.c

@@ -240,7 +240,11 @@ static srtp_err_status_t srtp_hmac_init(void *statev,
        return srtp_err_status_auth_fail;
    }
 #else
-    if (HMAC_Init_ex(hmac->ctx, key, key_len, EVP_sha1(), NULL) == 0) {
+    if (key_len > INT_MAX) {


Should this be moved above the #ifdef, since it is used in that code, too (line 238).

The keyLen param of EVP_MAC_init is of type size_t so cast and check is not required.

It's ok if the #ifdef SRTP_OSSL_USE_EVP_MAC block if this key_len > INT_MAX? If so, fine. Seems like it would matter both places.

paulej · 2025-02-18T00:14:50Z

crypto/hash/hmac_wssl.c

@@ -154,7 +159,11 @@ static srtp_err_status_t srtp_hmac_wolfssl_update(void *statev,
    debug_print(srtp_mod_hmac, "input: %s",
                srtp_octet_string_hex_string(message, msg_octets));

-    err = wc_HmacUpdate(state, message, msg_octets);
+    if (msg_octets > INT_MAX) {


You might want to move this above the debug print above that uses msg_octets.

paulej · 2025-02-18T00:15:06Z

crypto/hash/hmac_wssl.c

@@ -182,8 +191,12 @@ static srtp_err_status_t srtp_hmac_wolfssl_compute(void *statev,
        return srtp_err_status_bad_param;
    }

+    if (msg_octets > INT_MAX) {


You might want to move this above the debug print above that uses msg_octets.

paulej · 2025-02-18T00:16:39Z

srtp/srtp.c

@@ -850,10 +850,14 @@ static srtp_err_status_t srtp_kdf_generate(srtp_kdf_t *kdf,
    }
    octet_string_set_to_zero(key, length);

+    if (length > INT_MAX) {


Could add this to line 845, which checks for a zero length.

It could but it is here only to verify it wont be shortened when calling wc_SRTP_KDF_label() not if the value is other wise valid.

Same issue. No problem if you don't mind printing a wrong value (and it doesn't cause issues)

Pierre Bodilis and others added 5 commits January 21, 2025 15:55

[code] fix size_t to int cast to comply to openssl signature EVP_Decr…

a06b7ed

…yptUpdate EVP_CIPHER_CTX_ctrl EVP_EncryptUpdate

[code] fix clang reformat

3b9af4b

add shorten-64-to-32 warning

16a4835

Add the warning when using clang. This error is also set by default when build building ios so add a ci build.

Merge commit '3b9af4be743fdc7d3e1dc35da6dacd98c92357fa'

ebfb916

fix remaining shorten-64-to-32 warnings

6e6c1dc

Add checks before casting.

pabuhler mentioned this pull request Jan 21, 2025

fix size_t to int cast to comply to openssl signatures #742

Merged

paulej suggested changes Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shorten-64-to-32 #743

Fix shorten-64-to-32 #743

pabuhler commented Jan 21, 2025

paulej Feb 17, 2025

pabuhler Feb 20, 2025

paulej Feb 20, 2025

paulej Feb 17, 2025

pabuhler Feb 20, 2025

paulej Feb 20, 2025

paulej Feb 17, 2025

paulej Feb 17, 2025

paulej Feb 17, 2025

paulej Feb 18, 2025

paulej Feb 18, 2025

pabuhler Feb 20, 2025

paulej Feb 20, 2025

paulej Feb 18, 2025

paulej Feb 18, 2025

paulej Feb 18, 2025

pabuhler Feb 20, 2025

paulej Feb 20, 2025

Fix shorten-64-to-32 #743

Are you sure you want to change the base?

Fix shorten-64-to-32 #743

Conversation

pabuhler commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment