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 incorrecly fail with a
division by zero error as found by Guido Vranken's cryptofuzz.

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

Fixes #15021

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15112)

diff --git a/crypto/ec/ec2_oct.c b/crypto/ec/ec2_oct.c
index 48543265eeabb782906e6c0dc39211059107514f..a0ff0496b3afe20784a9e66ae765a34f8144f0df 100644 (file)
--- a/crypto/ec/ec2_oct.c
+++ b/crypto/ec/ec2_oct.c
@@ -247,9 +247,21 @@ int ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
         ECerr(EC_F_EC_GF2M_SIMPLE_OCT2POINT, EC_R_BUFFER_TOO_SMALL);
         return 0;
     }
-    form = buf[0];
-    y_bit = form & 1;
-    form = form & ~1U;
+
+    /*
+     * The first octet is the point converison 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)) {
@@ -261,6 +273,7 @@ int ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
         return 0;
     }
 
+    /* The point at infinity is represented by a single zero octet. */
     if (form == 0) {
         if (len != 1) {
             ECerr(EC_F_EC_GF2M_SIMPLE_OCT2POINT, EC_R_INVALID_ENCODING);
@@ -312,11 +325,23 @@ int 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)) {
-                ECerr(EC_F_EC_GF2M_SIMPLE_OCT2POINT, 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) {
+                    ECerr(ERR_LIB_EC, 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)) {
+                    ECerr(ERR_LIB_EC, EC_R_INVALID_ENCODING);
+                    goto err;
+                }
             }
         }