return internal error code, if failed #1096

RonEld · 2017-09-19T14:32:38Z

Description

in case mbedtls_aes_crypt_ecb failed, return an error from other modes
calling it
Resolves #1092

Status

READY

Requires Backporting

Yes
Which branch?
mbedtls-2.1
mbedtls-1.3

Migrations

NO

Todos

Tests
Documentation
Changelog updated
Backported

hanno-becker · 2017-09-20T09:16:53Z

ChangeLog

+
+Bugfix
+   * return error code of `mbedtls_aes_crypt_ecb` if failed, when calling it from
+     other modes. FIx for #1092 reported by adustm


Two typos here

hanno-becker · 2017-09-20T09:20:01Z

library/aes.c

@@ -979,6 +995,9 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx,

    *iv_off = n;

+end:
+    if ( ret != 0 )
+        mbedtls_zeroize( output, length );
    return( 0 );


This should be return( ret );

hanno-becker · 2017-09-20T09:20:17Z

library/aes.c

-
+end:
+    if ( ret != 0 )
+        mbedtls_zeroize( output, length );
    return( 0 );


This should be return( ret );

hanno-becker · 2017-09-20T09:20:20Z

library/aes.c

-
+end:
+    if ( ret != 0 )
+        mbedtls_zeroize( output, length );
    return( 0 );


This should be return( ret );

hanno-becker · 2017-09-20T09:24:09Z

library/aes.c

