From: tb <tb@openbsd.org>
Date: Mon, 19 Apr 2021 17:06:37 +0000 (+0000)
Subject: Avoid division by zero in hybrid point encoding
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=eec146bd45a3fe2bbd65928cb2797588727ad017;p=openbsd

Avoid division by zero in hybrid point encoding

In hybrid and compressed point encodings, the form octet contains a bit
of information allowing to calculate y from x.  For a point on a binary
curve, this bit is zero if x is zero, otherwise it must match the
rightmost bit of of the field element y / x. The existing code only
considers the second possibility. It could thus fail with a division by
zero error as found by Guido Vranken's cryptofuzz.

This commit adds a few explanatory comments to oct2point and fixes some
KNF issues. The only actual code change is in the last hunk which adds a
BN_is_zero(x) check to avoid the division by zero.

ok jsing
---

diff --git a/lib/libcrypto/ec/ec2_oct.c b/lib/libcrypto/ec/ec2_oct.c
index 5f7f7e3c99e..9cd1dddfa16 100644
--- a/lib/libcrypto/ec/ec2_oct.c
+++ b/lib/libcrypto/ec/ec2_oct.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec2_oct.c,v 1.12 2020/12/04 08:55:30 tb Exp $ */
+/* $OpenBSD: ec2_oct.c,v 1.13 2021/04/19 17:06:37 tb Exp $ */
 /* ====================================================================
  * Copyright 2002 Sun Microsystems, Inc. ALL RIGHTS RESERVED.
  *
@@ -280,10 +280,11 @@ ec_GF2m_simple_point2oct(const EC_GROUP *group, const EC_POINT *point,
 }
 
 
-/* Converts an octet string representation to an EC_POINT.
+/*
+ * Converts an octet string representation to an EC_POINT.
  * Note that the simple implementation only uses affine coordinates.
  */
-int 
+int
 ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
     const unsigned char *buf, size_t len, BN_CTX *ctx)
 {
@@ -298,19 +299,35 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
 		ECerror(EC_R_BUFFER_TOO_SMALL);
 		return 0;
 	}
-	form = buf[0];
-	y_bit = form & 1;
-	form = form & ~1U;
-	if ((form != 0) && (form != POINT_CONVERSION_COMPRESSED) &&
-	    (form != POINT_CONVERSION_UNCOMPRESSED) &&
-	    (form != POINT_CONVERSION_HYBRID)) {
+
+	/*
+	 * The first octet is the point conversion octet PC, see X9.62, page 4
+	 * and section 4.4.2.  It must be:
+	 *	0x00		for the point at infinity
+	 *	0x02 or 0x03	for compressed form
+	 *	0x04		for uncompressed form
+	 *	0x06 or 0x07	for hybrid form.
+	 * For compressed or hybrid forms, we store the last bit of buf[0] as
+	 * y_bit and clear it from buf[0] so as to obtain a POINT_CONVERSION_*.
+	 * We error if buf[0] contains any but the above values.
+	 */
+	y_bit = buf[0] & 1;
+	form = buf[0] & ~1U;
+
+	if (form != 0 && form != POINT_CONVERSION_COMPRESSED &&
+	    form != POINT_CONVERSION_UNCOMPRESSED &&
+	    form != POINT_CONVERSION_HYBRID) {
 		ECerror(EC_R_INVALID_ENCODING);
 		return 0;
 	}
-	if ((form == 0 || form == POINT_CONVERSION_UNCOMPRESSED) && y_bit) {
-		ECerror(EC_R_INVALID_ENCODING);
-		return 0;
+	if (form == 0 || form == POINT_CONVERSION_UNCOMPRESSED) {
+		if (y_bit != 0) {
+			ECerror(EC_R_INVALID_ENCODING);
+			return 0;
+		}
 	}
+
+	/* The point at infinity is represented by a single zero octet. */
 	if (form == 0) {
 		if (len != 1) {
 			ECerror(EC_R_INVALID_ENCODING);
@@ -318,6 +335,7 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
 		}
 		return EC_POINT_set_to_infinity(group, point);
 	}
+
 	field_len = (EC_GROUP_get_degree(group) + 7) / 8;
 	enc_len = (form == POINT_CONVERSION_COMPRESSED) ? 1 + field_len :
 	    1 + 2 * field_len;
@@ -326,6 +344,7 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
 		ECerror(EC_R_INVALID_ENCODING);
 		return 0;
 	}
+
 	if (ctx == NULL) {
 		ctx = new_ctx = BN_CTX_new();
 		if (ctx == NULL)
@@ -360,11 +379,24 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
 			goto err;
 		}
 		if (form == POINT_CONVERSION_HYBRID) {
-			if (!group->meth->field_div(group, yxi, y, x, ctx))
-				goto err;
-			if (y_bit != BN_is_odd(yxi)) {
-				ECerror(EC_R_INVALID_ENCODING);
-				goto err;
+			/*
+			 * Check that the form in the encoding was set
+			 * correctly according to X9.62 4.4.2.a, 4(c),
+			 * see also first paragraph of X9.62 4.4.1.b.
+			 */
+			if (BN_is_zero(x)) {
+				if (y_bit != 0) {
+					ECerror(EC_R_INVALID_ENCODING);
+					goto err;
+				}
+			} else {
+				if (!group->meth->field_div(group, yxi, y, x,
+				    ctx))
+					goto err;
+				if (y_bit != BN_is_odd(yxi)) {
+					ECerror(EC_R_INVALID_ENCODING);
+					goto err;
+				}
 			}
 		}
 		/*