Bug 934545: ensure that DH public values are not 0, 1, p-1 nor >= p.
authorAdam Langley <agl@chromium.org>
Thu, 12 Dec 2013 15:54:39 -0800
changeset 10965 12c42006aed8
parent 10964 7915c49baf3d
child 10966 f50e7e84f082
push id243
push userwtc@google.com
push dateThu, 12 Dec 2013 23:54:49 +0000
bugs934545
Bug 934545: ensure that DH public values are not 0, 1, p-1 nor >= p. r=wtc.
lib/freebl/dh.c
--- a/lib/freebl/dh.c
+++ b/lib/freebl/dh.c
@@ -198,37 +198,57 @@ cleanup:
 
 SECStatus 
 DH_Derive(SECItem *publicValue, 
           SECItem *prime, 
           SECItem *privateValue, 
           SECItem *derivedSecret, 
           unsigned int outBytes)
 {
-    mp_int p, Xa, Yb, ZZ;
+    mp_int p, Xa, Yb, ZZ, psub1;
     mp_err err = MP_OKAY;
     int len = 0;
     unsigned int nb;
     unsigned char *secret = NULL;
     if (!publicValue || !prime || !privateValue || !derivedSecret) {
 	PORT_SetError(SEC_ERROR_INVALID_ARGS);
 	return SECFailure;
     }
     memset(derivedSecret, 0, sizeof *derivedSecret);
     MP_DIGITS(&p)  = 0;
     MP_DIGITS(&Xa) = 0;
     MP_DIGITS(&Yb) = 0;
     MP_DIGITS(&ZZ) = 0;
+    MP_DIGITS(&psub1) = 0;
     CHECK_MPI_OK( mp_init(&p)  );
     CHECK_MPI_OK( mp_init(&Xa) );
     CHECK_MPI_OK( mp_init(&Yb) );
     CHECK_MPI_OK( mp_init(&ZZ) );
+    CHECK_MPI_OK( mp_init(&psub1) );
     SECITEM_TO_MPINT(*publicValue,  &Yb);
     SECITEM_TO_MPINT(*privateValue, &Xa);
     SECITEM_TO_MPINT(*prime,        &p);
+    CHECK_MPI_OK( mp_sub_d(&p, 1, &psub1) );
+
+    /* We assume that the modulus, p, is a safe prime. That is, p = 2q+1 where
+     * q is also a prime. Thus the orders of the subgroups are factors of 2q:
+     * namely 1, 2, q and 2q.
+     *
+     * We check that the peer's public value isn't zero (which isn't in the
+     * group), one (subgroup of order one) or p-1 (subgroup of order 2). We
+     * also check that the public value is less than p, to avoid being fooled
+     * by values like p+1 or 2*p-1.
+     *
+     * Thus we must be operating in the subgroup of size q or 2q. */
+    if (mp_cmp_d(&Yb, 1) <= 0 ||
+	mp_cmp(&Yb, &psub1) >= 0) {
+	err = MP_BADARG;
+	goto cleanup;
+    }
+
     /* ZZ = (Yb)**Xa mod p */
     CHECK_MPI_OK( mp_exptmod(&Yb, &Xa, &p, &ZZ) );
     /* number of bytes in the derived secret */
     len = mp_unsigned_octet_size(&ZZ);
     if (len <= 0) {
         err = MP_BADARG;
         goto cleanup;
     }
@@ -255,16 +275,17 @@ DH_Derive(SECItem *publicValue,
     } else {
 	memcpy(derivedSecret->data, secret + len - nb, nb);
     }
 cleanup:
     mp_clear(&p);
     mp_clear(&Xa);
     mp_clear(&Yb);
     mp_clear(&ZZ);
+    mp_clear(&psub1);
     if (secret) {
 	/* free the buffer allocated for the full secret. */
 	PORT_ZFree(secret, len);
     }
     if (err) {
 	MP_TO_SEC_ERROR(err);
 	if (derivedSecret->data) 
 	    PORT_ZFree(derivedSecret->data, derivedSecret->len);