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

Derive public key from Wallet extended public key without the need of the elliptic library. #712

Closed
lucanicoladebiasi opened this issue Mar 14, 2024 · 6 comments · Fixed by #751

Comments

@lucanicoladebiasi
Copy link
Contributor

The #105 Try to substitute secp256k1 using noble-secp256k1 moves from elliptic lib to the secured noble-curves lib for cryptographic function.

The elliptic lib is used only to derive public keys from Wallet HD extended public keys hence no risk of security infringements, but the same functionalities seems to be implemented elsewhere in the SDK.

The hdnode.ts code should provide the same functionalities through the ether lib without the need of the elliptic one.

@nwbrettski
Copy link
Collaborator

This would result in being able to remove the dependency elliptic

@nwbrettski nwbrettski changed the title Derive public key from Wallet extended bublic key without the need of the elliptic library. Derive public key from Wallet extended public key without the need of the elliptic library. Mar 19, 2024
@lucanicoladebiasi
Copy link
Contributor Author

Experimenting libraries

to provide alternatives to elliptic libraries.

My notes so far:

  • Ether.js not needed.
  • hdnode.ts sould be refactored using the certified algorithms.- Tests must be rewritten because they do not test any HDNode key: extended keys only have the private or public components.

@lucanicoladebiasi
Copy link
Contributor Author

lucanicoladebiasi commented Mar 26, 2024

packages/core/src/secp256k1/secp256k1.ts provides the function randomBytes(bytesLength?: number | undefined): Buffer as drop-in replacement for the same function provided by crypto.js.

The new implementation is based on noble-hashes library, used for seckp256k1 ECDSA curve, hence

Source of randomness based on crypto.js are replaced.
Tests simplified removing redundant random generator functions.

@lucanicoladebiasi
Copy link
Contributor Author

packages/core/tests/mnemonic/mnemonic.unit.test.ts uses crypto.js in

  • test('Custom generation parameters'
    • describe('Mnemonic', ()

because refactoring to complex at the moment.

@lucanicoladebiasi
Copy link
Contributor Author

The PR at #751 removes the deprecated crypto-js vulnerabilities.

fabiorigam pushed a commit that referenced this issue Mar 27, 2024
* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function

* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function

* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function

* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function

* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function

* refactor: #712 `crypto.js` randomBytes replaced by `secp256k1.randomByte` function
@lucanicoladebiasi
Copy link
Contributor Author

lucanicoladebiasi commented Mar 27, 2024

The missing function to complete noble-curves with the functionalities of elliptic is the function inflating a compressed public key to its uncompressed form. My implementation in TS is

function inflate(compressedPublicKey: Uint8Array): Buffer {
    const a = new BN(_secp256k1.CURVE.a.toString());
    const b = new BN(_secp256k1.CURVE.b.toString());
    const p = new BN(_secp256k1.CURVE.p.toString());
    const prefix = compressedPublicKey[0];
    const x = new BN(compressedPublicKey.slice(1), 'hex');
    const y2 = x.pow(new BN('3')).add(x.mul(a)).add(b).umod(p);
    let y = y2.pow(p.addn(1).divn(4)).umod(p);
    const two = new BN(2);
    if (y.mod(two) !== new BN(prefix - 2)) {
        y = p.sub(y);
    }
    return Buffer.concat([
        Buffer.from([0x04]), // Prefix byte for uncompressed key
        Buffer.from(x.toArray()), // X-coordinate
        Buffer.from(y.toArray()) // Y-coordinate
    ]);
}

using the secp256k1 constants provided by the noble-curves library.
The math at the base of the transformation is published at https://www.secg.org/sec2-v2.pdf, hence the above function is what must be certified.
The mathematician developed the noble-curve library thought is trivial hence omitted to implement the function.

Unfortunately BN and BigInt are prone to time based side channel attack, hence I must derive a better way to decompose the finite field of the elliptic transoformation, working on the base of what published at https://paulmillr.com/posts/noble-secp256k1-fast-ecc/#public-key

lucanicoladebiasi added a commit that referenced this issue Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment