Fix EVP_CIPHER_CTX_get_block_size() calls to check for -1. #23000

slontis · 2023-12-10T21:41:11Z

EVP_CIPHER_CTX_get_block_size() returns an int which can be -1.
Fix all the calls that cast it to a size_t.

This is a follow on from reviewing #22995
which makes it more obvious that EVP_CIPHER_CTX_get_block_size() can return -1.

There are a couple of places in the code that already check for this, but the rest do bad things.

Checklist

documentation is added or updated
tests are added or updated

Fix all the calls that assume it returns a size_t.

slontis · 2023-12-10T21:41:22Z

Needs tests

…. Fix all the calls that assume it returns a size_t.

t-j-h · 2023-12-10T22:55:10Z

Nice to see this being tackled - I too saw that we would have some "fun" with this issue where we have had size_t logic get put in a few places incorrectly.

mattcaswell

makes it more obvious that EVP_CIPHER_CTX_get_block_size() can return -1.

It's not currently clear to me why #22995 is making that change so that EVP_CIPHER_CTX_get_block_size() can suddenly return -1, where it doesn't seem to have done so before. This seems like a significant change in behaviour.

If we're assuming that the return value of this function can be safely cast to a size_t, then I'd say its a fair bet that applications are making the same assumption.

mattcaswell · 2023-12-11T08:39:50Z

ssl/record/methods/dtls_meth.c

-        blocksize = EVP_CIPHER_CTX_get_block_size(rl->enc_ctx);
+        (EVP_CIPHER_CTX_get_mode(rl->enc_ctx) == EVP_CIPH_CBC_MODE)) {
+        blksz = EVP_CIPHER_CTX_get_block_size(rl->enc_ctx);
+        assert(blksz >= 0);


Shouldn't we instead propagate this error to the caller (there is only one)?

Also a block size of 0 is really an error. Stream ciphers have block size of 1.

Updated - the propagation required a few more changes

nhorman · 2023-12-11T13:47:43Z

@mattcaswell this is sort of an interesting question - The documentation has never been clear on the return value of this call, save to say it returns the block size of the cipher. Given that the function is typed to return an int, it seems that a negative return code is feasible. Whats more, as @slontis has corrected here, we have a myriad of call points that treat the return code differently. Currently, our code base has 24 calls to EVP_CIPHER_CTX_get_block_size:

2 cmac.c       CMAC_CTX_copy                   98 if ((bl = EVP_CIPHER_CTX_get_block_size(in->cctx)) < 0)
3 cmac.c       CMAC_Init                      122 memset(ctx->tbl, 0, EVP_CIPHER_CTX_get_block_size(ctx->cctx));
4 cmac.c       CMAC_Init                      145 if ((bl = EVP_CIPHER_CTX_get_block_size(ctx->cctx)) < 0)
5 cmac.c       CMAC_Update                    173 if ((bl = EVP_CIPHER_CTX_get_block_size(ctx->cctx)) < 0)
6 cmac.c       CMAC_Final                     237 if ((bl = EVP_CIPHER_CTX_get_block_size(ctx->cctx)) < 0)
7 cms_pwri.c   kek_unwrap_key                 204 size_t blocklen = EVP_CIPHER_CTX_get_block_size(ctx);
8 cms_pwri.c   kek_wrap_key                   257 size_t blocklen = EVP_CIPHER_CTX_get_block_size(ctx);
9 bio_enc.c    enc_read                       134 blocksize = EVP_CIPHER_CTX_get_block_size(ctx->cipher);
a e_aes.c      aesni_ecb_cipher               193 size_t bl = EVP_CIPHER_CTX_get_block_size(ctx);
b e_aes.c      aes_ecb_cipher                2530 size_t bl = EVP_CIPHER_CTX_get_block_size(ctx);
c e_camellia.c camellia_ecb_cipher            239 size_t bl = EVP_CIPHER_CTX_get_block_size(ctx);
d e_sm4.c      sm4_ecb_cipher                 172 size_t bl = EVP_CIPHER_CTX_get_block_size(ctx);
e evp_enc.c    EVP_EncryptFinal_ex            747 blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
f evp_enc.c    EVP_DecryptUpdate              834 blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
g evp_enc.c    EVP_DecryptFinal_ex            969 blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
h evp_lib.c    EVP_CIPHER_CTX_get_block_size  393 int EVP_CIPHER_CTX_get_block_size(const EVP_CIPHER_CTX *ctx)
i evp_lib.c    EVP_Cipher                     416 size_t blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
j p12_decr.c   PKCS12_pbe_crypt_ex             46 max_out_len = inlen + EVP_CIPHER_CTX_get_block_size(ctx);
k evp.h        EVP_CIPHER_CTX_block_size      620 #define EVP_CIPHER_CTX_block_size EVP_CIPHER_CTX_get_block_size
 0 krb5kdf.c    KRB5KDF                        417 blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
1 cmac_prov.c  cmac_size                      109 return EVP_CIPHER_CTX_get_block_size(cipherctx);
2 dtls_meth.c  dtls_get_max_record_overhead   749 blocksize = EVP_CIPHER_CTX_get_block_size(rl->enc_ctx);
3 ssl3_meth.c  ssl3_cipher                    116 bs = EVP_CIPHER_CTX_get_block_size(ds);
4 evp.h        EVP_CIPHER_CTX_block_size      615 #define EVP_CIPHER_CTX_block_size EVP_CIPHER_CTX_get_block_size

a) Some, like CMAC_CTX_copy check for a return values expressly less than 0 (treating a 0 return as valid)
b) Some, like CMAC_Init assume a >= 0 return code, and use it in a nested function call
c) some, like kek_unwrap_key assume a >= zero return and automatically cast the return to a size_t, which will lead to all sorts of odd behavior on negative return values

