{"action":"closed","issue":{"url":"https://api.github.com/repos/ARMmbed/mbedtls/issues/740","repository_url":"https://api.github.com/repos/ARMmbed/mbedtls","labels_url":"https://api.github.com/repos/ARMmbed/mbedtls/issues/740/labels{/name}","comments_url":"https://api.github.com/repos/ARMmbed/mbedtls/issues/740/comments","events_url":"https://api.github.com/repos/ARMmbed/mbedtls/issues/740/events","html_url":"https://github.com/ARMmbed/mbedtls/issues/740","id":199693787,"number":740,"title":"Unintended retention of old MPI values if mbedtls_mpi_fill_random fails","user":{"login":"guidovranken","id":6846644,"avatar_url":"https://avatars3.githubusercontent.com/u/6846644?v=4","gravatar_id":"","url":"https://api.github.com/users/guidovranken","html_url":"https://github.com/guidovranken","followers_url":"https://api.github.com/users/guidovranken/followers","following_url":"https://api.github.com/users/guidovranken/following{/other_user}","gists_url":"https://api.github.com/users/guidovranken/gists{/gist_id}","starred_url":"https://api.github.com/users/guidovranken/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/guidovranken/subscriptions","organizations_url":"https://api.github.com/users/guidovranken/orgs","repos_url":"https://api.github.com/users/guidovranken/repos","events_url":"https://api.github.com/users/guidovranken/events{/privacy}","received_events_url":"https://api.github.com/users/guidovranken/received_events","type":"User","site_admin":false},"labels":[{"id":19280384,"url":"https://api.github.com/repos/ARMmbed/mbedtls/labels/bug","name":"bug","color":"fc2929","default":true},{"id":273990272,"url":"https://api.github.com/repos/ARMmbed/mbedtls/labels/mirrored","name":"mirrored","color":"ededed","default":false},{"id":434949040,"url":"https://api.github.com/repos/ARMmbed/mbedtls/labels/tracking","name":"tracking","color":"006b75","default":false}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-01-09T23:38:43Z","updated_at":"2017-07-19T23:32:01Z","closed_at":"2017-07-19T23:32:01Z","body":"There are 5 places in the library where ```mbedtls_mpi_fill_random``` is called, but its return value isn't checked:\r\n\r\n- ```mbedtls_dhm_make_params```\r\n- ```mbedtls_dhm_make_public```\r\n- ```dhm_update_blinding```\r\n- ```ecp_randomize_jac```\r\n- ```ecp_randomize_mxz```\r\n\r\nThe function itself is defined as follows:\r\n```c\r\n1866 int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size,                                                                                    \r\n1867                      int (*f_rng)(void *, unsigned char *, size_t),\r\n1868                      void *p_rng )\r\n1869 {\r\n1870     int ret;         \r\n1871     unsigned char buf[MBEDTLS_MPI_MAX_SIZE];\r\n1872 \r\n1873     if( size > MBEDTLS_MPI_MAX_SIZE )                       \r\n1874         return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA );       \r\n1875                                                             \r\n1876     MBEDTLS_MPI_CHK( f_rng( p_rng, buf, size ) );   \r\n1877     MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( X, buf, size ) );\r\n1878                                                     \r\n1879 cleanup:\r\n1880     return( ret );\r\n1881 }   \r\n```\r\n\r\n```f_rng``` can fail due to entropy shortage, memory shortage, or for any other reason if a custom RNG implementation is used for ```f_rng```.\r\n\r\n```mbedtls_mpi_read_binary``` can fail due to memory shortage:\r\n\r\n```c\r\n 667 int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t buflen )\r\n 668 {\r\n 669     int ret;\r\n 670     size_t i, j, n;\r\n 671 \r\n 672     for( n = 0; n < buflen; n++ )\r\n 673         if( buf[n] != 0 )\r\n 674             break;\r\n 675 \r\n 676     MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, CHARS_TO_LIMBS( buflen - n ) ) );\r\n 677     MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) );\r\n 678 \r\n 679     for( i = buflen, j = 0; i > n; i--, j++ )\r\n 680         X->p[j / ciL] |= ((mbedtls_mpi_uint) buf[i - 1]) << ((j % ciL) << 3);\r\n 681 \r\n 682 cleanup:\r\n 683 \r\n 684     return( ret );\r\n 685 }\r\n```\r\n\r\nEvery failure path leaves the ```X``` MPI untouched.\r\n\r\nFor ```ecp_randomize_jac``` and ```ecp_randomize_mxz``` failure in ```mbedtls_mpi_fill_random``` is not important, because the MPI variable they operate on (\"```l```\") is initialized prior to the loop in which ```mbedtls_mpi_fill_random``` is called, and the loop condition guarantees that ```l``` is set to a non-zero value at some point:\r\n\r\n```ecp_randomize_jac```:\r\n```c\r\n1085 \r\n1086     mbedtls_mpi_init( &l ); mbedtls_mpi_init( &ll );\r\n1087 \r\n1088     /* Generate l such that 1 < l < p */\r\n1089     do\r\n1090     {\r\n1091         mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng );                                                                                 \r\n1092 \r\n1093         while( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 )\r\n1094             MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) );\r\n1095 \r\n1096         if( count++ > 10 )\r\n1097             return( MBEDTLS_ERR_ECP_RANDOM_FAILED );\r\n1098     }\r\n1099     while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 );\r\n```\r\n\r\n```ecp_randomize_mxz```:\r\n```c\r\n1468     mbedtls_mpi_init( &l );\r\n1469 \r\n1470     /* Generate l such that 1 < l < p */\r\n1471     do\r\n1472     {\r\n1473         mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng );                                                                                 \r\n1474 \r\n1475         while( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 )\r\n1476             MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) );\r\n1477 \r\n1478         if( count++ > 10 )\r\n1479             return( MBEDTLS_ERR_ECP_RANDOM_FAILED );\r\n1480     }\r\n1481     while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 );\r\n```\r\n\r\n```mbedtls_dhm_make_params``` and ```mbedtls_dhm_make_public``` both operate on ```ctx->X``` as the MPI where ```ctx``` is a ```mbedtls_dhm_context```. I haven't really traced back all the possible call path to see whether execution can arrive in either of these functions with a ```ctx->X``` that has retained a previous value (and thus leaving that value intact should ```mbedtls_mpi_fill_random``` fail), but in ```dhm_update_blinding``` its obvious from the function that the MPI in question (```ctx->Vi```) is already initialized to a certain value:\r\n\r\n```c\r\n304     /*\r\n305      * Ok, we need blinding. Can we re-use existing values?\r\n306      * If yes, just update them by squaring them.\r\n307      */\r\n308     if( mbedtls_mpi_cmp_int( &ctx->Vi, 1 ) != 0 )\r\n309     {\r\n310         MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vi, &ctx->Vi, &ctx->Vi ) );\r\n311         MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vi, &ctx->Vi, &ctx->P ) );\r\n312 \r\n313         MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vf, &ctx->Vf, &ctx->Vf ) );\r\n314         MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vf, &ctx->Vf, &ctx->P ) );\r\n315 \r\n316         return( 0 );\r\n317     }\r\n318 \r\n319     /*\r\n320      * We need to generate blinding values from scratch\r\n321      */\r\n322 \r\n323     /* Vi = random( 2, P-1 ) */\r\n324     count = 0;\r\n325     do\r\n326     {\r\n327         mbedtls_mpi_fill_random( &ctx->Vi, mbedtls_mpi_size( &ctx->P ), f_rng, p_rng );\r\n328 \r\n329         while( mbedtls_mpi_cmp_mpi( &ctx->Vi, &ctx->P ) >= 0 )\r\n330             MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &ctx->Vi, 1 ) );\r\n331 \r\n332         if( count++ > 10 )\r\n333             return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE );\r\n334     }\r\n335     while( mbedtls_mpi_cmp_int( &ctx->Vi, 1 ) <= 0 );\r\n```\r\n\r\nHere, a ```mbedtls_mpi_fill_random``` could cause the previous ```ctx->Vi``` to be retained.\r\n\r\nSo I suggest you put a ```MBEDTLS_MPI_CHK``` around these calls."}}
{"actor":{"display_login":"sbutcher-arm"}}