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

1 files changed, 49 insertions, 17 deletions

diff --git a/src/lib/libcrypto/ec/ec2_oct.c b/src/lib/libcrypto/ec/ec2_oct.c
index 5f7f7e3c99..9cd1dddfa1 100644
--- a/src/lib/libcrypto/ec/ec2_oct.c
+++ b/src/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;
+                                }
                         }
                 }
                 /*