provider: return error if buf too small when get ec pubkey param

[liyi77]

FIX: #20889
Should check buffer size first when get ec pubkey param, if buffer size too-small, return -1 as error code.

Checklist

• documentation is added or updated
• tests are added or updated

[slontis]

Please add a test for these conditions...

You could add something to an existing test like the following
see evp_extra_test.c

static int test_EC_priv_pub(void)
{
    ....
    ....
    size_t len = 0;
    unsigned char buffer[128];
    ....
    ....
    ....
    *** Add this after the EVP_PKEY_get1_encoded_public_key() calls..
    
    if (!TEST_int_eq(EVP_PKEY_get_octet_string_param(params_and_pub,
                                                     OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY,
                                                     buffer, sizeof(buffer), &len), 1)
        || !TEST_int_eq(len, 65))
        goto err;

    if (!TEST_int_eq(EVP_PKEY_get_octet_string_param(params_and_pub,
                                                     OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY,
                                                     NULL, 0, &len), 1)
        || !TEST_int_eq(len, 65))
        goto err;

    if (!TEST_int_eq(EVP_PKEY_get_octet_string_param(params_and_pub,
                                                     OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY,
                                                     buffer, 10, &len), 0)
        || !TEST_int_le(len, 10))
        goto err;

[liyi77]

Thanks for review, patch updated.

[slontis]

This test is not right,,,
If the above test was to magically pass (which it wont), then we would expect the returned len <= 10 if we passed in a max buffer of 10. If it returned 65 it would indicate we just trashed the buffer.

[liyi77]

This test is not right,,, If the above test was to magically pass (which it wont), then we would expect the returned len <= 10 if we passed in a max buffer of 10. If it returned 65 it would indicate we just trashed the buffer.

This case means if we input a too-short buffer, this function will not change the value of len (which was set to 65 in the previous testcase).
It's really confusing here, maybe we can set the value of len to 0 before this test.

int EVP_PKEY_get_octet_string_param()
{
    OSSL_PARAM params[2];
    int ret1 = 0, ret2 = 0;
......
   /* return 0 here */
    if ((ret1 = EVP_PKEY_get_params(pkey, params)))
        ret2 = OSSL_PARAM_modified(params);
   /* will not change out_len here */
    if (ret2 && out_len != NULL)
        *out_len = params[0].return_size;
    return ret1 && ret2;
}

[t8m]

I do NOT think we should check the return size is assigned in case of failure. IMO this is implementation-defined behavior.

[liyi77]

Sounds reasonable.
There is no reason for the caller to check out_len if the function has returned an error value

[liyi77]

Thanks for review, patch updated.

[t8m]

@paulidale please reconfirm

[openssl-machine]

This pull request is ready to merge

[t8m]

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.