node:crypto RSA-PSS seems to be broken

[DuAnto]

Version

v21.6.2

Platform

Linux host 6.6.19-1-MANJARO #1 SMP PREEMPT_DYNAMIC Fri Mar 1 18:16:16 UTC 2024 x86_64 GNU/Linux

Subsystem

node:crypto

What steps will reproduce the bug?

I was experimenting with digital signatures and found that RSA-PSS signatures does not work as expected. I was running jest tests during my experiments and got stuck in a message saying:

Fatal error in HandleScope::HandleScope

Entering the V8 API without proper locking in place

Efficiently, i was running test containing following lines:

import * as MyCrypto from "~/core/crypto"
describe(`Built-in crypto implementation` , () =>
{
    test.each( MyCrypto.Signing.knownSchemes() )(`Key generation (%p)` , async (algo) =>
    {
        const scheme  = MyCrypto.Signing.implementation( algo );
        const keypair = await scheme.generateRawKey();

        expect(keypair.privateKey.type).toBe('private');
        expect(keypair.publicKey.type).toBe('public');

        const data = Buffer.from("hello,world!")
        const q = await scheme.makeSignature(  keypair.privateKey , data );
        console.log(`Signing via ${algo} yields ` , q );
    });
});

And "~/core/crypto.ts" contain effectiely (excluding lot of commented-out lines):

import * as Crypto from "node:crypto";
export type SupportedRsaModulusSizes  = 4096 /*| 6144 | 8192*/
export const defaultRsaPublicExponent = new Uint8Array([1, 0, 1]);
export const defaultRsaModulusLength : SupportedRsaModulusSizes = 4096;

export interface SignatureScheme
{
    name : string
    generateRawKey( modulusLength ?: number ) : Promise<CryptoKeyPair>;
    makeSignature(  key : CryptoKey , data : BufferSource ) : Promise<ArrayBuffer>;
    checkSignature( key : CryptoKey , data : BufferSource , sign : BufferSource ) : Promise<boolean>;
}

export class Signing
{
    private static _presets : Record<string , SignatureScheme> = {}

    static implementation( name : string ) : SignatureScheme
    {
        const  impl = this._presets[name];
        if( ! impl )
        {
            const e = new Error(`Unknown SignatureScheme (${name})`)
            e.name = "UnknownSignatureScheme"
            throw e;
        }

        return impl;
    }

    static knownSchemes() { return Object.keys(this._presets) }
    static registerSignatureScheme( name : string , impl : SignatureScheme )
    {
        this._presets[ name ] = impl;
    }
}

(function export_rsa_pss()
{
    type RsaPssModulusSizes  = /*4096 | 6144 |*/ 8192
    type RsaPssAlgorithms   = "rsa_pss_sha256"   | "rsa_pss_sha384"   | "rsa_pss_sha512"
    type RsaPssQualifiedAlgorithms  =  `${RsaPssAlgorithms}-${RsaPssModulusSizes}`

    class RsaPss implements SignatureScheme
    {
        private static  _presets : Record<RsaPssQualifiedAlgorithms , Pick<RsaHashedKeyGenParams,"hash"> & RsaPssParams > = {
            // "rsa_pss_sha256-4096" : {hash:"SHA-256", name:"RSA-PSS", saltLength: 256} ,
            // "rsa_pss_sha256-6144" : {hash:"SHA-256", name:"RSA-PSS", saltLength: 256} ,
            "rsa_pss_sha256-8192" : {hash:"SHA-256", name:"RSA-PSS", saltLength: 256} ,
            // "rsa_pss_sha384-4096" : {hash:"SHA-384", name:"RSA-PSS", saltLength: 384} ,
            // "rsa_pss_sha384-6144" : {hash:"SHA-384", name:"RSA-PSS", saltLength: 384} ,
            "rsa_pss_sha384-8192" : {hash:"SHA-384", name:"RSA-PSS", saltLength: 384} ,
            // "rsa_pss_sha512-4096" : {hash:"SHA-512", name:"RSA-PSS", saltLength: 512} ,
            // "rsa_pss_sha512-6144" : {hash:"SHA-512", name:"RSA-PSS", saltLength: 512} ,
            "rsa_pss_sha512-8192" : {hash:"SHA-512", name:"RSA-PSS", saltLength: 512} ,
        }

        readonly name  : RsaPssQualifiedAlgorithms;
        generateRawKey = (modulusLength = defaultRsaModulusLength) =>
        {
            const params : RsaHashedKeyGenParams = {
                name :           'RSA-PSS' ,
                hash :           RsaPss._presets[this.name].hash,
                publicExponent:  defaultRsaPublicExponent,
                modulusLength :  modulusLength
            }
    
            return Crypto.subtle.generateKey( params , true , ["sign" , "verify"] );
        }
        makeSignature(key: CryptoKey, data: BufferSource): Promise<ArrayBuffer>
        {
            return Crypto.subtle.sign( RsaPss._presets[this.name] , key , data );
        }
        checkSignature(key: CryptoKey, data: BufferSource, sign: BufferSource): Promise<boolean>
        {
            return Crypto.subtle.verify( RsaPss._presets[this.name] , key , sign , data );
        }
        static knownAlgorithmNames = () => Object.keys(RsaPss._presets) as RsaPssQualifiedAlgorithms[]
        constructor( variant : RsaPssQualifiedAlgorithms )
        {
            this.name  = variant;
        }
    }


    for( const n  of RsaPss.knownAlgorithmNames() )
    {
        Signing.registerSignatureScheme( n , new RsaPss(n) )
    }
})();

