Harden constitution validation for JWT, CA, and member keys

CHANGELOG.md

@@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 - `ccf.ledger` `MERKLE` verification level now also verifies COSE-only ledgers (previously a silent no-op) (#7904).
 - Nodes started in recovery or join mode from a snapshot more recent than the latest ledger file now correctly resume writing from the snapshot boundary (#7901).
+- Default and minimal sample constitution validation now rejects weak or malformed JWT, CA bundle, and member encryption key inputs, and supports P-521 EC JWK validation (#7924).
 
 ## [7.0.3]
 

doc/build_apps/auth/jwt.rst

@@ -20,7 +20,7 @@ Before adding public token signing keys to a running CCF network, the IdP has to
         {
           "name": "set_jwt_issuer",
           "args": {
-            "issuer": "my_issuer",
+            "issuer": "https://my.issuer",
             "auto_refresh": false
           }
         }
@@ -38,7 +38,7 @@ After this proposal is accepted, signing keys for an issuer can be updated with
         {
           "name": "set_jwt_public_signing_keys",
           "args": {
-            "issuer": "my_issuer",
+            "issuer": "https://my.issuer",
             "jwks": {
               "keys": [
                 {

include/ccf/crypto/curve.h

@@ -24,7 +24,9 @@ namespace ccf::crypto
     SECP256R1,
     /// The CURVE25519 curve
     CURVE25519,
-    X25519
+    X25519,
+    /// The SECP521R1 curve
+    SECP521R1
   };
 
   DECLARE_JSON_ENUM(
@@ -33,7 +35,8 @@ namespace ccf::crypto
      {CurveID::SECP384R1, "Secp384R1"},
      {CurveID::SECP256R1, "Secp256R1"},
      {CurveID::CURVE25519, "Curve25519"},
-     {CurveID::X25519, "X25519"}});
+     {CurveID::X25519, "X25519"},
+     {CurveID::SECP521R1, "Secp521R1"}});
 
   static constexpr CurveID service_identity_curve_choice = CurveID::SECP384R1;
   // SNIPPET_END: supported_curves
@@ -53,6 +56,8 @@ namespace ccf::crypto
         return MDType::SHA384;
       case CurveID::SECP256R1:
         return MDType::SHA256;
+      case CurveID::SECP521R1:
+        return MDType::SHA512;
       default:
       {
         throw std::logic_error(fmt::format("Unhandled CurveId: {}", ec));

include/ccf/crypto/jwk.h

@@ -78,6 +78,8 @@ namespace ccf::crypto
         return JsonWebKeyECCurve::P384;
       case CurveID::SECP256R1:
         return JsonWebKeyECCurve::P256;
+      case CurveID::SECP521R1:
+        return JsonWebKeyECCurve::P521;
       default:
         throw std::logic_error(fmt::format("Unknown curve {}", curve_id));
     }
@@ -88,8 +90,7 @@ namespace ccf::crypto
     switch (jwk_curve)
     {
       case JsonWebKeyECCurve::P521:
-        throw std::logic_error(
-          fmt::format("Unsupported JWK curve {}", jwk_curve));
+        return CurveID::SECP521R1;
       case JsonWebKeyECCurve::P384:
         return CurveID::SECP384R1;
       case JsonWebKeyECCurve::P256:
@@ -116,6 +117,7 @@ namespace ccf::crypto
       case CurveID::NONE:
       case CurveID::SECP384R1:
       case CurveID::SECP256R1:
+      case CurveID::SECP521R1:
         throw std::logic_error(fmt::format("Invalid EdDSA curve {}", curve_id));
       case CurveID::CURVE25519:
         return JsonWebKeyEdDSACurve::ED25519;

js/ccf-app/src/global.ts

@@ -386,7 +386,8 @@ export interface CCFCrypto {
   /**
    * Generate an ECDSA key pair.
    *
-   * @param curve The name of the curve, one of "secp256r1", "secp384r1".
+   * @param curve The name of the curve, one of "secp256r1", "secp384r1",
+   * "secp521r1".
    */
   generateEcdsaKeyPair(curve: string): CryptoKeyPair;
 

js/ccf-app/test/polyfill.test.ts

@@ -97,6 +97,13 @@ describe("polyfill", function () {
       assert.isTrue(pair.privateKey.startsWith("-----BEGIN PRIVATE KEY-----"));
     });
   });
+  describe("generateEcdsaKeyPair/secp521r1", function () {
+    it("generates a random ECDSA P521R1 key pair", function () {
+      const pair = ccf.crypto.generateEcdsaKeyPair("secp521r1");
+      assert.isTrue(pair.publicKey.startsWith("-----BEGIN PUBLIC KEY-----"));
+      assert.isTrue(pair.privateKey.startsWith("-----BEGIN PRIVATE KEY-----"));
+    });
+  });
   describe("generateEddsaKeyPair/Curve25519", function () {
     it("generates a random EdDSA Curve25519 key pair", function () {
       const pair = ccf.crypto.generateEddsaKeyPair("curve25519");
@@ -584,7 +591,7 @@ describe("polyfill", function () {
   describe("pemToJwk and jwkToPem", function () {
     it("EC", function () {
       const my_kid = "my_kid";
-      const curves = ["secp256r1", "secp384r1"];
+      const curves = ["secp256r1", "secp384r1", "secp521r1"];
       for (const curve of curves) {
         const pair = ccf.crypto.generateEcdsaKeyPair(curve);
         {

samples/constitutions/default/actions.js

@@ -95,6 +95,90 @@ function checkArrayBufferLength(value, min, max, field) {
   }
 }
 
+function checkBase64Url(value, field) {
+  checkType(value, "string", field);
+  if (!/^[A-Za-z0-9_-]+$/.test(value) || value.length % 4 === 1) {
+    throw new Error(`${field} must be base64url encoded`);
+  }
+}
+
+function base64UrlByteLength(value, field) {
+  checkBase64Url(value, field);
+  return Math.floor(value.length / 4) * 3 + [0, 0, 1, 2][value.length % 4];
+}
+
+function splitX509CertBundle(value) {
+  const sep = "-----END CERTIFICATE-----";
+  const items = value.split(sep);
+  if (items.length === 1) {
+    return [];
+  }
+  return items.slice(0, -1).map((p) => p + sep);
+}
+
+function checkX509CACertBundle(value, field) {
+  checkX509CertBundle(value, field);
+  // isValidX509CertChain(target, trusted) is backed by verify_certificate(),
+  // which (a) rejects the call outright if any cert in `trusted` is not a CA
+  // (via X509_check_ca), and (b) enables X509_V_FLAG_PARTIAL_CHAIN so that
+  // intermediate CAs can act as trust anchors. By passing the whole bundle as
+  // `trusted`, we therefore enforce that every cert in the bundle is a CA
+  // certificate, while still accepting bundles containing intermediates as
+  // well as roots.
+  for (const [i, cert] of splitX509CertBundle(value).entries()) {
+    if (!ccf.crypto.isValidX509CertChain(cert, value)) {
+      throw new Error(
+        `${field}[${i}] must be a CA certificate with a currently valid chain to a root within the bundle`,
+      );
+    }
+  }
+}
+
+function checkRsaPublicKey(jwk, field) {
+  checkType(jwk.n, "string", `${field}.n`);
+  checkType(jwk.e, "string", `${field}.e`);
+  if (base64UrlByteLength(jwk.n, `${field}.n`) < 256) {
+    throw new Error(`${field}.n must be at least 2048 bits`);
+  }
+  base64UrlByteLength(jwk.e, `${field}.e`);
+  try {
+    ccf.crypto.pubRsaJwkToPem({
+      kty: "RSA",
+      kid: jwk.kid,
+      n: jwk.n,
+      e: jwk.e,
+    });
+  } catch (e) {
+    throw new Error(`${field} must be a valid RSA public key`);
+  }
+}
+
+function checkEcPublicKey(jwk, field) {
+  checkType(jwk.x, "string", `${field}.x`);
+  checkType(jwk.y, "string", `${field}.y`);
+  checkType(jwk.crv, "string", `${field}.crv`);
+  checkEnum(jwk.crv, ["P-256", "P-384", "P-521"], `${field}.crv`);
+  const coordinateLengths = { "P-256": 32, "P-384": 48, "P-521": 66 };
+  const coordinateLength = coordinateLengths[jwk.crv];
+  if (base64UrlByteLength(jwk.x, `${field}.x`) !== coordinateLength) {
+    throw new Error(`${field}.x must be ${coordinateLength} bytes`);
+  }
+  if (base64UrlByteLength(jwk.y, `${field}.y`) !== coordinateLength) {
+    throw new Error(`${field}.y must be ${coordinateLength} bytes`);
+  }
+  try {
+    ccf.crypto.pubJwkToPem({
+      kty: "EC",
+      kid: jwk.kid,
+      crv: jwk.crv,
+      x: jwk.x,
+      y: jwk.y,
+    });
+  } catch (e) {
+    throw new Error(`${field} must be a valid EC public key`);
+  }
+}
+
 const cpuid_length_bytes = 4;
 function checkValidCpuid(value, field) {
   checkType(value, "string", field);
@@ -174,26 +258,88 @@ function getActiveRecoveryMembersCount() {
 function checkJwks(value, field) {
   checkType(value, "object", field);
   checkType(value.keys, "array", `${field}.keys`);
+  const kids = new Set();
   for (const [i, jwk] of value.keys.entries()) {
+    const keyField = `${field}.keys[${i}]`;
     checkType(jwk.kid, "string", `${field}.keys[${i}].kid`);
+    if (kids.has(jwk.kid)) {
+      throw new Error(`${field}.keys[${i}].kid must be unique`);
+    }
+    kids.add(jwk.kid);
     checkType(jwk.kty, "string", `${field}.keys[${i}].kty`);
+    checkEnum(jwk.kty, ["RSA", "EC"], `${field}.keys[${i}].kty`);
+    if (jwk.use !== undefined) {
+      checkType(jwk.use, "string", `${keyField}.use`);
+      checkEnum(jwk.use, ["sig"], `${keyField}.use`);
+    }
+    if (jwk.alg !== undefined) {
+      checkType(jwk.alg, "string", `${keyField}.alg`);
+      let allowedAlg;
+      if (jwk.kty === "RSA") {
+        allowedAlg = ["RS256"];
+      } else {
+        // Per RFC 7518 section 3.4, EC alg is determined by the curve. When
+        // only x5c is supplied, crv may not be present on the JWK; in that
+        // case allow any of the supported ES* algorithms and rely on the cert
+        // to bind alg to curve.
+        const ecAlgByCrv = {
+          "P-256": "ES256",
+          "P-384": "ES384",
+          "P-521": "ES512",
+        };
+        allowedAlg =
+          jwk.crv && ecAlgByCrv[jwk.crv]
+            ? [ecAlgByCrv[jwk.crv]]
+            : Object.values(ecAlgByCrv);
+      }
+      checkEnum(jwk.alg, allowedAlg, `${keyField}.alg`);
+    }
     if (jwk.x5c) {
       checkArrayLength(jwk.x5c, 1, null, `${field}.keys[${i}].x5c`);
+      let certBundle = "";
       for (const [j, b64der] of jwk.x5c.entries()) {
         checkType(b64der, "string", `${field}.keys[${i}].x5c[${j}]`);
+        if (!/^[A-Za-z0-9+/]+={0,2}$/.test(b64der)) {
+          throw new Error(
+            `${field}.keys[${i}].x5c[${j}] must be base64 encoded`,
+          );
+        }
         const pem =
           "-----BEGIN CERTIFICATE-----\n" +
           b64der +
           "\n-----END CERTIFICATE-----";
         checkX509CertBundle(pem, `${field}.keys[${i}].x5c[${j}]`);
+        certBundle += pem;
+      }
+      const trustedRoot =
+        "-----BEGIN CERTIFICATE-----\n" +
+        jwk.x5c[jwk.x5c.length - 1] +
+        "\n-----END CERTIFICATE-----";
+      if (!ccf.crypto.isValidX509CertChain(certBundle, trustedRoot)) {
+        throw new Error(`${field}.keys[${i}].x5c must chain to its root`);
+      }
+      if (jwk.n !== undefined || jwk.e !== undefined) {
+        if (jwk.kty !== "RSA") {
+          throw new Error(`${field}.keys[${i}].kty must be RSA for n/e keys`);
+        }
+        checkRsaPublicKey(jwk, keyField);
+      }
+      if (jwk.x !== undefined || jwk.y !== undefined || jwk.crv !== undefined) {
+        if (jwk.kty !== "EC") {
+          throw new Error(`${field}.keys[${i}].kty must be EC for x/y keys`);
+        }
+        checkEcPublicKey(jwk, keyField);
       }
     } else if (jwk.n && jwk.e) {
-      checkType(jwk.n, "string", `${field}.keys[${i}].n`);
-      checkType(jwk.e, "string", `${field}.keys[${i}].e`);
+      if (jwk.kty !== "RSA") {
+        throw new Error(`${field}.keys[${i}].kty must be RSA for n/e keys`);
+      }
+      checkRsaPublicKey(jwk, keyField);
     } else if (jwk.x && jwk.y) {
-      checkType(jwk.x, "string", `${field}.keys[${i}].x`);
-      checkType(jwk.y, "string", `${field}.keys[${i}].y`);
-      checkType(jwk.crv, "string", `${field}.keys[${i}].crv`);
+      if (jwk.kty !== "EC") {
+        throw new Error(`${field}.keys[${i}].kty must be EC for x/y keys`);
+      }
+      checkEcPublicKey(jwk, keyField);
     } else {
       throw new Error(
         "JWK must contain either x5c, or n/e for RSA key type, or x/y/crv for EC key type",
@@ -437,7 +583,15 @@ const actions = new Map([
             "Cannot specify a recovery_role value when encryption_pub_key is not specified",
           );
         }
-        // Also check that public encryption key is well formed, if it exists
+        if (
+          args.encryption_pub_key !== null &&
+          args.encryption_pub_key !== undefined
+        ) {
+          checkRsaPublicKey(
+            ccf.crypto.pubRsaPemToJwk(args.encryption_pub_key),
+            "encryption_pub_key",
+          );
+        }
       },
 
       function (args) {
@@ -928,7 +1082,7 @@ const actions = new Map([
     new Action(
       function (args) {
         checkType(args.name, "string", "name");
-        checkX509CertBundle(args.cert_bundle, "cert_bundle");
+        checkX509CACertBundle(args.cert_bundle, "cert_bundle");
       },
       function (args) {
         const name = args.name;
@@ -963,28 +1117,24 @@ const actions = new Map([
         if (args.jwks) {
           checkJwks(args.jwks, "jwks");
         }
+        let url;
+        try {
+          url = parseUrl(args.issuer);
+        } catch (e) {
+          throw new Error("issuer must be a URL");
+        }
+        if (url.scheme != "https" || !url.authority) {
+          throw new Error("issuer must be a URL starting with https://");
+        }
+        if (url.query || url.fragment) {
+          throw new Error("issuer must be a URL without query/fragment");
+        }
         if (args.auto_refresh) {
           if (!args.ca_cert_bundle_name) {
             throw new Error(
               "ca_cert_bundle_name is missing but required if auto_refresh is true",
             );
           }
-          let url;
-          try {
-            url = parseUrl(args.issuer);
-          } catch (e) {
-            throw new Error("issuer must be a URL if auto_refresh is true");
-          }
-          if (url.scheme != "https") {
-            throw new Error(
-              "issuer must be a URL starting with https:// if auto_refresh is true",
-            );
-          }
-          if (url.query || url.fragment) {
-            throw new Error(
-              "issuer must be a URL without query/fragment if auto_refresh is true",
-            );
-          }
         }
       },
       function (args) {