I think in my head, what should be done boils down to

weather or not a block_size of zero is ever valid (based on @t8m statement). If that is true (and can reasonably never be expected to be zero).
weather there is value in differentiating an error that indicates a block size has been (inappropriately) set to zero, from an error that indicates the cipher block size is unknown (i.e. because a cipher hasn't been initalized)

If (1) is true and (2) is false, then we can safely just use 0 as an error return from this function, and fixup (a) and (b) and (c) above to coform to that. If that is the case then we should also consider modifying the EVP_CIPHER_CTX_get_block_size and EVP_CIPHER_get_block size to return a size_t or unsigned int accordingly.

t8m · 2023-12-11T19:43:57Z

EVP_CIPHER_CTX_get_block_size() could already return -1 with the pre-existing code so yeah, this is a correct bug fix.

…. Fix all the calls that assume it returns a size_t.

slontis · 2023-12-13T05:13:44Z

Added some tests for the ones that I was able to..

…. Fix all the calls that assume it returns a size_t.

t8m · 2023-12-13T09:17:13Z

crypto/cms/cms_pwri.c

+    if (blocksz <= 0 || inlen % blocklen) {
        /* Invalid size */
        return 0;
    }


I would also put this if before the previous one. Funnily enough this newly added condition actually will never be evaluated for blocksz being negative as the blocklen value in the previous if will be a huge number and thus the condition inlen < 2 * blocklen will be true.

t8m · 2023-12-13T09:18:09Z

crypto/evp/bio_enc.c

@@ -132,6 +132,8 @@ static int enc_read(BIO *b, char *out, int outl)
    }

    blocksize = EVP_CIPHER_CTX_get_block_size(ctx->cipher);
+    if (blocksize < 0)


This should be <= as the 0 return is an error as well.

t8m · 2023-12-13T09:22:30Z

crypto/evp/evp_lib.c

@@ -413,8 +413,10 @@ int EVP_Cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
         */
        int ret = -1;
        size_t outl = 0;
-        size_t blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
+        int blocksize = EVP_CIPHER_CTX_get_block_size(ctx);


This is OK, however below in those ccipher/cupdate/cfinal calls you need to cast blocksize to size_t explicitly before adding inl as otherwise there can be an overflow. Rather theoretical one but...

t8m · 2023-12-13T09:22:59Z

crypto/pkcs12/p12_decr.c

