Skip to content

Commit

Permalink
Fix signature verification issues.
Browse files Browse the repository at this point in the history 
**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.
  • Loading branch information
@davidlehn
davidlehn committed Mar 17, 2022
1 parent c20f309 commit 3f0b49a
Show file tree
Hide file tree
Showing 5 changed files with 284 additions and 4 deletions.
38 changes: 38 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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

Expand Down
19 changes: 18 additions & 1 deletion lib/asn1.js
View file
Expand Up @@ -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;
}
Expand All @@ -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;
};

/**
Expand Down
1 change: 1 addition & 0 deletions lib/oids.js
View file
Expand Up @@ -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
Expand Down
80 changes: 77 additions & 3 deletions lib/rsa.js
View file
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 3f0b49a

Please sign in to comment.