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.

Sign up for GitHub

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

feat: add validation for public key #2841

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add validation for public key #2841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a76ce1
feat: add validation for public key
ivaylonikolov7 Feb 3, 2025
c028b1b
refactor: use curve order of ecdsa and ed25519 for tests
ivaylonikolov7 Feb 4, 2025
3171ba4
chore: const order
ivaylonikolov7 Feb 4, 2025
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
1 change: 1 addition & 0 deletions packages/cryptography/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"last 1 Firefox versions"
],
"dependencies": {
"@noble/curves": "^1.8.1",
"asn1js": "^3.0.5",
"bignumber.js": "^9.1.1",
"bn.js": "^5.2.1",
Expand Down
17 changes: 17 additions & 0 deletions packages/cryptography/pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 33 additions & 2 deletions packages/cryptography/src/EcdsaPublicKey.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { arrayEqual } from "./util/array.js";
import * as hex from "./encoding/hex.js";
import * as ecdsa from "./primitive/ecdsa.js";
import { keccak256 } from "./primitive/keccak.js";
import { secp256k1 } from "@noble/curves/secp256k1";
import elliptic from "elliptic";
const ec = new elliptic.ec("secp256k1");

Expand Down Expand Up @@ -92,7 +93,8 @@ export default class EcdsaPublicKey extends Key {
`cannot decode ECDSA private key data from DER format`,
);
}
return new EcdsaPublicKey(ecdsaPublicKeyBytes);

return EcdsaPublicKey.fromBytesRaw(ecdsaPublicKeyBytes);
}