@@ -37,13 +37,16 @@ unsigned char *PKCS12_pbe_crypt_ex(const X509_ALGOR *algor,
                               algor->parameter, ctx, en_de, libctx, propq))
        goto err;

+    blksz = EVP_CIPHER_CTX_get_block_size(ctx);
+    if (blksz < 0)


<= as 0 is also invalid.

t8m · 2023-12-13T09:23:30Z

providers/implementations/kdfs/krb5kdf.c

-    /* Initialize input block */
-    blocksize = EVP_CIPHER_CTX_get_block_size(ctx);
+    blksz = EVP_CIPHER_CTX_get_block_size(ctx);
+    if (blksz < 0)


openssl-machine · 2024-01-13T00:09:31Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

openssl-machine · 2024-02-13T00:09:22Z

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

openssl-machine · 2024-03-15T00:09:04Z

This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago

openssl-machine · 2024-04-15T00:09:16Z

This PR is in a state where it requires action by @openssl/otc but the last update was 123 days ago

openssl-machine · 2024-05-16T00:09:06Z

This PR is in a state where it requires action by @openssl/otc but the last update was 154 days ago

EVP_CIPHER_CTX_get_block_size() returns an int which can be -1.

1d247b7

Fix all the calls that assume it returns a size_t.

slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member hold: needs tests The PR needs tests to be added to it labels Dec 10, 2023

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Dec 10, 2023

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

e4111dc

…. Fix all the calls that assume it returns a size_t.

mattcaswell requested changes Dec 11, 2023

View reviewed changes

mattcaswell mentioned this pull request Dec 11, 2023

Add appropriate NULL checks in EVP_CIPHER api #22995

Closed

1 task

t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 labels Dec 11, 2023

nhorman self-requested a review December 12, 2023 13:08

slontis added 2 commits December 13, 2023 15:10

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

7e6b2b9

…. Fix all the calls that assume it returns a size_t.

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

641b145

…. Fix all the calls that assume it returns a size_t.

slontis added tests: present The PR has suitable tests present and removed hold: needs tests The PR needs tests to be added to it labels Dec 13, 2023

slontis added 3 commits December 13, 2023 15:24

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

c192d67

…. Fix all the calls that assume it returns a size_t.

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

7699a87

…. Fix all the calls that assume it returns a size_t.

fixup! EVP_CIPHER_CTX_get_block_size() returns an int which can be -1…

d4f4985

…. Fix all the calls that assume it returns a size_t.

t8m requested changes Dec 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EVP_CIPHER_CTX_get_block_size() calls to check for -1. #23000

Fix EVP_CIPHER_CTX_get_block_size() calls to check for -1. #23000

slontis commented Dec 10, 2023 •

edited

slontis commented Dec 10, 2023

t-j-h commented Dec 10, 2023

mattcaswell left a comment

mattcaswell Dec 11, 2023

t8m Dec 11, 2023

slontis Dec 13, 2023

nhorman commented Dec 11, 2023

t8m commented Dec 11, 2023

slontis commented Dec 13, 2023

t8m Dec 13, 2023

t8m Dec 13, 2023

t8m Dec 13, 2023

t8m Dec 13, 2023

t8m Dec 13, 2023

openssl-machine commented Jan 13, 2024

openssl-machine commented Feb 13, 2024

openssl-machine commented Mar 15, 2024

openssl-machine commented Apr 15, 2024

openssl-machine commented May 16, 2024

Fix EVP_CIPHER_CTX_get_block_size() calls to check for -1. #23000

Are you sure you want to change the base?

Fix EVP_CIPHER_CTX_get_block_size() calls to check for -1. #23000

Conversation

slontis commented Dec 10, 2023 • edited

Checklist

slontis commented Dec 10, 2023

t-j-h commented Dec 10, 2023

mattcaswell left a 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

nhorman commented Dec 11, 2023

t8m commented Dec 11, 2023

slontis commented Dec 13, 2023

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

openssl-machine commented Jan 13, 2024

openssl-machine commented Feb 13, 2024

openssl-machine commented Mar 15, 2024

openssl-machine commented Apr 15, 2024

openssl-machine commented May 16, 2024

slontis commented Dec 10, 2023 •

edited