As a result of such launch I see following in a terminal emulator window:

[user@hostname server]$ npm run test:exec -- tests/operations/liman.crypto.test.ts

LiMan@1.0.0 test:exec
NODE_ENV=example jest -i tests/operations/liman.crypto.test.ts

console.log
Signing via rsa_pss_sha256-8192 yields ArrayBuffer {
[Uint8Contents]: <30 4a e9 2c 56 1a 01 6c 74 19 53 c2 4b 82 9f 79 cf 08 e4 4f 82 31 8c e7 cb e6 1b 74 9d 31 ea 71 49 2d 3f d9 fc 44 9a cf 44 bd b6 38 e7 bd 4f 2a 18 8d ea 61 6c d4 3a 7c 68 d1 0e ce 51 aa e4 3a ad 6e 95 af 5d 38 39 72 04 9a 5e 1d 6a 2d 9a 72 a8 a2 98 89 43 be ed ca 42 cb 72 69 65 91 13 98 08 78 c6 c8 ... 412 more bytes>,
byteLength: 512
}

  at tests/operations/liman.crypto.test.ts:109:17

console.log
Signing via rsa_pss_sha384-8192 yields ArrayBuffer {
[Uint8Contents]: <77 26 0c ff fd 2d aa fb 52 43 95 1e 8a 77 59 1e 0a 2f d6 d4 6a 9c 18 43 65 67 38 af 68 40 25 a1 06 c4 23 94 c6 e7 94 02 5a f7 c2 69 a0 71 e8 17 6d 97 78 f9 b9 62 51 14 0b 85 e2 b9 26 c1 a8 78 7c b2 66 4b cd 92 a2 19 e0 cc b9 75 1f dd 76 e2 a5 db f3 05 19 43 94 ef 73 7e 5b 61 8e d6 6e 29 20 67 05 5a ... 412 more bytes>,
byteLength: 512
}

  at tests/operations/liman.crypto.test.ts:109:17

RUNS tests/operations/liman.crypto.test.ts

Fatal error in HandleScope::HandleScope

Entering the V8 API without proper locking in place

[user@hostname server]$

How often does it reproduce? Is there a required condition?

At my host and project is reproduces constantly

What is the expected behavior? Why is that the expected behavior?

Expected behavior is to see a message that signature has been created. Why? Just because it has been asked to do that.

What do you see instead?

As it was pointer above, I see an error message. And, also notification that core has been dumped.
Again, following message is diplayed (entire output of a run you'll find above):

Fatal error in HandleScope::HandleScope

Entering the V8 API without proper locking in place

Additional information

No response

[panva]

I can reproduce the abort, probably something we can add validations for.

The reason is that the saltLength you're asking for exceeds the maximum possible for the operation.

You're asking for 512 bytes, maximum possible salt length is signature length - digest output length - 2 in bytes, so for your setup > 512 - 64 - 2 > 446.

I assume you intended to have saltLength equal to the length of the output of the hash function, as is the convention, but the value of saltLength is supposed to be in bytes, not bits.

[panva]

There's even a TODO from @jasnell to add this validation

node/lib/internal/crypto/rsa.js

Lines 313 to 316 in 511be6c

	// TODO(@jasnell): Validate maximum size of saltLength
	// based on the key size:
	// Math.ceil((keySizeInBits - 1)/8) - digestSizeInBytes - 2
	validateInt32(saltLength, 'algorithm.saltLength', -2);

[DuAnto]

Thank you for a response.

You are absolutely correct about my intention to use hash size as a salt length.

And special thanks for pointing that

You're asking for 512 bytes, maximum possible salt length is signature length - digest output length - 2 in bytes, so for your setup > 512 - 64 - 2 > 446.

as it helped me do discover my forgiveness (I've forgot to ignore modulusLength argument that will be made unneccessary during refactoring).

As to published fix, I have two suggestions.. At first, in my opinion, exception message shall point measurement units; most of cryptographic parameters are expressed in terms of bits (e.g. key sizes), but salt length gets measured in terms of bytes, hence message pointing valid length range may be misinterpreted.

Another suggestion relates to types definitions. There are no comments at all; it would be nice to see these - in the same project I am using TypeORM providing those.

P.S. Irrespective of my suggestions, I think issue shall be closed - uncaught exception is much better that server process termination.