Fix order value for curve x448

[polhenarejos]

Closes: #5810

Status

READY

Requires Backporting

Yes
Which branch? 2.28 only

Todos

• Changelog updated
• Backported (in Backport 2.28: Fix for order value for curve448 #5825)

Steps to test or reproduce

See #5810

[polhenarejos]

How do you usually backport bugfixes?

[tom-cosgrove-arm]

@polhenarejos any chance of a non-regression test for this?

[tom-cosgrove-arm]

Backport only needed to mbedtls-2.28

[polhenarejos]

@polhenarejos any chance of a non-regression test for this?

To make a quick test I use the provided test code in #5810

#include "common.h"
#include "mbedtls/private_access.h"

#include "mbedtls/build_info.h"
#include "mbedtls/ecp.h"

#define DEBUG_BUF(p,s) { \
    printf("Payload %s (%d bytes):\r\n", #p,(int)s);\
    for (int i = 0; i < s; i += 16) {\
        for (int j = 0; j < 16; j++) {\
            if (j < s-i) printf("%02x ",(p)[i+j]);\
            else printf("   ");\
            if (j == 7) printf(" ");\
            } \
            printf("\r\n");\
        } printf("\r\n"); \
    }

int main() 
{
    int ret; 
    mbedtls_ecp_group grp;
    mbedtls_mpi N, Ns;
    uint8_t bf[56];
    static const unsigned char curve448_part_of_n[] = {
        0x83, 0x35, 0xDC, 0x16, 0x3B, 0xB1, 0x24,
        0xB6, 0x51, 0x29, 0xC9, 0x6F, 0xDE, 0x93,
        0x3D, 0x8D, 0x72, 0x3A, 0x70, 0xAA, 0xDC,
        0x87, 0x3D, 0x6D, 0x54, 0xA7, 0xBB, 0x0D,
    };
    
    mbedtls_mpi_init( &N );
    mbedtls_mpi_init( &Ns );
    mbedtls_ecp_group_init( &grp );
    
    /* N = 2^446 - 0x8335dc163bb124b65129c96fde933d8d723a70aadc873d6d54a7bb0d */
    MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( &N, 446, 1 ) );
    MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &Ns,
                        curve448_part_of_n, sizeof( curve448_part_of_n ) ) );
    MBEDTLS_MPI_CHK( mbedtls_mpi_sub_mpi( &N, &N, &Ns ) );
    
    MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_CURVE448 ) );
    
    printf( "Are equal? %s\n\n", mbedtls_mpi_cmp_mpi( &N, &grp.N ) == 0 ? "YES!" : "Oh no... :(" );

    mbedtls_mpi_write_binary( &grp.N, bf, sizeof( bf ) );
    DEBUG_BUF( bf, sizeof( bf ) );
    mbedtls_mpi_write_binary( &N, bf, sizeof( bf ) );
    DEBUG_BUF( bf, sizeof( bf ) );
    cleanup:
    mbedtls_mpi_free( &N );
    mbedtls_mpi_free( &Ns );
    return 0;
}

I am not sure if it fits to you.

[gilles-peskine-arm]

It would be nice to have this as a unit test, generalized to all the curves and values. Murphy's law: even with something as simple as a constant, if it's possible to get it wrong, sooner or later somebody will.

[mpg]

How do you usually backport bugfixes?

By creating new PR(s) for the supported branch(es) - currently, that's only mbedtls-2.28 as Tom mentioned. (See for example #5733 which is the backport of #5732.)

[AndrzejKurek]

Looks good. I can confirm that without this fix the ECP check order for CURVE448 fails.

[tom-cosgrove-arm]

LGTM

[JanneKiiskila]

Please explain to me how it can be right to free the grp and then init it right after that?
https://os.mbed.com/teams/sandbox/code/mbedtls/docs/tip/ecp_8c_source.html#l00321

Clearly shows everything gets freed after that.
What am I missing here?

As far as I can understand this will essentially set the already freed memory region to zero.

[AndrzejKurek]

The version you point to is 2.2.0. There's a different behaviour in current development:

mbedtls/library/ecp.c

Line 538 in f5b7082

void mbedtls_ecp_group_init( mbedtls_ecp_group *grp )

Please see #5810 (comment) for more details.