Added check for the return value of the RAND_bytes() function #21706

Klavishnik · 2023-08-10T10:57:22Z

The return value of the RAND_bytes() function is not checked. At the same time, the function might return 0 without changing the value of the out variable passed to it. This variable will be used later on, so a check for the function's return value has been added.

Perhaps this would be more correct:

  if (RAND_bytes(out, 16) <= 0)
        goto err;

t8m · 2023-08-10T11:37:16Z

apps/speed.c

+                if (RAND_bytes(out, 16) > 0) {
+                    len += 16;
+                    aad[11] = (unsigned char)(len >> 8);
+                    aad[12] = (unsigned char)(len);
+                    pad = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_TLS1_AAD,
+                                              EVP_AEAD_TLS1_AAD_LEN, aad);
+                    ciph_success = EVP_Cipher(ctx, out, inp, len + pad);
+                }


Instead, I would use app_bail_out() in case of RAND_bytes() failure. However it is not clear to me why the RAND_bytes() call is there at all - the out buffer should be overwritten by the EVP_Cipher call anyway.

@mattcaswell any idea why the RAND_bytes() call is there?

No, I don't see an obvious reason for it.

In that case I wouldn't bother checking the return value, just explicitly cast to (void) as we do not want to change what is the speed command measuring to keep the results stable.

Hmm, I wonder if it's meant to be setting inp rather than out, since - unless OPENSSL_malloc() initialises the memory it returns - it doesn't look like inp is being initialised here

What should we do about this issue?

Use app_bail_out if RAND_bytes() fails instead. And also change the output parameter of RAND_bytes() to inp.

Changed the output parameter of RAND_bytes() to inp.

paulidale · 2023-10-05T20:30:51Z

apps/speed.c

                len += 16;
                aad[11] = (unsigned char)(len >> 8);
                aad[12] = (unsigned char)(len);
                pad = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_TLS1_AAD,
-                                          EVP_AEAD_TLS1_AAD_LEN, aad);
+                                            EVP_AEAD_TLS1_AAD_LEN, aad);


Please don't fix the indentation here, it was already correct as per our coding style.

openssl-machine · 2023-10-10T11:00:22Z

This pull request is ready to merge

t8m · 2023-10-10T15:19:19Z

Squashed and merged to master, 3.1 and 3.0 branches. Thank you for your contribution.

Call app_bail_out if RAND_bytes() fails. Also changed the output parameter of RAND_bytes() to inp as writing to encrypted output buffer does not make sense. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21706) (cherry picked from commit 8d120ae)

Call app_bail_out if RAND_bytes() fails. Also changed the output parameter of RAND_bytes() to inp as writing to encrypted output buffer does not make sense. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21706)

Call app_bail_out if RAND_bytes() fails. Also changed the output parameter of RAND_bytes() to inp as writing to encrypted output buffer does not make sense. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21706) (cherry picked from commit 8d120ae)

Call app_bail_out if RAND_bytes() fails. Also changed the output parameter of RAND_bytes() to inp as writing to encrypted output buffer does not make sense. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21706) Signed-off-by: fly2x <fly2x@hitls.org>

Call app_bail_out if RAND_bytes() fails. Also changed the output parameter of RAND_bytes() to inp as writing to encrypted output buffer does not make sense. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21706) (cherry picked from commit 8d120ae) Signed-off-by: fly2x <fly2x@hitls.org>

Klavishnik added 2 commits August 10, 2023 13:56

Added check for the return value of the RAND_bytes() function

39e4e9e

Code formatting has been corrected

c15e77c

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

t8m reviewed Aug 10, 2023

View reviewed changes

t8m added branch: master Merge to master branch triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly labels Aug 10, 2023

paulidale added the stalled: awaiting contributor response This pull request is awaiting a response by the contributor label Oct 4, 2023

Added app_bail_out if RAND_bytes() fails instead.

0b33db6

Changed the output parameter of RAND_bytes() to inp.

paulidale removed the stalled: awaiting contributor response This pull request is awaiting a response by the contributor label Oct 5, 2023

paulidale reviewed Oct 5, 2023

View reviewed changes

github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Oct 5, 2023

t8m added the tests: exempted The PR is exempt from requirements for testing label Oct 6, 2023

Correction of code formatting

14b74b6

t8m approved these changes Oct 9, 2023

View reviewed changes

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 approval: review pending This pull request needs review by a committer labels Oct 9, 2023

mattcaswell approved these changes Oct 9, 2023

View reviewed changes

tom-cosgrove-arm approved these changes Oct 9, 2023

View reviewed changes

tom-cosgrove-arm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 9, 2023

paulidale approved these changes Oct 9, 2023

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 10, 2023

t8m closed this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added check for the return value of the RAND_bytes() function #21706

Added check for the return value of the RAND_bytes() function #21706

Klavishnik commented Aug 10, 2023

t8m Aug 10, 2023 •

edited

mattcaswell Aug 10, 2023

t8m Aug 10, 2023

tom-cosgrove-arm Aug 11, 2023

t8m Aug 14, 2023

Klavishnik Oct 3, 2023

t8m Oct 3, 2023

paulidale Oct 5, 2023

openssl-machine commented Oct 10, 2023

t8m commented Oct 10, 2023

Added check for the return value of the RAND_bytes() function #21706

Added check for the return value of the RAND_bytes() function #21706

Conversation

Klavishnik commented Aug 10, 2023

t8m Aug 10, 2023 • edited

Choose a reason for hiding this comment

mattcaswell Aug 10, 2023

Choose a reason for hiding this comment

t8m Aug 10, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm Aug 11, 2023

Choose a reason for hiding this comment

t8m Aug 14, 2023

Choose a reason for hiding this comment

Klavishnik Oct 3, 2023

Choose a reason for hiding this comment

t8m Oct 3, 2023

Choose a reason for hiding this comment

paulidale Oct 5, 2023

Choose a reason for hiding this comment

openssl-machine commented Oct 10, 2023

t8m commented Oct 10, 2023

t8m Aug 10, 2023 •

edited