Fix signature verification issues.

davidlehn · davidlehn · commit 3f0b49a0573e · 2022-03-17T18:45:51.000-04:00
**SECURITY**: Three RSA PKCS#1 v1.5 signature verification issues were reported by Moosa Yahyazadeh (moosa-yahyazadeh@uiowa.edu): - Leniency in checking `digestAlgorithm` structure can lead to signature forgery. - The code is lenient in checking the digest algorithm structure. This can allow a crafted structure that steals padding bytes and uses unchecked portion of the PKCS#1 encoded message to forge a signature when a low public exponent is being used. - Failing to check tailing garbage bytes can lead to signature forgery. - The code does not check for tailing garbage bytes after decoding a `DigestInfo` ASN.1 structure. This can allow padding bytes to be removed and garbage data added to forge a signature when a low public exponent is being used. - Leniency in checking type octet. - `DigestInfo` is not properly checked for proper ASN.1 structure. This can lead to successful verification with signatures that contain invalid structures but a valid digest. For more information, please see "Bleichenbacher's RSA signature forgery based on implementation error" by Hal Finney: https://mailarchive.ietf.org/arch/msg/openpgp/5rnE9ZRN1AokBVj3VqblGlP63QE/ Fixed with the following: - [asn1] `fromDer` is now more strict and will default to ensuring all input bytes are parsed or throw an error. A new option `parseAllBytes` can disable this behavior. - **NOTE**: The previous behavior is being changed since it can lead to security issues with crafted inputs. It is possible that code doing custom DER parsing may need to adapt to this new behavior and optional flag. - [rsa] Add and use a validator to check for proper structure of parsed ASN.1 `RSASSA-PKCS-v1_5` `DigestInfo` data. Additionally check that the hash algorithm identifier is a known value. An invalid `DigestInfo` or algorithm identifier will now cause an error to be thrown. - [oid] Added `1.2.840.113549.2.2` / `md2` for hash algorithm checking. - [tests] Tests were added for all of the reported issues. A private verify option was added to assist in checking multiple possible failures in the test data.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,8 +3,46 @@ Forge ChangeLog
 
 ## 1.3.0 - 2022-XXX
 
