[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Phishing Risk] [External] Re: [PATCH 4/7] crypto: Add ECDSA key par
From: |
Michael S. Tsirkin |
Subject: |
Re: [Phishing Risk] [External] Re: [PATCH 4/7] crypto: Add ECDSA key parser |
Date: |
Thu, 16 Jun 2022 12:44:28 -0400 |
On Tue, Jun 14, 2022 at 09:43:59AM +0800, 何磊 wrote:
> 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.
expect a new version then