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

[Michael S. Tsirkin]

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é  
> > wrote:
> > 
> > On 13/6/22 10:45, Lei He wrote:
> >> Add ECDSA key parser and ECDSA signautre parser.
> >> Signed-off-by: lei he 
> >> ---
> >>  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 00..5da317ec44
> >> --- /dev/null
> >> +++ b/crypto/ecdsakey-builtin.c.inc
> >> @@ -0,0 +1,248 @@
> >> +/*
> >> + * QEMU Crypto akcipher algorithms
> >> + *
> >> + * Copyright (c) 2022 Bytedance
> >> + * Author: lei he 
> >> + *
> >> + * 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 
> >> .
> >> + *
> >> + */
> >> +
> >> +#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

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

[Philippe Mathieu-Daudé via]

On 14/6/22 03:43, 何磊 wrote:

Hi Philippe, lots of thanks for your review!


On Jun 13, 2022, at 10:19 PM, Philippe Mathieu-Daudé  wrote:

On 13/6/22 10:45, Lei He wrote:

Add ECDSA key parser and ECDSA signautre parser.
Signed-off-by: lei he 
---
  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 00..5da317ec44
--- /dev/null
+++ b/crypto/ecdsakey-builtin.c.inc
@@ -0,0 +1,248 @@
+/*
+ * QEMU Crypto akcipher algorithms
+ *
+ * Copyright (c) 2022 Bytedance
+ * Author: lei he 
+ *
+ * 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 .
+ *
+ */
+
+#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.


QCryptoDERDecodeCb seems to only return a boolean, so should follow the
style recommended in 
https://gitlab.com/qemu-project/qemu/-/commit/e3fe3988d7. Can be done 
later as a follow-up cleanup of course.



+}
+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.


Oops, indeed you are right :)




+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.

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é  wrote:
> 
> On 13/6/22 10:45, Lei He wrote:
>> Add ECDSA key parser and ECDSA signautre parser.
>> Signed-off-by: lei he 
>> ---
>>  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 00..5da317ec44
>> --- /dev/null
>> +++ b/crypto/ecdsakey-builtin.c.inc
>> @@ -0,0 +1,248 @@
>> +/*
>> + * QEMU Crypto akcipher algorithms
>> + *
>> + * Copyright (c) 2022 Bytedance
>> + * Author: lei he 
>> + *
>> + * 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 
>> .
>> + *
>> + */
>> +
>> +#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.