Re: [Phishing Risk] [External] Re: [PATCH 4/7] crypto: Add ECDSA key parser

[何磊]

Hi Philippe, lots of thanks for your review!

> On Jun 13, 2022, at 10:19 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> On 13/6/22 10:45, Lei He wrote:
>> Add ECDSA key parser and ECDSA signautre parser.
>> Signed-off-by: lei he <helei.sig11@bytedance.com>
>> ---
>>  crypto/ecdsakey-builtin.c.inc | 248 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  crypto/ecdsakey.c             | 118 ++++++++++++++++++++
>>  crypto/ecdsakey.h             |  66 +++++++++++
>>  crypto/meson.build            |   1 +
>>  4 files changed, 433 insertions(+)
>>  create mode 100644 crypto/ecdsakey-builtin.c.inc
>>  create mode 100644 crypto/ecdsakey.c
>>  create mode 100644 crypto/ecdsakey.h
>> diff --git a/crypto/ecdsakey-builtin.c.inc b/crypto/ecdsakey-builtin.c.inc
>> new file mode 100644
>> index 0000000000..5da317ec44
>> --- /dev/null
>> +++ b/crypto/ecdsakey-builtin.c.inc
>> @@ -0,0 +1,248 @@
>> +/*
>> + * QEMU Crypto akcipher algorithms
>> + *
>> + * Copyright (c) 2022 Bytedance
>> + * Author: lei he <helei.sig11@bytedance.com>
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#include "der.h"
>> +#include "ecdsakey.h"
>> +
>> +#define QCRYPTO_ECDSA_PUBKEY_FMT_UNCOMPRESSED 0x04
>> +
>> +static int extract_mpi(void *ctx, const uint8_t *value,
>> +                       size_t vlen, Error **errp)
>> +{
>> +    QCryptoAkCipherMPI *mpi = (QCryptoAkCipherMPI *)ctx;
>> +    if (vlen == 0) {
>> +        error_setg(errp, "Empty mpi field");
>> +        return -1;
> 
> Functions taking Error* param usually return a boolean.

It's a good idea to make such functions that only return 0 or -1 return bool 
directly, but this change 
will require modification of rsakey related code. If you strongly request it, I 
will modify it in another patch.

> 
>> +    }
>> +    mpi->data = g_memdup2(value, vlen);
>> +    mpi->len = vlen;
>> +    return 0;
>> +}
>> +
>> +static int extract_version(void *ctx, const uint8_t *value,
>> +                           size_t vlen, Error **errp)
>> +{
>> +    uint8_t *version = (uint8_t *)ctx;
>> +    if (vlen != 1 || *value > 1) {
>> +        error_setg(errp, "Invalid rsakey version");
>> +        return -1;
>> +    }
>> +    *version = *value;
>> +    return 0;
>> +}
>> +
>> +static int extract_cons_content(void *ctx, const uint8_t *value,
>> +                                size_t vlen, Error **errp)
>> +{
>> +    const uint8_t **content = (const uint8_t **)ctx;
>> +    if (vlen == 0) {
>> +        error_setg(errp, "Empty sequence");
>> +        return -1;
>> +    }
>> +    *content = value;
> 
> You need to check (vlen >= sizeof(uint8_t *)) to avoid overrun.

The decoder will parse the meta data of ASN1 types and pass the real data part 
to the callback function. 
The above statement only saves the starting address of the ‘data part' and does 
not actually access the 
data, so there is no need to check the size of vlen. 

> 
>> +    return 0;
>> +}
>> +
>> +static int __qcrypto_akcipher_builtin_ecdsa_pubkey_parse(
>> +    QCryptoAkCipherECDSAKey *ecdsa,
>> +    const uint8_t *key, size_t keylen, Error **errp);
> 
> Why use the reserved __prefix?

I will fix it later.