@@ -880,7 +880,7 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
                    const unsigned char *input,
                    unsigned char *output )
 {
-    int i;
+    int i, ret;


ret should be initialized as the body of the function doesn't alter it for length == 0.

hanno-becker · 2017-09-20T09:24:31Z

library/aes.c

@@ -947,15 +955,19 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx,
                       const unsigned char *input,
                       unsigned char *output )
 {
-    int c;
+    int c, ret;


ret should be initialized here, too.

hanno-becker · 2017-09-20T09:24:38Z

library/aes.c

@@ -994,11 +1013,14 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx,
 {
    unsigned char c;
    unsigned char ov[17];
+    int ret;


ret should be initialized here, too.

hanno-becker · 2017-09-20T09:24:44Z

library/aes.c

@@ -1027,13 +1051,15 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx,
                       const unsigned char *input,
                       unsigned char *output )
 {
-    int c, i;
+    int c, i, ret;


ret should be initialized here, too.

andresag01

Additional to @hanno-arm's comments. I did a quick grep and found that there are a few other occurrences of mbedtls_aes_*() functions across the library that do not have their return codes being checked. For example:

--> grep -rIn "mbedtls_aes_crypt_\(ecb\|cbc\)" ./library/*
./library/aes.c:1307:                mbedtls_aes_crypt_ecb( &ctx, v, buf, buf );
./library/aes.c:1323:                mbedtls_aes_crypt_ecb( &ctx, v, buf, buf );
./library/aes.c:1364:                mbedtls_aes_crypt_cbc( &ctx, v, 16, iv, buf, buf );
./library/aes.c:1383:                mbedtls_aes_crypt_cbc( &ctx, v, 16, iv, buf, buf );
./library/ctr_drbg.c:202:            mbedtls_aes_crypt_ecb( &aes_ctx, MBEDTLS_AES_ENCRYPT, chain, chain );
./library/ctr_drbg.c:222:        mbedtls_aes_crypt_ecb( &aes_ctx, MBEDTLS_AES_ENCRYPT, iv, iv );
./library/ctr_drbg.c:253:        mbedtls_aes_crypt_ecb( &ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, p );
./library/ctr_drbg.c:380:        mbedtls_aes_crypt_ecb( &ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, tmp );
./library/pem.c:195:    mbedtls_aes_crypt_cbc( &aes_ctx, MBEDTLS_AES_DECRYPT, buflen,

Do you think we should fix these ones here as well?

andresag01 · 2017-09-20T09:23:46Z

library/aes.c

    size_t n = *nc_off;

    while( length-- )
    {
        if( n == 0 ) {
-            mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block );
+            ret = mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block );


Style: This line is a bit too long. Could you please break it up?

hanno-becker

I agree with the approach to fix the bug, but there are still some issues with the code mentioned in the comments.

RonEld · 2017-09-24T11:25:42Z

Hi @andresag01 @hanno-arm Thank you for your comments. I agree we should change other files as well

RonEld · 2017-09-24T12:27:01Z

Hi @andresag01 @hanno-arm I modified according to your comments
Note that there are still locations where we don't check ther return code:

> grep -rIn "mbedtls_aes_crypt_\(ecb\|cbc\|ctr\)" ./library/*
./library/aes.c:1308:                mbedtls_aes_crypt_ecb( &ctx, v, buf, buf );
./library/aes.c:1324:                mbedtls_aes_crypt_ecb( &ctx, v, buf, buf );
./library/aes.c:1365:                mbedtls_aes_crypt_cbc( &ctx, v, 16, iv, buf, buf );
./library/aes.c:1384:                mbedtls_aes_crypt_cbc( &ctx, v, 16, iv, buf, buf );
./library/aes.c:1489:            mbedtls_aes_crypt_ctr( &ctx, len, &offset, nonce_counter, stream_block,
./library/aes.c:1506:            mbedtls_aes_crypt_ctr( &ctx, len, &offset, nonce_counter, stream_block,
./library/pem.c:195:    mbedtls_aes_crypt_cbc( &aes_ctx, MBEDTLS_AES_DECRYPT, buflen,

In aes.c these are in the selftest, which is already addressed in #964 and I prefer to avoid the conflicts
In pem.c , the call is in static void pem_aes_decrypt() , which we may consider changing its return value as well, but I think this should be done in a different PR

andresag01 · 2017-09-25T09:53:23Z

ChangeLog

@@ -3,8 +3,8 @@ mbed TLS ChangeLog (Sorted per branch, date)
 = mbed TLS x.x.x branch released xxxx-xx-xx

 Bugfix
-   * return error code of `mbedtls_aes_crypt_ecb` if failed, when calling it from
-     other modes. FIx for #1092 reported by adustm
+   * Return error code of `mbedtls_aes_crypt_ecb` if failed, when calling it from


Sorry for not pointing it out later, but please note that ChangeLog is not markdown and the ` character has no efect there. I would suggest ``mbedtls_aes_crypt_ecb`` -> mbedtls_aes_crypt_ecb()

RonEld · 2017-09-25T10:00:24Z

I fixed the ChangeLog
@andresag01 @hanno-arm please approve

andresag01 · 2017-09-25T10:13:06Z

@RonEld: Now that I recall, the return value for the PEM functions has been fixed in a separate PR. Please refer to #828

andresag01

@RonEld: Many thanks for considering the comments. I have gone through the ctr_drbg file more carefuly and pointed out a few things. What do you think?

andresag01 · 2017-09-25T10:01:27Z

library/ctr_drbg.c

-    mbedtls_aes_setkey_enc( &aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS );
-
+    ret = mbedtls_aes_setkey_enc( &aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS );
+    if ( ret != 0 )


Style: Please remove between if and (

andresag01 · 2017-09-25T10:06:15Z

library/ctr_drbg.c

    memcpy( ctx->counter, tmp + MBEDTLS_CTR_DRBG_KEYSIZE, MBEDTLS_CTR_DRBG_BLOCKSIZE );

-    return( 0 );
+exit:
+    return( ret );
 }

 void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx,


I know that its slightly pointless to check the return codes of block_cipher_df() and ctr_drbg_update_internal() as mbedtls_ctr_drbg_update() does not have a return value. However, it would be nice if we could at least abort the update if either of the calls fails. Also, it would be good to add a comment nearby explaning the situation (and perhaps raise a ticket as the API might need to be modified).

It would be good to check the return values of of the functions ctr_drbg_update_internal() and ctr_drbg_update_internal() called from mbedtls_ctr_drbg_reseed() as well.

@RonEld: I dont think this was fully addressed. There are still a couple of unchecked return values here and here. Thanks!

sorry, I missed this

RonEld · 2017-09-25T10:32:35Z

@andresag01 I addressed your comments. Please approve

andresag01 · 2017-09-25T10:42:17Z

library/ctr_drbg.c

+        if( ret != 0)
+            return;
+        ret = ctr_drbg_update_internal( ctx, add_input );
+        if( ret != 0)


Style: This might not even be worth pointing out, but there is a missing before )...

RonEld · 2017-09-25T10:50:56Z

@andresag01 Thanks for your comments. I missed that part.
Please review

andresag01

@RonEld: Thanks for considering my comments. I am happy with the changes.

hanno-becker · 2017-09-25T11:45:18Z

library/ctr_drbg.c

-    ctr_drbg_update_internal( ctx, seed );
+    ret = ctr_drbg_update_internal( ctx, seed );
+    if( ret != 0 )
+        return ret;


Minor: brackets missing around ret here and a few lines above.

RonEld · 2017-09-25T14:15:29Z

I have added brackets as your comment.
Please approve

hanno-becker · 2017-09-26T06:22:10Z

library/aes.c

-    return( 0 );
+end:
+    if ( ret != 0 )
+        mbedtls_zeroize( output, length );


Sorry for not bringing this up earlier, but this probably does not do what it is intended to do: The output and length values are updated during the operations, so this zeroization will clear the remaining, so far unwritten, output buffer, while the part that has already been written is left untouched. Also, the loops decrease length at the beginning of the iteration while output is increased at the end, so the zeroization will omit one byte at the end.

Good catch! I missed that for some reason. I'll fix that.

RonEld · 2017-09-26T08:19:49Z

Hi @hanno-arm I fixed according to your comment
Please review

RonEld · 2019-01-10T08:43:30Z

@sbutcher-arm Thanks for the review.
I addressed your comments.

cc @Patater

I will squash the commits once the PR is fully approved

simonbutcher

Looks good.

simonbutcher · 2019-01-10T09:08:57Z

I guess the PR just needs squashing, back porting and re-approval now.

RonEld · 2019-01-10T09:15:24Z

@sbutcher-arm yes that's my plan

@andresag01 \ @Patater Do you want me to squash before you approve?

RonEld · 2019-01-10T09:28:49Z

@sbutcher-arm
Actually, I think that #1096 (comment) should be addressed on all other mode as well.
I will fix that first

RonEld · 2019-01-10T09:50:42Z

I removed the bytes_written in all other modes, except xts, as in this mode, output does not point to the end of the buffer, when the operation finishes.

In case `mbedtls_aes_crypt_ecb` failed, return an error from other modes calling it, and check return code of internal functions.

Add ChangeLog entry for bug fix of returning error code of ecb calls.

RonEld · 2019-01-31T14:29:40Z

@Patater @andresag01 I have rebased to resolve conflicts, and squashedthe commits. Please review.
Once approved, I will backport this to 2.7

RonEld · 2019-01-31T16:48:26Z

Backports available at #2396 and #2397

mpg · 2019-02-08T09:53:57Z

Almost ready for merge, just waiting for the 2.7 backport to be approved.

RonEld · 2019-03-20T10:50:40Z

Both backports have been fully reviewed and approved, so removing the "needs backports" label"

cc @Patater

gilles-peskine-arm · 2020-04-17T21:04:17Z

Unfortunately, there are now merge conflicts. I think I independently rediscovered the issue in ctr_drbg. The aes part is still relevant.

gilles-peskine-arm · 2021-07-07T19:23:48Z

Superseded by Catch failures of mbedtls_aes_crypt_ecb and its DES equivalents #4760 for the AES result checks.
Tracked internally for the missing zeroizations.
Already done independently for the CTR_DRBG fixes.

RonEld requested review from andresag01 and hanno-becker September 19, 2017 14:33

RonEld added bug mbed TLS team labels Sep 19, 2017

hanno-becker reviewed Sep 20, 2017

View reviewed changes

andresag01 reviewed Sep 20, 2017

View reviewed changes

hanno-becker previously requested changes Sep 20, 2017

View reviewed changes

andresag01 reviewed Sep 25, 2017

View reviewed changes

andresag01 previously approved these changes Sep 25, 2017

View reviewed changes

hanno-becker reviewed Sep 25, 2017

View reviewed changes

hanno-becker reviewed Sep 26, 2017

View reviewed changes

RonEld mentioned this pull request Apr 23, 2018

Extend the AES block modes with OFB (Output Feedback) #1593

Merged

3 tasks

simonbutcher added the needs-work label Jun 21, 2018

simonbutcher removed the needs-review Every commit must be reviewed by at least two team members, label Jan 9, 2019

RonEld dismissed Patater’s stale review via 03036aa January 10, 2019 08:42

RonEld added the needs-review Every commit must be reviewed by at least two team members, label Jan 10, 2019

simonbutcher previously approved these changes Jan 10, 2019

View reviewed changes

RonEld dismissed simonbutcher’s stale review via caa2b03 January 10, 2019 09:36

Ron Eldor added 2 commits January 31, 2019 16:24

Return internal error code, if failed

c187e06

In case `mbedtls_aes_crypt_ecb` failed, return an error from other modes calling it, and check return code of internal functions.

Add ChangeLog entry

7f074f8

Add ChangeLog entry for bug fix of returning error code of ecb calls.

RonEld force-pushed the 1092 branch from caa2b03 to 7f074f8 Compare January 31, 2019 14:28

Patater approved these changes Jan 31, 2019

View reviewed changes

This was referenced Jan 31, 2019

Backport 2.7: Return internal error code, if failed #2396

Closed

Backport 2.16: Return internal error code, if failed #2397

Closed

andresag01 approved these changes Jan 31, 2019

View reviewed changes

RonEld removed the needs-review Every commit must be reviewed by at least two team members, label Feb 3, 2019

RonEld removed the needs-backports Backports are missing or are pending review and approval. label Mar 20, 2019

RonEld added the needs-ci Needs to pass CI tests label Apr 14, 2019

gilles-peskine-arm added the needs-work label Apr 17, 2020

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Oct 6, 2020

gilles-peskine-arm closed this Jul 7, 2021

davidhorstmann-arm mentioned this pull request Sep 27, 2021

Catch failures of mbedtls_aes_crypt_ecb and its DES equivalents #4760

Merged

return internal error code, if failed #1096

return internal error code, if failed #1096

Conversation

RonEld commented Sep 19, 2017 • edited Loading

Description

Status

Requires Backporting

Migrations

Todos

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

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

RonEld commented Sep 24, 2017

RonEld commented Sep 24, 2017

Choose a reason for hiding this comment

RonEld commented Sep 25, 2017

andresag01 commented Sep 25, 2017

andresag01 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Sep 25, 2017

Choose a reason for hiding this comment

RonEld commented Sep 25, 2017

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Sep 26, 2017

RonEld commented Jan 10, 2019

simonbutcher left a comment

Choose a reason for hiding this comment

simonbutcher commented Jan 10, 2019

RonEld commented Jan 10, 2019 • edited Loading

RonEld commented Jan 10, 2019

RonEld commented Jan 10, 2019

RonEld commented Jan 31, 2019

RonEld commented Jan 31, 2019

mpg commented Feb 8, 2019

RonEld commented Mar 20, 2019

gilles-peskine-arm commented Apr 17, 2020

gilles-peskine-arm commented Jul 7, 2021

RonEld commented Sep 19, 2017 •

edited

Loading

RonEld commented Jan 10, 2019 •

edited

Loading