[PATCH 02/12] dpp-util: add dpp_point_from_asn1()

[PATCH 02/12] dpp-util: add dpp_point_from_asn1()

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

Given an ASN1 blob of the right form, parse and create
an l_ecc_point object. The form used is specific to DPP
hence why this isn't general purpose and put into dpp-util.
---
 src/dpp-util.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/dpp-util.h |  1 +
 2 files changed, 70 insertions(+)

diff --git a/src/dpp-util.c b/src/dpp-util.c
index 4823b2f0..81a97f97 100644
--- a/src/dpp-util.c
+++ b/src/dpp-util.c
@@ -796,3 +796,72 @@ uint8_t *dpp_point_to_asn1(const struct l_ecc_point *p, size_t *len_out)
 
 	return asn1;
 }
+
+/*
+ * Only checking for the ASN.1 form:
+ *
+ * SEQUENCE {
+ * 	SEQUENCE {
+ * 		OBJECT IDENTIFIER ecPublicKey
+ * 		OBJECT IDENTIFIER key type (p256/p384)
+ * 	}
+ * 	BITSTRING (key data)
+ * }
+ */
+struct l_ecc_point *dpp_point_from_asn1(const uint8_t *asn1, size_t len)
+{
+
+	const uint8_t *outer_seq;
+	size_t outer_len;
+	const uint8_t *inner_seq;
+	size_t inner_len;
+	const uint8_t *elem;
+	const uint8_t *key_data;
+	size_t elen = 0;
+	uint8_t tag;
+	unsigned int curve_num;
+	const struct l_ecc_curve *curve;
+
+	/* SEQUENCE */
+	outer_seq = asn1_der_find_elem(asn1, len, 0, &tag, &outer_len);
+	if (!outer_seq || tag != ASN1_ID_SEQUENCE)
+		return NULL;
+
+	/* SEQUENCE */
+	inner_seq = asn1_der_find_elem(outer_seq, outer_len, 0, &tag, &inner_len);
+	if (!inner_seq || tag != ASN1_ID_SEQUENCE)
+		return NULL;
+
+	/* OBJECT IDENTIFIER (ecPublicKey) */
+	elem = asn1_der_find_elem(inner_seq, inner_len, 0, &tag, &elen);
+	if (!elem || tag != ASN1_ID_OID)
+		return NULL;
+
+	/* Check that this OID is ecPublicKey */
+	if (!asn1_oid_eq(&ec_oid, elen, elem))
+		return NULL;
+
+	elem = asn1_der_find_elem(inner_seq, inner_len, 1, &tag, &elen);
+	if (!elem || tag != ASN1_ID_OID)
+		return NULL;
+
+	/* Check if ELL supports this curve */
+	if (asn1_oid_eq(&ec_p256_oid, elen, elem))
+		curve_num = 19;
+	else if (asn1_oid_eq(&ec_p384_oid, elen, elem))
+		curve_num = 20;
+	else
+		return NULL;
+
+	/* BITSTRING */
+	key_data = asn1_der_find_elem(outer_seq, outer_len, 1, &tag, &elen);
+	if (!key_data || tag != ASN1_ID_BIT_STRING || elen < 34)
+		return NULL;
+
+	curve = l_ecc_curve_from_ike_group(curve_num);
+	if (!curve)
+		return NULL;
+
+	return l_ecc_point_from_data(curve, key_data[1],
+					key_data + 2, elen - 2);
+}
diff --git a/src/dpp-util.h b/src/dpp-util.h
index 84d33656..82535ff8 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -167,3 +167,4 @@ bool dpp_derive_ke(const uint8_t *i_nonce, const uint8_t *r_nonce,
 				void *ke);
 
 uint8_t *dpp_point_to_asn1(const struct l_ecc_point *p, size_t *len_out);
+struct l_ecc_point *dpp_point_from_asn1(const uint8_t *asn1, size_t len);
-- 
2.31.1

Re: [PATCH 02/12] dpp-util: add dpp_point_from_asn1()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

Hi James,

On 1/18/22 15:25, James Prestwood wrote:
> Given an ASN1 blob of the right form, parse and create
> an l_ecc_point object. The form used is specific to DPP
> hence why this isn't general purpose and put into dpp-util.
> ---
>   src/dpp-util.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/dpp-util.h |  1 +
>   2 files changed, 70 insertions(+)
> 

<snip>

> +	/* BITSTRING */
> +	key_data = asn1_der_find_elem(outer_seq, outer_len, 1, &tag, &elen);
> +	if (!key_data || tag != ASN1_ID_BIT_STRING || elen < 34)

Why 34? Aren't you only concerned that elen >= 2?

> +		return NULL;
> +
> +	curve = l_ecc_curve_from_ike_group(curve_num);
> +	if (!curve)
> +		return NULL;

Perhaps this check should be moved up?

> +
> +	return l_ecc_point_from_data(curve, key_data[1],
> +					key_data + 2, elen - 2);

Aren't you relying on ecc_point_from_data to validate the length?  Hence the 
unnecessary '34' magic number above?

Should ecc_point_from_data also validate that all of the data passed in was 
consumed?  Or maybe return the number of bytes consumed so that we can validate 
it here?

Regards,
-Denis

Re: [PATCH 02/12] dpp-util: add dpp_point_from_asn1()

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

On Thu, 2022-01-20 at 14:10 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 1/18/22 15:25, James Prestwood wrote:
> > Given an ASN1 blob of the right form, parse and create
> > an l_ecc_point object. The form used is specific to DPP
> > hence why this isn't general purpose and put into dpp-util.
> > ---
> >   src/dpp-util.c | 69
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/dpp-util.h |  1 +
> >   2 files changed, 70 insertions(+)
> > 
> 
> <snip>
> 
> > +       /* BITSTRING */
> > +       key_data = asn1_der_find_elem(outer_seq, outer_len, 1,
> > &tag, &elen);
> > +       if (!key_data || tag != ASN1_ID_BIT_STRING || elen < 34)
> 
> Why 34? Aren't you only concerned that elen >= 2?

Yes, thats right.

> 
> > +               return NULL;
> > +
> > +       curve = l_ecc_curve_from_ike_group(curve_num);
> > +       if (!curve)
> > +               return NULL;
> 
> Perhaps this check should be moved up?
> 
> > +
> > +       return l_ecc_point_from_data(curve, key_data[1],
> > +                                       key_data + 2, elen - 2);
> 
> Aren't you relying on ecc_point_from_data to validate the length? 
> Hence the 
> unnecessary '34' magic number above?
> 
> Should ecc_point_from_data also validate that all of the data passed
> in was 
> consumed?  Or maybe return the number of bytes consumed so that we
> can validate 
> it here?

I think changing point_from_data to require a strict length is fine,
and probably the best route. Only other way would be to manually check
before calling it.

> 
> Regards,
> -Denis