Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Ed25519 and ES256K Test Vectors and Ensure Low-S ECDSA Signatures #375

Merged
merged 2 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/crypto-aws-kms/src/ecdsa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ export class EcdsaAlgorithm implements
* Hashing the data first ensures that the input to the signing operation is within this limit,
* regardless of the original data size.
*
* Note: The signature returned is normalized to low-S to prevent signature malleability. This
* ensures that the signature can be verified by other libraries that enforce strict verification.
* More information on signature malleability can be found
* {@link @web5/crypto#Secp256k1.adjustSignatureToLowS | here}.
*
* @example
* ```ts
* const ecdsa = new EcdsaAlgorithm({ crypto, kmsClient });
Expand Down Expand Up @@ -214,7 +219,10 @@ export class EcdsaAlgorithm implements
const derSignature = response.Signature;

// Convert the DER encoded signature to a compact R+S signature.
const signature = await Secp256k1.convertDerToCompactSignature({ derSignature });
let signature = await Secp256k1.convertDerToCompactSignature({ derSignature });

// Ensure the signature is in low-S, normalized form to prevent signature malleability.
signature = await Secp256k1.adjustSignatureToLowS({ signature });

return signature;
}
Expand Down
26 changes: 25 additions & 1 deletion packages/crypto-aws-kms/tests/ecdsa.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import type { Jwk } from '@web5/crypto';

import sinon from 'sinon';
import { expect } from 'chai';
import { Convert } from '@web5/common';
import { CreateKeyCommand, DescribeKeyCommand, KMSClient, SignCommand } from '@aws-sdk/client-kms';

import { AwsKmsCrypto } from '../src/api.js';
import { EcdsaAlgorithm } from '../src/ecdsa.js';
import { mockEcdsaSecp256k1 } from './fixtures/mock-ecdsa-secp256k1.js';
import { mockEcdsaSecp256k1, mockSignCommandOutput } from './fixtures/mock-ecdsa-secp256k1.js';

describe('EcdsaAlgorithm', () => {
let crypto: AwsKmsCrypto;
Expand Down Expand Up @@ -85,6 +86,29 @@ describe('EcdsaAlgorithm', () => {
expect(kmsClientStub.send.calledTwice).to.be.true;
});

it('returns normalized, low-s form signatures', async () => {
// Setup.
const mockHighSSignCommandOutput = {
...mockSignCommandOutput,
// Return the DER encoded signature from Wycheproof test case 1, which has a high-s value.
signature: Convert.hex('3046022100813ef79ccefa9a56f7ba805f0e478584fe5f0dd5f567bc09b5123ccbc9832365022100900e75ad233fcc908509dbff5922647db37c21f4afd3203ae8dc4ae7794b0f87').toUint8Array()
};
kmsClientStub.send.withArgs(sinon.match.instanceOf(SignCommand)).resolves(mockHighSSignCommandOutput);
kmsClientStub.send.withArgs(sinon.match.instanceOf(DescribeKeyCommand)).resolves(mockEcdsaSecp256k1.getKeySpec.output);

// Test the method.
const signature = await ecdsa.sign({
algorithm : 'ES256K',
data : new Uint8Array([0, 1, 2, 3, 4]),
keyUri : 'urn:jwk:U01_M3_A9vMLOWixG-rlfC-_f3LLdurttn7c7d3_upU'
});

// Validate the signature returned by EcdsaAlgorithm.sign() has been adjust to low-s form.
expect(signature).to.deep.equal(
Convert.hex('8891914c431baae682defc57fe074c8cb700f790d72e2a51474cca0ee00faa8451e462395b70b51ae6b98d3fadc233ae4db15583e9b75ac32d94a697c71aa426').toUint8Array()
);
});

it('throws an error if an unsupported algorithm is specified', async () => {
// Setup.
const keyUri = 'urn:jwk:U01_M3_A9vMLOWixG-rlfC-_f3LLdurttn7c7d3_upU';
Expand Down
8 changes: 4 additions & 4 deletions packages/crypto/.c8rc.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
".js"
],
"include": [
"tests/compiled/src/**"
"tests/compiled/**/src/**"
],
"exclude": [
"tests/compiled/src/index.js",
"tests/compiled/src/types.js",
"tests/compiled/src/types/**"
"tests/compiled/**/src/index.js",
"tests/compiled/**/src/types.js",
"tests/compiled/**/src/types/**"
],
"reporter": [
"cobertura",
Expand Down
70 changes: 70 additions & 0 deletions packages/crypto/src/primitives/secp256k1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,76 @@ import { computeJwkThumbprint, isEcPrivateJwk, isEcPublicJwk } from '../jose/jwk
* ```
*/
export class Secp256k1 {
/**
* Adjusts an ECDSA signature to a normalized, low-S form.
*
* @remarks
* All ECDSA signatures, regardless of the curve, consist of two components, `r` and `s`, both of
* which are integers. The curve's order (the total number of points on the curve) is denoted by
* `n`. In a valid ECDSA signature, both `r` and `s` must be in the range [1, n-1]. However, due
* to the mathematical properties of ECDSA, if `(r, s)` is a valid signature, then `(r, n - s)` is
* also a valid signature for the same message and public key. In other words, for every
* signature, there's a "mirror" signature that's equally valid. For these elliptic curves:
*
* - Low S Signature: A signature where the `s` component is in the lower half of the range,
* specifically less than or equal to `n/2`.
*
* - High S Signature: This is where the `s` component is in the upper half of the range, greater
* than `n/2`.
*
* The practical implication is that a third-party can forge a second valid signature for the same
* message by negating the `s` component of the original signature, without any knowledge of the
* private key. This is known as a "signature malleability" attack.
*
* This type of forgery is not a problem in all systems, but it can be an issue in systems that
* rely on digital signature uniqueness to ensure transaction integrity. For example, in Bitcoin,
* transaction malleability is an issue because it allows for the modification of transaction
* identifiers (and potentially, transactions themselves) after they're signed but before they're
* confirmed in a block. By enforcing low `s` values, the Bitcoin network reduces the likelihood of
* this occurring, making the system more secure and predictable.
*
* For this reason, it's common practice to normalize ECDSA signatures to a low-S form. This
* form is considered standard and preferable in some systems and is known as the "normalized"
* form of the signature.
*
* This method takes a signature, and if it's high-S, returns the normalized low-S form. If the
* signature is already low-S, it's returned unmodified. It's important to note that this
* method does not change the validity of the signature but makes it compliant with systems that
* enforce low-S signatures.
*
* @example
* ```ts
* const signature = new Uint8Array([...]); // Your ECDSA signature
* const adjustedSignature = await Secp256k1.adjustSignatureToLowS({ signature });
* // Now 'adjustedSignature' is in the low-S form.
* ```
*
* @param params - The parameters for the signature adjustment.
* @param params.signature - The ECDSA signature as a `Uint8Array`.
*
* @returns A Promise that resolves to the adjusted signature in low-S form as a `Uint8Array`.
*/
public static async adjustSignatureToLowS({ signature }: {
signature: Uint8Array;
}): Promise<Uint8Array> {
// Convert the signature to a `secp256k1.Signature` object.
const signatureObject = secp256k1.Signature.fromCompact(signature);

if (signatureObject.hasHighS()) {
// Adjust the signature to low-S format if it's high-S.
const adjustedSignatureObject = signatureObject.normalizeS();

// Convert the adjusted signature object back to a byte array.
const adjustedSignature = adjustedSignatureObject.toCompactRawBytes();

return adjustedSignature;

} else {
// Return the unmodified signature if it is already in low-S format.
return signature;
}
}

/**
* Converts a raw private key in bytes to its corresponding JSON Web Key (JWK) format.
*
Expand Down
112 changes: 36 additions & 76 deletions packages/crypto/tests/primitives/ed25519.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import chaiAsPromised from 'chai-as-promised';

import type { Jwk, JwkParamsOkpPrivate } from '../../src/jose/jwk.js';

import ed25519Sign from '../fixtures/test-vectors/ed25519/sign.json' assert { type: 'json' };
import ed25519Verify from '../fixtures/test-vectors/ed25519/verify.json' assert { type: 'json' };
import CryptoEd25519SignTestVector from '../../../../test-vectors/crypto_ed25519/sign.json' assert { type: 'json' };
import ed25519ComputePublicKey from '../fixtures/test-vectors/ed25519/compute-public-key.json' assert { type: 'json' };
import CryptoEd25519VerifyTestVector from '../../../../test-vectors/crypto_ed25519/verify.json' assert { type: 'json' };
import ed25519BytesToPublicKey from '../fixtures/test-vectors/ed25519/bytes-to-public-key.json' assert { type: 'json' };
import ed25519PublicKeyToBytes from '../fixtures/test-vectors/ed25519/public-key-to-bytes.json' assert { type: 'json' };
import ed25519BytesToPrivateKey from '../fixtures/test-vectors/ed25519/bytes-to-private-key.json' assert { type: 'json' };
Expand Down Expand Up @@ -315,17 +315,24 @@ describe('Ed25519', () => {
expect(signature).to.be.instanceOf(Uint8Array);
});

for (const vector of ed25519Sign.vectors) {
it(vector.description, async () => {
const signature = await Ed25519.sign({
key : vector.input.key as Jwk,
data : Convert.hex(vector.input.data).toUint8Array()
});

const signatureHex = Convert.uint8Array(signature).toHex();
expect(signatureHex).to.deep.equal(vector.output);
describe('Web5TestVectorsCryptoEd25519', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the other examples are 1 level deep:

describe('PresentationExchange', () -> describe('Web5TestVectorsPresentationExchange', () 
describe('Verifiable Credential Tests', () -> describe('Web5TestVectorsCredentials', ()

yours are 2 levels deep

describe('Ed25519', () -> describe('verify()', () -> describe('Web5TestVectorsCryptoEd25519', ()

I -think- it should work and get to the checkmark screen, but if it doesn't then this will probably be the reason.

I think we should put all vector test in one file eventually for better ease of use, but we can discuss that offline

Copy link
Contributor

@finn-block finn-block Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

junit xml doesnt have a way to do more than one class deep as far as i know, so the mocha junit reporter thing will just smush it down, which may or may not pass the regex. Probably best to just keep doing it the way we've established works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give it a shot and see how it does.

I had the same thought but the reason I ended up nesting them is that, otherwise, it isn't clear by looking at the tests that those methods are being rigorously tested.

it('sign', async () => {
for (const vector of CryptoEd25519SignTestVector.vectors) {
let errorOccurred = false;
try {
const signature = await Ed25519.sign({
key : vector.input.key as Jwk,
data : Convert.hex(vector.input.data).toUint8Array()
});

const signatureHex = Convert.uint8Array(signature).toHex();
expect(signatureHex).to.deep.equal(vector.output, vector.description);

} catch { errorOccurred = true; }
expect(errorOccurred).to.equal(vector.errors, `Expected '${vector.description}' to${vector.errors ? ' ' : ' not '}throw an error`);
}
});
}
});
});

describe('validatePublicKey()', () => {
Expand Down Expand Up @@ -368,70 +375,23 @@ describe('Ed25519', () => {
expect(isValid).to.be.true;
});

it('returns false if the signed data was mutated', async () => {
const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
let isValid: boolean;

// Generate signature using the private key.
const signature = await Ed25519.sign({ key: privateKey, data });

// Verification should return true with the data used to generate the signature.
isValid = await Ed25519.verify({ key: publicKey, signature, data });
expect(isValid).to.be.true;

// Make a copy and flip the least significant bit (the rightmost bit) in the first byte of the array.
const mutatedData = new Uint8Array(data);
mutatedData[0] ^= 1 << 0;

// Verification should return false if the given data does not match the data used to generate the signature.
isValid = await Ed25519.verify({ key: publicKey, signature, data: mutatedData });
expect(isValid).to.be.false;
});

it('returns false if the signature was mutated', async () => {
const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
let isValid: boolean;

// Generate a new private key and get its public key.
privateKey = await Ed25519.generateKey();
publicKey = await Ed25519.computePublicKey({ key: privateKey });

// Generate signature using the private key.
const signature = await Ed25519.sign({ key: privateKey, data });

// Make a copy and flip the least significant bit (the rightmost bit) in the first byte of the array.
const mutatedSignature = new Uint8Array(signature);
mutatedSignature[0] ^= 1 << 0;

// Verification should return false if the signature was modified.
isValid = await Ed25519.verify({ key: publicKey, signature: signature, data: mutatedSignature });
expect(isValid).to.be.false;
});

it('returns false with a signature generated using a different private key', async () => {
const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
const privateKeyA = await Ed25519.generateKey();
const publicKeyA = await Ed25519.computePublicKey({ key: privateKeyA });
const privateKeyB = await Ed25519.generateKey();
let isValid: boolean;

// Generate a signature using private key B.
const signatureB = await Ed25519.sign({ key: privateKeyB, data });

// Verification should return false with the public key from private key A.
isValid = await Ed25519.verify({ key: publicKeyA, signature: signatureB, data });
expect(isValid).to.be.false;
});

for (const vector of ed25519Verify.vectors) {
it(vector.description, async () => {
const isValid = await Ed25519.verify({
key : vector.input.key as Jwk,
signature : Convert.hex(vector.input.signature).toUint8Array(),
data : Convert.hex(vector.input.data).toUint8Array()
});
expect(isValid).to.equal(vector.output);
describe('Web5TestVectorsCryptoEd25519', () => {
it('verify', async () => {
for (const vector of CryptoEd25519VerifyTestVector.vectors) {
let errorOccurred = false;
try {
const isValid = await Ed25519.verify({
key : vector.input.key as Jwk,
signature : Convert.hex(vector.input.signature).toUint8Array(),
data : Convert.hex(vector.input.data).toUint8Array()
});

expect(isValid).to.equal(vector.output);

} catch { errorOccurred = true; }
expect(errorOccurred).to.equal(vector.errors, `Expected '${vector.description}' to${vector.errors ? ' ' : ' not '}throw an error`);
}
});
}
});
});
});
Loading