stm32 sha1 acceleration does not work since 2.16.8 release

[xhpohanka]

I'm using mbedtls on stm32f7 board. Until 2.16.7 it worked fine with alternative functions providing hw acceleration (same as ones in mbedos (https://github.com/ARMmbed/mbed-os/blob/master/connectivity/drivers/mbedtls/TARGET_STM/sha1_alt.cpp)). I have recently tried to update to actual 2.16 tip but here I noticed that my webserver does not work and reports SSL - Verification of the message MAC failed when MBEDTLS_SHA1_ALT is defined. With internal SHA1 everything works fine.

With bisecting I have found that it started with this commit de02b58. I do not see on the first sight what could be the issue here, hopefuly someone with more knowledge can help...

[xhpohanka]

Any progress here, please?

[mpg]

Hi, and sorry for the long response time.

Thanks for bisecting the issue. The commit is somewhat consistent with the error you're getting, in the this commit changes how the MAC is verified, which is what the error is about. OTOH, I don't really see what in this commit could trigger issues in the SHA1 alt implementation. First I was going to say that perhaps it might have to do with the fact that after this commit, the computation relies on mbedtls_md_clone() which wasn't used here before this commit. But on the other hand, clone() is also used in computing the TLS handshake hash and always has been, so if the alt implementation got it wrong, we should be already have been observing failures in all handshakes that use SHA-1 long before this commit.

This would be easily verified by running the test suites on your device, but I'm afraid we don't have good facilities for running our test suites on embedded devices yet... Could you try running the following test on your device (possibly adapting the calls to printf) with check if the two printed hash values are identical?

#include "mbedtls/md.h"
#include "stdio.h"

int main( void ) {
    int ret;
    mbedtls_md_context_t ctx, clo;
    const mbedtls_md_info_t *sha1 = mbedtls_md_info_from_type( MBEDTLS_MD_SHA1 );
    unsigned char buf[250];
    const size_t split = 123;
    unsigned char out[20];

    mbedtls_md_init( &ctx );
    mbedtls_md_init( &clo );

    for( size_t i = 0; i < sizeof( buf ); i++ )
        buf[i] = i;

    /* start computation with one context */
    ret = mbedtls_md_setup( &ctx, sha1, 0 );
    if( ret != 0 )
        goto exit;

    ret = mbedtls_md_starts( &ctx );
    if( ret != 0 )
        goto exit;

    ret = mbedtls_md_update( &ctx, buf, split );
    if( ret != 0 )
        goto exit;

    /* clone in the middle */
    ret = mbedtls_md_setup( &clo, sha1, 0 );
    if( ret != 0 )
        goto exit;

    ret = mbedtls_md_clone( &clo, &ctx );
    if( ret != 0 )
        goto exit;

    /* finish computation with original */
    ret = mbedtls_md_update( &ctx, buf + split, sizeof( buf ) - split );
    if( ret != 0 )
        goto exit;

    ret = mbedtls_md_finish( &ctx, out );
    if( ret != 0 )
        goto exit;

    printf( "Origin:" );
    for( size_t i = 0; i < sizeof( out ); i++ )
        printf( " %02x", out[i] );
    printf( "\n" );

    /* finish computation with clone */
    ret = mbedtls_md_update( &clo, buf + split, sizeof( buf ) - split );
    if( ret != 0 )
        goto exit;

    ret = mbedtls_md_finish( &clo, out );
    if( ret != 0 )
        goto exit;

    printf( "Cloned:" );
    for( size_t i = 0; i < sizeof( out ); i++ )
        printf( " %02x", out[i] );
    printf( "\n" );


exit:
    mbedtls_md_free( &ctx );
    mbedtls_md_free( &clo );

    return( ret );
}

(If the results are identical, can you try varying the value of split, trying for example 63, 64, 65, 127, 128, 129?)

[xhpohanka]

Hello,

Originally I have mentioned only sha-1 but later I found that it concerns also sha-256.

I have run your tests and indeed the values are identical.

  SHA-224 test #1: passed
  SHA-224 test #2: passed
  SHA-224 test #3: passed
  SHA-256 test #1: passed
  SHA-256 test #2: passed
  SHA-256 test #3: passed

  HMAC_DRBG (PR = True) : passed
  HMAC_DRBG (PR = False) : passed

------ split 63
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 64
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 65
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 123
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 127
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 128
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
------ split 129
Origin: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2
Cloned: a1 21 9d 23 a9 ef aa ed e8 6c 39 28 88 a1 8f 99 5b 15 e8 e2

However imediately after during TLS handshake in web server...

ssl_msg.c:1818: |1| dumping 'expected mac' (32 bytes)
ssl_msg.c:1818: |1| 0000:  21 e5 e1 39 43 e6 43 b9 22 33 fa 32 96 e5 fd 34  !..9C.C."3.2...4
ssl_msg.c:1818: |1| 0010:  0c 41 9c b9 8b a3 bf 9c fb 10 eb d1 c4 22 f1 a4  .A..........."..
ssl_msg.c:1819: |1| dumping 'message  mac' (32 bytes)
ssl_msg.c:1819: |1| 0000:  1c 58 b9 bf ca cf 3e 48 99 23 5d c2 1f 17 ae 1e  .X....>H.#].....
ssl_msg.c:1819: |1| 0010:  7a 93 0d de d0 90 7a f5 1a e6 27 34 d1 ae ab 33  z.....z...'4...3
ssl_msg.c:1826: |1| message mac does not match
ssl_msg.c:3900: |1| ssl_decrypt_buf() returned -29056 (-0x7180)
ssl_msg.c:4062: |1| ssl_get_next_record() returned -29056 (-0x7180)
ssl_tls.c:3574: |1| mbedtls_ssl_read_record() returned -29056 (-0x7180)```

[mpg]

Ok, thanks for running the test!

The debug output you posted confirms that the problem is with the computation of the expected MAC, which was modified in the commit identified by git bisect, so that part really checks out.

So, I went again line by line over this commit looking for things that would trip up a alt implementation, and now I think I've found my mistake. Could you try applying the following patch on top of the current mbedtls-2.16 and see if it fixes the problem?

diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index c749a8611c43..a6c629748163 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -1895,6 +1895,9 @@ int mbedtls_ssl_cf_hmac(
             MD_CHK( mbedtls_md_update( ctx, data + offset, 1 ) );
     }
 
+    /* The context needs to finish() before it starts() again */
+    MD_CHK( mbedtls_md_finish( ctx, aux_out ) );
+
     /* Now compute HASH(okey + inner_hash) */
     MD_CHK( mbedtls_md_starts( ctx ) );
     MD_CHK( mbedtls_md_update( ctx, okey, block_size ) );

[xhpohanka]

Bingo!

This really solves the described issue. Thank you for your support.

[mpg]

Good news! Thanks for testing the patch! I'll create a PR so that the fix lands in the next release of 2.16.