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 5f7f7e3..9cd1ddd 100644 (file)
--- 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;
+                               }
                        }
                }
                /*