/**
Expand All @@ -106,7 +108,36 @@ export default class EcdsaPublicKey extends Key {
);
}

return new EcdsaPublicKey(data);
if (this.isValid(data)) {
return new EcdsaPublicKey(data);
} else {
throw new BadKeyError("invalid public key");
}
}

/**
* @param {Uint8Array} data
* @returns {boolean}
*/
static isValid(data) {
const EMPTY_BUFFER_33_BYTES = Buffer.from(new Uint8Array(33).fill(0));
const EMPTY_BUFFER_65_BYTES = Buffer.from(new Uint8Array(65).fill(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need those buffers in memory to do the following checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a little more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean do you need to allocate those buffers and save them in variables to do the checks? Isn't there some sort of faster check for this without having EMPTY_BUFFER_33_BYTES, EMPTY_BUFFER_65_BYTES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use the Buffer.alloc(33) without allocating it to a const I am not sure if it's descriptive enough for the next dev who's going to read it tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave it up to you it seemed better to me.

const bufferData = Buffer.from(data);

// SEE HIP-540
if (
EMPTY_BUFFER_33_BYTES.equals(bufferData) ||
EMPTY_BUFFER_65_BYTES.equals(bufferData)
) {
return true;
}

try {
secp256k1.ProjectivePoint.fromHex(data);
return true;
} catch (error) {
return false;
}
}

/**
Expand Down
28 changes: 26 additions & 2 deletions packages/cryptography/src/Ed25519PublicKey.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import nacl from "tweetnacl";
import { arrayEqual } from "./util/array.js";
import * as hex from "./encoding/hex.js";
import forge from "node-forge";
import { ed25519 } from "@noble/curves/ed25519";

const derPrefix = "302a300506032b6570032100";
const derPrefixBytes = hex.decode(derPrefix);
Expand Down Expand Up @@ -77,7 +78,7 @@ export default class Ed25519PublicKey extends Key {
);
}

return new Ed25519PublicKey(publicKey);
return Ed25519PublicKey.fromBytesRaw(publicKey);
}

/**
Expand All @@ -91,7 +92,11 @@ export default class Ed25519PublicKey extends Key {
);
}

return new Ed25519PublicKey(data);
if (this.isValid(data)) {
return new Ed25519PublicKey(data);
} else {
throw new BadKeyError("invalid public key");
}
}

/**
Expand All @@ -106,6 +111,25 @@ export default class Ed25519PublicKey extends Key {
return Ed25519PublicKey.fromBytes(hex.decode(text));
}

/**
* @param {Uint8Array} data
* @returns {boolean}
*/
static isValid(data) {
if (!data) return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this check if data is empty? If it does would make it more verbose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if you have this here why not have it in the other public key class as well?


const EMPTY_BUFFER = Buffer.from(new Uint8Array(33).fill(0));
const bufferData = Buffer.from(data);
if (bufferData.equals(EMPTY_BUFFER)) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seem like some redundant operations to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

Copy link
Contributor

Choose a reason for hiding this comment

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

if (Buffer.from(data).equals(Buffer.alloc(33))) return true;


try {
ed25519.ExtendedPoint.fromHex(data);
return true;
} catch {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

If an error is caught wouldn't it make sense to propagate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think considering the method's name is isValidKey you would expect it to return true or false.

Copy link
Contributor

Choose a reason for hiding this comment

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

throw new BadKeyError("invalid public key"); is thrown in another layer. The message of the error for the key parse would be useful there imho. I know you've defined it so but that error could get up there somehow

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this method is created in this PR, we can ditch it and do the validation in the fromBytesRaw func. There we can throw new BadKeyError("invalid public key: ", e);, giving more info to the user as Georgi said.

}
}

/**
* Verify a signature on a message with this public key.
* @param {Uint8Array} message
Expand Down
40 changes: 40 additions & 0 deletions test/unit/PublicKey.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,44 @@ describe("PublicKey", function () {
const ed25519PublicKey1 = PublicKey.fromString(PUBLIC_KEY_DER1);
expect(ed25519PublicKey1.toStringRaw()).to.be.equal(PUBLIC_KEY1);
});

it("should throw error when creating ED25519 key from invalid bytes", function () {
// Test cases with invalid byte
const invalidPrivateKeyBytes = Uint8Array.from([
0xca, 0x35, 0x4b, 0x7c, 0xf4, 0x87, 0xd1, 0xbc, 0x43, 0x5a, 0x25,
0x66, 0x77, 0x09, 0xc1, 0xab, 0x98, 0x0c, 0x11, 0x4d, 0x35, 0x94,
0xe6, 0x25, 0x9e, 0x81, 0x2e, 0x6a, 0x70, 0x3d, 0x4f, 0x51,
]);

expect(() => {
PublicKey.fromBytesED25519(invalidPrivateKeyBytes);
}).to.throw;
});

it("should throw error when creating ECDSA key from invalid bytes", function () {
// Test cases with invalid byte
const invalidPrivateKeyBytes = Uint8Array.from([
0x00, 0xca, 0x35, 0x4b, 0x7c, 0xf4, 0x87, 0xd1, 0xbc, 0x43, 0x5a,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that padding the zero byte makes the key invalid?

Copy link
Contributor Author

@ivaylonikolov7 ivaylonikolov7 Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I copied the invalid bytes from the Java SDK tests. I think the first byte checks if the key is compressed or not (0x02 or 0x03). Therefore 0x00 making it invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is really a question of convention:
048ae371ee33535a3fa40e31c20485cd8bf869c151184c56cae2775b0156eb3a4b29421847d57caf98b271ab954a42bfb1bed565a9c238b2f7b30274cbd793a163 - also a valid key.
The bad thing about conventions around this topic is that many would trim the 0x02, 0x03 prefix or the 0x04 prefix. So to do a fully invalid key check i would go with bytes which I know for sure a wrong like 1+ the maximum value of the curve which would be N.

Copy link
Contributor

Choose a reason for hiding this comment

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

This way you can validate the point verification function and will be really useful.

0x25, 0x66, 0x77, 0x09, 0xc1, 0xab, 0x98, 0x0c, 0x11, 0x4d, 0x35,
0x94, 0xe6, 0x25, 0x9e, 0x81, 0x2e, 0x6a, 0x70, 0x3d, 0x4f, 0x51,
]);

expect(() => {
PublicKey.fromBytesECDSA(invalidPrivateKeyBytes);
}).to.throw;
});

it("should be able to create ECDSA key from valid bytes", function () {
const validPubKey = PrivateKey.generateECDSA().publicKey;
const bytes = validPubKey.toBytes();
const publicKey = PublicKey.fromBytes(bytes);
expect(publicKey.toString()).to.be.equal(validPubKey.toString());
});

it("should be able to create ED25519 key from valid bytes", function () {
const validPubKey = PrivateKey.generateED25519().publicKey;
const bytes = validPubKey.toBytes();
const publicKey = PublicKey.fromBytes(bytes);
expect(publicKey.toString()).to.be.equal(validPubKey.toString());
});
});
Loading