crypto: DSA parameter validation in FIPS mode

[stefanmb]

For DSA signatures FIPS 180-4 Section 4.2 requires one of the following well-known parameter combinations: (L, N) pairs must be (1024, 160), (2048, 224), (2048, 256), or (3072, 256). Due to a bug in OpenSSL's error checking and memory allocation using an unacceptable combination will cause OpenSSL to overwrite random uninitialized pointers and it will mostly likely segfault instead of producing a friendly error. To prevent crashes we must do the checks before calling out to OpenSSL.

For the OpenSSL bug, see handling of 'BIGNUM m' in dsa_do_sign():
https://github.com/openssl/openssl/blob/OpenSSL-fips-2_0-dev/crypto/dsa/dsa_ossl.c

This bug has been previously found and reported to OpenSSL here: https://www.mail-archive.com/openssl-dev@openssl.org/msg36826.html

[indutny]

LGTM

[indutny]

cc @nodejs/crypto I would like to have one more LGTM on this, just to be sure

[shigeki]

It is better to add a test to check if an error is thrown when it has a different key length in FIPS mode.

[stefanmb]

@shigeki Do you mean it is better to check if OpenSSL returns an error rather than doing the key length checks in Node?

If so, that is not possible because their error checking is broken and the process will segfault. We have to duplicate these checks inside Node until the bug is fixed in OpenSSL.

[stefanmb]

Removed superfluous newlines and switched to using NODE_FIPS_MODE.

[shigeki]

We can have the test by using a different key length specified in FIPS. For example, 1025bit dsa key (actually 1088 bit) shows the following errors.

var crypto = require('crypto');
var fs = require('fs');

var input = 'hello';

var dsapub = fs.readFileSync('./dsapub_1025.pem');
var dsapri = fs.readFileSync('./dsapri_1025.pem');
var sign = crypto.createSign('DSS1');
sign.update(input);
var signature = sign.sign(dsapri);

var verify = crypto.createVerify('DSS1');
verify.update(input);
console.log(verify.verify(dsapub, signature));

$ ~/github/node_fips/node test2.js
crypto.js:279
  var ret = this._handle.sign(toBuf(key), null, passphrase);
                         ^

Error: PEM_read_bio_PrivateKey failed

[stefanmb]

@shigeki

Done, thank you for the explanation!

[jasnell]

LGTM

[shigeki]

That's fine. LGTM.

[jasnell]

Landed in multiple commits.