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

[Daniel P . Berrangé]

On Mon, Jun 13, 2022 at 04:45:28PM +0800, Lei He wrote:
> Add ECDSA key parser and ECDSA signautre parser.

                         typo:  'signature'

> 
> 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;
> +    }
> +    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;
> +    return 0;
> +}
> +
> +static int __qcrypto_akcipher_builtin_ecdsa_pubkey_parse(
> +    QCryptoAkCipherECDSAKey *ecdsa,
> +    const uint8_t *key, size_t keylen, Error **errp);

It is not good practice to use '_' on the start of method
names in apps, as names with a leading '_' are reserved.

> +
> +static int extract_pubkey(void *ctx, const uint8_t *value,
> +                          size_t vlen, Error **errp)
> +{
> +    QCryptoAkCipherECDSAKey *ecdsa = (QCryptoAkCipherECDSAKey *)ctx;
> +    if (vlen < 4) {
> +        error_setg(errp, "Public key part too short");
> +        return -1;
> +    }
> +    /* Skip meta bit of BIT STRING */
> +    value++;
> +    vlen--;
> +    return __qcrypto_akcipher_builtin_ecdsa_pubkey_parse(
> +        ecdsa, value, vlen, errp);
> +}
> +
> +/**
> + *
> + *        ECDSASignature ::= SEQUENCE {
> + *             r           INTEGER
> + *             s           INTEGER
> + *         }
> + */
> +QCryptoAkCipherECDSASig *qcrypto_akcipher_ecdsasig_parse(
> +    const uint8_t *signature, size_t len, Error **errp)
> +{
> +    QCryptoAkCipherECDSASig *sig = g_new0(QCryptoAkCipherECDSASig, 1);

Use  g_autoptr(QCryptoAkCipherECDSASig) sig  here

> +    const uint8_t *seq;
> +    size_t seq_length;
> +    int decode_ret;
> +
> +    decode_ret = qcrypto_der_decode_seq(&signature, &len,
> +                                        extract_cons_content, &seq, errp);
> +
> +    if (decode_ret < 0 || len != 0) {
> +        goto error;
> +    }

If 'decode_ret < 0' then errp should be set by qcrypto_der_decode_seq
which is fine.  For len != 0, we need to report an error ourselves.
I see you pushed it to the error label so later codepath can share it.
I think it is better to do it here though, because it makes it clear
to the reader which codepaths are triggering this generic error
messages. So

     if (decode_ret < 0)
         goto error;
     }
     if (len != 0) {
         error_setg(errp, "Invalid RSA public key");
     }


> +    seq_length = decode_ret;
> +
> +    if (qcrypto_der_decode_int(&seq, &seq_length, extract_mpi,
> +                               &sig->r, errp) < 0 ||
> +        qcrypto_der_decode_int(&seq, &seq_length, extract_mpi,
> +                               &sig->s, errp) < 0) {
> +        goto error;
> +    }
> +    if (seq_length != 0) {

Add

        error_setg(errp, "Invalid RSA public key");

> +        goto error;
> +    }
> +
> +    return sig;

return g_steal_pointer(&sig)

> +
> +error:
> +    if (errp && !*errp) {
> +        error_setg(errp, "Invalid RSA public key");
> +    }

and remove this

> +    qcrypto_akcipher_ecdsasig_free(sig);
> +    return NULL;
> +}

This error block won't need to exist at all. Everywhere can
just 'return NULL' instead of 'goto error'



> +static QCryptoAkCipherECDSAKey *qcrypto_akcipher_builtin_ecdsa_privkey_parse(
> +    const uint8_t *key, size_t keylen, Error **errp)
> +{
> +    QCryptoAkCipherECDSAKey *ecdsa = g_new0(QCryptoAkCipherECDSAKey, 1);

g_autoptr(QCryptoAkCipherECDSAKey)

and change all the 'goto error' to 'return NULL'

> +    uint8_t version;
> +    const uint8_t *seq, *pubkey;
> +    int decode_ret;
> +    size_t seq_length, pubkey_length;
> +
> +    decode_ret = qcrypto_der_decode_seq(&key, &keylen, extract_cons_content,
> +                                        &seq, errp);
> +    if (decode_ret < 0 || keylen != 0) {
> +        goto error;
> +    }
> +    seq_length = decode_ret;
> +
> +    if (qcrypto_der_decode_int(&seq, &seq_length, extract_version,
> +                               &version, errp) < 0 ||
> +        qcrypto_der_decode_octet_str(&seq, &seq_length, extract_mpi,
> +                                     &ecdsa->priv, errp) < 0) {
> +        goto error;
> +    }
> +
> +    /* Here we just ignore curve id */
> +    qcrypto_der_decode_ctx_tag(&seq, &seq_length, 0, NULL, NULL, NULL);
> +
> +    decode_ret = qcrypto_der_decode_ctx_tag(&seq, &seq_length, 1,
> +                                            extract_cons_content,
> +                                            &pubkey, NULL);
> +    if (decode_ret > 0) {
> +        pubkey_length = decode_ret;
> +        if (qcrypto_der_decode_bit_str(&pubkey, &pubkey_length,
> +                                       extract_pubkey, ecdsa, errp) < 0 ||
> +            pubkey_length != 0) {
> +            goto error;
> +        }
> +    }
> +
> +    if (seq_length != 0) {
> +        goto error;
> +    }
> +
> +    return ecdsa;

return g_steal_pointer(&ecdsa)

> +
> +error:
> +    if (errp && !*errp) {
> +        error_setg(errp, "Failed to parse ecdsa private key");
> +    }

Same note as earlier, about having this error_setg earlier
at the exact places where the relevant error condition first
occurs

> +    qcrypto_akcipher_ecdsakey_free(ecdsa);
> +    return NULL;
> +}



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|