+### Security
+- **SECURITY**: Three RSA PKCS#1 v1.5 signature verification issues were
+  reported by Moosa Yahyazadeh (moosa-yahyazadeh@uiowa.edu).
+  - Leniency in checking `digestAlgorithm` structure can lead to signature
+    forgery.
+    - The code is lenient in checking the digest algorithm structure. This can
+      allow a crafted structure that steals padding bytes and uses unchecked
+      portion of the PKCS#1 encoded message to forge a signature when a low
+      public exponent is being used. For more information, please see
+      ["Bleichenbacher's RSA signature forgery based on implementation
+      error"](https://mailarchive.ietf.org/arch/msg/openpgp/5rnE9ZRN1AokBVj3VqblGlP63QE/)
+      by Hal Finney.
+  - Failing to check tailing garbage bytes can lead to signature forgery.
+    - The code does not check for tailing garbage bytes after decoding a
+      `DigestInfo` ASN.1 structure. This can allow padding bytes to be removed
+      and garbage data added to forge a signature when a low public exponent is
+      being used.  For more information, please see ["Bleichenbacher's RSA
+      signature forgery based on implementation
+      error"](https://mailarchive.ietf.org/arch/msg/openpgp/5rnE9ZRN1AokBVj3VqblGlP63QE/)
+      by Hal Finney.
+  - Leniency in checking type octet.
+    - `DigestInfo` is not properly checked for proper ASN.1 structure. This can
+      lead to successful verification with signatures that contain invalid
+      structures but a valid digest.
+
 ### Fixed
 - [asn1] Add fallback to pretty print invalid UTF8 data.
+- [asn1] `fromDer` is now more strict and will default to ensuring all input
+  bytes are parsed or throw an error. A new option `parseAllBytes` can disable
+  this behavior.
+  - **NOTE**: The previous behavior is being changed since it can lead to
+    security issues with crafted inputs. It is possible that code doing custom
+    DER parsing may need to adapt to this new behavior and optional flag.
+- [rsa] Add and use a validator to check for proper structure of parsed ASN.1
+  `RSASSA-PKCS-v1_5` `DigestInfo` data. Additionally check that the hash
+  algorithm identifier is a known value. An invalid `DigestInfo` or algorithm
+  identifier will now cause an error to be thrown.
+
+### Added
+- [oid] Added `1.2.840.113549.2.2` / `md2` for hash algorithm checking.
 
 ## 1.2.1 - 2022-01-11
 
diff --git a/lib/asn1.js b/lib/asn1.js
@@ -411,31 +411,40 @@ var _getValueLength = function(bytes, remaining) {
  * @param [options] object with options or boolean strict flag
  *          [strict] true to be strict when checking value lengths, false to
  *            allow truncated values (default: true).
+ *          [parseAllBytes] true to ensure all bytes are parsed
+ *            (default: true)
  *          [decodeBitStrings] true to attempt to decode the content of
  *            BIT STRINGs (not OCTET STRINGs) using strict mode. Note that
  *            without schema support to understand the data context this can
  *            erroneously decode values that happen to be valid ASN.1. This
  *            flag will be deprecated or removed as soon as schema support is
  *            available. (default: true)
  *
+ * @throws Will throw an error for various malformed input conditions.
+ *
  * @return the parsed asn1 object.
  */
 asn1.fromDer = function(bytes, options) {
   if(options === undefined) {
     options = {
       strict: true,
+      parseAllBytes: true,
       decodeBitStrings: true
     };
   }
   if(typeof options === 'boolean') {
     options = {
       strict: options,
+      parseAllBytes: true,
       decodeBitStrings: true
     };
   }
   if(!('strict' in options)) {
     options.strict = true;
   }
+  if(!('parseAllBytes' in options)) {
+    options.parseAllBytes = true;
+  }
   if(!('decodeBitStrings' in options)) {
     options.decodeBitStrings = true;
   }
@@ -445,7 +454,15 @@ asn1.fromDer = function(bytes, options) {
     bytes = forge.util.createBuffer(bytes);
   }
 
-  return _fromDer(bytes, bytes.length(), 0, options);
+  var byteCount = bytes.length();
+  var value = _fromDer(bytes, bytes.length(), 0, options);
+  if(options.parseAllBytes && bytes.length() !== 0) {
+    var error = new Error('Unparsed DER bytes remain after ASN.1 parsing.');
+    error.byteCount = byteCount;
+    error.remaining = bytes.length();
+    throw error;
+  }
+  return value;
 };
 
 /**
diff --git a/lib/oids.js b/lib/oids.js
@@ -47,6 +47,7 @@ _IN('1.3.14.3.2.29', 'sha1WithRSASignature');
 _IN('2.16.840.1.101.3.4.2.1', 'sha256');
 _IN('2.16.840.1.101.3.4.2.2', 'sha384');
 _IN('2.16.840.1.101.3.4.2.3', 'sha512');
+_IN('1.2.840.113549.2.2', 'md2');
 _IN('1.2.840.113549.2.5', 'md5');
 
 // pkcs#7 content types
diff --git a/lib/rsa.js b/lib/rsa.js
@@ -264,6 +264,40 @@ var publicKeyValidator = forge.pki.rsa.publicKeyValidator = {
   }]
 };
 
+// validator for a DigestInfo structure
+var digestInfoValidator = {
+  name: 'DigestInfo',
+  tagClass: asn1.Class.UNIVERSAL,
+  type: asn1.Type.SEQUENCE,
+  constructed: true,
+  value: [{
+    name: 'DigestInfo.DigestAlgorithm',
+    tagClass: asn1.Class.UNIVERSAL,
+    type: asn1.Type.SEQUENCE,
+    constructed: true,
+    value: [{
+      name: 'DigestInfo.DigestAlgorithm.algorithmIdentifier',
+      tagClass: asn1.Class.UNIVERSAL,
+      type: asn1.Type.OID,
+      constructed: false,
+      capture: 'algorithmIdentifier'
+    }, {
+      // NULL paramters
+      name: 'DigestInfo.DigestAlgorithm.parameters',
+      tagClass: asn1.Class.UNIVERSAL,
+      type: asn1.Type.NULL,
+      constructed: false
+    }]
+  }, {
+    // digest
+    name: 'DigestInfo.digest',
+    tagClass: asn1.Class.UNIVERSAL,
+    type: asn1.Type.OCTETSTRING,
+    constructed: false,
+    capture: 'digest'
+  }]
+};
+
 /**
  * Wrap digest in DigestInfo object.
  *
@@ -1092,25 +1126,65 @@ pki.setRsaPublicKey = pki.rsa.setPublicKey = function(n, e) {
    *          a Forge PSS object for RSASSA-PSS,
    *          'NONE' or null for none, DigestInfo will not be expected, but
    *            PKCS#1 v1.5 padding will still be used.
+   * @param options optional verify options
+   *          _parseAllDigestBytes testing flag to control parsing of all
+   *            digest bytes. Unsupported and not for general usage.
+   *            (default: true)
    *
    * @return true if the signature was verified, false if not.
    */
-  key.verify = function(digest, signature, scheme) {
+  key.verify = function(digest, signature, scheme, options) {
     if(typeof scheme === 'string') {
       scheme = scheme.toUpperCase();
     } else if(scheme === undefined) {
       scheme = 'RSASSA-PKCS1-V1_5';
     }
+    if(options === undefined) {
+      options = {
+        _parseAllDigestBytes: true
+      };
+    }
+    if(!('_parseAllDigestBytes' in options)) {
+      options._parseAllDigestBytes = true;
+    }
 
     if(scheme === 'RSASSA-PKCS1-V1_5') {
       scheme = {
         verify: function(digest, d) {
           // remove padding
           d = _decodePkcs1_v1_5(d, key, true);
           // d is ASN.1 BER-encoded DigestInfo
-          var obj = asn1.fromDer(d);
+          var obj = asn1.fromDer(d, {
+            parseAllBytes: options._parseAllDigestBytes
+          });
+
+          // validate DigestInfo
+          var capture = {};
+          var errors = [];
+          if(!asn1.validate(obj, digestInfoValidator, capture, errors)) {
+            var error = new Error(
+              'ASN.1 object does not contain a valid RSASSA-PKCS1-v1_5 ' +
+              'DigestInfo value.');
+            error.errors = errors;
+            throw error;
+          }
+          // check hash algorithm identifier
+          // FIXME: add support to vaidator for strict value choices
+          var oid = asn1.derToOid(capture.algorithmIdentifier);
+          if(!(oid === forge.oids.md2 ||
+            oid === forge.oids.md5 ||
+            oid === forge.oids.sha1 ||
+            oid === forge.oids.sha256 ||
+            oid === forge.oids.sha384 ||
+            oid === forge.oids.sha512)) {
+            var error = new Error(
+              'Unknown RSASSA-PKCS1-v1_5 DigestAlgorithm identifier.');
+            error.oid = oid;
+            throw error;
+          }
+
           // compare the given digest to the decrypted one
-          return digest === obj.value[1].value;
+          return digest === capture.digest;
         }
       };
     } else if(scheme === 'NONE' || scheme === 'NULL' || scheme === null) {
diff --git a/tests/unit/rsa.js b/tests/unit/rsa.js
