Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix using of uninitialized buffer "t" #2942

Open
wants to merge 2 commits into
base: mbedtls-2.16
from

Conversation

@svpcom
Copy link

svpcom commented Nov 29, 2019

Status

READY

Requires Backporting

Yes

Which branch?
All branches

Migrations

NO

Additional comments

Security fix

@AndrzejKurek AndrzejKurek self-assigned this Nov 29, 2019
@AndrzejKurek AndrzejKurek self-requested a review Nov 29, 2019
Copy link
Contributor

AndrzejKurek left a comment

Hello, @svpcom, and thanks for the contribution!
Unfortunately, bzero that you've used is Unix-specific, and we're targeting multiple other platforms. You could consider using mbedtls_platform_zeroize instead, but could you please elaborate first about the details of this fix?
When exactly is t used as an uninitialized buffer? What are the preconditions?
Thanks in advance.

@svpcom

This comment has been minimized.

Copy link
Author

svpcom commented Nov 29, 2019

In mbedtls_hkdf_expand there are unsigned char t[MBEDTLS_MD_MAX_SIZE];
in loop:

/*
     * Compute T = T(1) | T(2) | T(3) | ... | T(N)
     * Where T(N) is defined in RFC 5869 Section 2.3
     */
    for( i = 1; i <= n; i++ )
    {

HMAC read data from uninitialized t at the first iteration of loop

ret = mbedtls_md_hmac_update( &ctx, t, t_len );

So T(1) can be undefined if compiler doesn't zeroize t

Copy link
Collaborator

Patater left a comment

Could you please add a regression test? We test with ASan and would expect a failure there if we provide the right test case.

@@ -93,7 +93,7 @@ int mbedtls_hkdf_expand( const mbedtls_md_info_t *md, const unsigned char *prk,
int ret = 0;
mbedtls_md_context_t ctx;
unsigned char t[MBEDTLS_MD_MAX_SIZE];
bzero(t, sizeof(t));
mbedtls_platform_zeroize(t, sizeof(t));

This comment has been minimized.

Copy link
@Patater

Patater Nov 29, 2019

Collaborator

memset() should be fine here, since t doesn't contain secrets at this point.

@Patater

This comment has been minimized.

Copy link
Collaborator

Patater commented Nov 29, 2019

For the mbedtls-2.16 and mbedtls-2.7 branches, we need a contributor license agreement (CLA) in order to take onboard your fix.

If this is a personal contribution, the easiest way to do this is if you create an mbed account and accept this click through agreement. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.

Thanks!

@AndrzejKurek

This comment has been minimized.

Copy link
Contributor

AndrzejKurek commented Nov 29, 2019

@svpcom, looking at the first iteration, mbedtls_md_hmac_update is called with t_len equal to 0.
Implementations shouldn't access a buffer of zero length, and if they do, accessing uninitialized data is just the smaller of their problems - accessing data out of bounds is worse (in my opinion).

Thanks to @yanesca for also taking a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.