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

Jump to bottom

Feat: TrezorHostProtocol #13542

Draft

szymonlesisz wants to merge 15 commits into develop from feat/thp

Contributor

szymonlesisz commented Jul 29, 2024

heavy draft

github-advanced-security bot found potential problems

View reviewed changes

packages/protocol/tests/protocol-thp/thpKeys.test.ts Fixed Show fixed Hide fixed

packages/protocol/tests/protocol-thp/thpKeys.test.ts Fixed Show fixed Hide fixed

packages/protocol/scripts/thp-protobuf-build.js Fixed Show fixed Hide fixed

packages/protocol/scripts/thp-protobuf-build.js Fixed Show fixed Hide fixed

onvej-sl reviewed

View reviewed changes

packages/protocol/src/protocol-thp/crypto/crypto.ts Outdated

+                  const iv1 = getIvFromNonce(recvNonce); // should be 1
+                  const hostStaticKeys = getCurve25519KeyPair(Buffer.alloc(32).fill(9));
+                  const hostEphemeralKeys = getCurve25519KeyPair(Buffer.alloc(32).fill(1));

Contributor

onvej-sl Jul 29, 2024

This looks suspicious. The host ephemeral key pair should be generated randomly before the HandshakeInitiationReq is sent, since its public part is contained in the message.

onvej-sl reviewed

View reviewed changes

packages/protocol/src/protocol-thp/crypto/crypto.ts Outdated

+                  const iv0 = getIvFromNonce(sendNonce); // should be 0
+                  const iv1 = getIvFromNonce(recvNonce); // should be 1
+                  const hostStaticKeys = getCurve25519KeyPair(Buffer.alloc(32).fill(9));

Contributor

onvej-sl Jul 29, 2024

The host static key should be generated randomly once and kept constant for all invocations of the protocol.

onvej-sl reviewed

View reviewed changes

packages/protocol/src/protocol-thp/crypto/crypto.ts Outdated

+                  // const credentials = knownCredentials.find(c => {
+                  //     const isPaired = findKnownPairingCredentials(knownCredentials[0], trezorEphemeralPubkey);
+                  // })
+                  const hostTempKeys = hostStaticKeys || getCurve25519KeyPair(Buffer.alloc(32).fill(9));

Contributor

onvej-sl Jul 29, 2024

The bytes inside getcurve25519KeyPair should be 0900000000000000000000000000000000000000000000000000000000000000 instead of 0909090909090909090909090909090909090909090909090909090909090909.

onvej-sl reviewed

View reviewed changes

packages/protocol/src/protocol-thp/crypto/curve25519.ts

Comment on lines +83 to +121

+              function conditionalSwap(a: bigint, b: bigint, condition: boolean): [bigint, bigint] {
+                  const mask = condition ? -1n : 0n;
+                  const newA = (a & ~mask) | (b & mask);
+                  const newB = (a & mask) | (b & ~mask);
+                  return [newA, newB];

Contributor

onvej-sl Jul 29, 2024

I'm not sure if it makes sense to try to make this function run in constant time in TypeScript.

If so, then the line const mask = condition ? -1n : 0n; is likely not constant time since it probably uses a conditional branch.
if not so, then the function can be defined as return condition ? [b, a] : [a, b];.

onvej-sl reviewed

View reviewed changes

packages/protocol/src/protocol-thp/crypto/curve25519.ts

Comment on lines +156 to +197

+              function conditionalMove(first: bigint, second: bigint, condition: boolean): bigint {
+                  const trueMask = condition ? -1n : 0n;
+                  const falseMask = ~trueMask;
+                  return (first & falseMask) | (second & trueMask);
+              }

Contributor

onvej-sl Jul 29, 2024

I'm not sure if it makes sense to try to make this function run in constant time in TypeScript.

If so, then the line const trueMask = condition ? -1n : 0n; is likely not constant time since it probably uses a conditional branch.
if not so, then the function can be defined as return condition ? second : first;.

szymonlesisz force-pushed the feat/thp branch from 9186c1c to 6eff732 Compare

July 30, 2024 08:45

onvej-sl reviewed

View reviewed changes

packages/transport/protocol-v2-e2e.ts Outdated

+                  if (!isPaired) {
+                      if (PAIRING_METHODS.includes(2)) {
+                          const createCodeEntryChallenge = await call('ThpCodeEntryChallenge', {
+                              challenge: Buffer.alloc(32).fill(1),

Contributor

onvej-sl Jul 31, 2024

This should be generated randomly.

onvej-sl reviewed

View reviewed changes

packages/transport/protocol-v2-e2e.ts Outdated

+                      const { secret } = codeEntry.payload.message;
+                      const sha = sha256(Buffer.from(secret, 'hex'));
+                      console.warn('secret compare with commitment', sha.compare(handshakeCommitment));

Contributor

onvej-sl Jul 31, 2024

You should return an error if the hashes don't match.

onvej-sl reviewed

View reviewed changes

packages/transport/protocol-v2-e2e.ts Outdated

+                              await new Promise(resolve => setTimeout(resolve, 1000));
+                              const debugState = await call('DebugLinkGetState', {});
+                              const { thp_pairing_code_entry_code, thp_pairing_secret } =

Contributor

onvej-sl Jul 31, 2024

code_qr_code and code_nfc_unidirectional should be transmitted from Trezor to Host instead of thp_pairing_secret.

onvej-sl reviewed

View reviewed changes

packages/transport/protocol-v2-e2e.ts Outdated

Comment on lines 334 to 310

+                      if (pairingReq.payload.message.commitment) {
+                          handshakeCommitment = Buffer.from(pairingReq.payload.message.commitment, 'hex');
+                      }

Contributor

onvej-sl Jul 31, 2024

This should be done only if code entry was a pairing method advertised in protocol parameters.

onvej-sl reviewed

View reviewed changes

packages/transport/protocol-v2-e2e.ts Outdated

		const sha = sha256(Buffer.from(secret, 'hex'));

		console.warn('secret compare with commitment', sha.compare(handshakeCommitment));

Contributor

onvej-sl Jul 31, 2024

You should check that code = SHA-256(h || secret || challenge || PairingMethod_CodeEntry) % 1000000.

Contributor

onvej-sl Jul 31, 2024

Similarly for nfcPairing and qrCodePairing.

szymonlesisz force-pushed the feat/thp branch from 6157652 to ae84f83 Compare

August 1, 2024 10:21

szymonlesisz force-pushed the feat/thp branch 4 times, most recently from b621c58 to 8bd0e9b Compare

August 19, 2024 18:54

szymonlesisz force-pushed the feat/thp branch 2 times, most recently from 3a87b56 to db8ab69 Compare

August 26, 2024 09:35

szymonlesisz force-pushed the feat/thp branch from cdbdc0a to 1a86c30 Compare

August 29, 2024 16:54

szymonlesisz force-pushed the feat/thp branch 5 times, most recently from b2da7e9 to 5ef1e08 Compare

September 16, 2024 15:56

github-advanced-security bot found potential problems

View reviewed changes

packages/protocol/tests/protocol-thp/thpKeys.test.ts Fixed Show fixed Hide fixed

packages/protocol/tests/protocol-thp/thpKeys.test.ts Fixed Show fixed Hide fixed

szymonlesisz force-pushed the feat/thp branch 3 times, most recently from 6c0b109 to bb1c4da Compare

September 25, 2024 15:20

github-advanced-security bot found potential problems

View reviewed changes

packages/protocol/tests/protocol-thp/aesgcm.test.ts

+              it('AESGCM encode/decode', () => {
+                  const key = Buffer.from(
+                      'ccbf529fc8dd4662d4d1d1fa66368b8758c0b6673a1bb9d532d95ca607cbf729',

Check failure

Code scanning / CodeQL

Hard-coded credentials Critical test

The hard-coded value "ccbf529fc8dd4662d4d1d1fa66368b8758c0b6673a1bb9d532d95ca607cbf729" is used as

key

.

packages/protocol/tests/protocol-thp/thpKeys.test.ts

+              import { aesgcm } from '../../src/protocol-thp/crypto/aesgcm';
+              import { curve25519, getCurve25519KeyPair } from '../../src/protocol-thp/crypto/curve25519';
+              const PROTOCOL_NAME = Buffer.from('Noise_XX_25519_AESGCM_SHA256', 'ascii');

Check failure

Code scanning / CodeQL

Hard-coded credentials Critical test

The hard-coded value "Noise_XX_25519_AESGCM_SHA256" is used as

key

.

github-advanced-security bot found potential problems

View reviewed changes

packages/protocol/scripts/thp-protobuf-build.ts Fixed Show fixed Hide fixed

szymonlesisz mentioned this pull request

Feat(protobuf): build messages.json using javascript #14752

Merged

szymonlesisz force-pushed the feat/thp branch from 757a81f to 8451c81 Compare

October 11, 2024 10:28

github-advanced-security bot found potential problems

View reviewed changes

packages/protobuf/scripts/build-protobuf.ts Fixed Show fixed Hide fixed

szymonlesisz force-pushed the feat/thp branch 6 times, most recently from 26daf9d to a0a7f06 Compare

October 24, 2024 12:20

mroz22 reviewed

View reviewed changes

packages/connect/src/api/resetDevice.ts Outdated

Comment on lines 45 to 61

+                      // const cmd = this.device.getCommands();
+                      // const response = await cmd.typedCall('ResetDevice', 'Success', this.params);
+                      console.log('----> running reset function!');
+                      const response = await this.device.transport.call({
+                          session: this.device.transportSession!,
+                          name: 'LoadDevice',
+                          data: {
+                              pin: '',
+                              label: 'THP device',
+                              passphrase_protection: true,
+                              mnemonics: ['all all all all all all all all all all all all'],
+                              skip_checksum: true,
+                          },
+                          protocol: this.device.protocol,
+                          protocolState: this.device.protocolState,
+                      });

Contributor

mroz22 Nov 12, 2024

this can be now dropped. TrezorConnect.loadDevice should be available

peter-sanderson mentioned this pull request

Bluetooth onboarding #15390

Open

szymonlesisz added 15 commits

December 12, 2024 16:05


          feat(protocol): add protocol-v2

86c12a4


          feat(protobuf): add THP protobuf messages

d38411d


          feat(protocol): add THP protobuf messages and types

eb15f56


          feat(protocol): add THP cryptography

7c0c60c

Co-authored-by: Ondřej Vejpustek <[email protected]>

following: trezor/trezor-firmware#3435


          feat(protocol): add protocol-thp

fc8a7d6


          feat(transport): add THP send/receive modules

0a13038


          feat(transport): implement THP

f07363e


          feat(transport-bridge) implement THP

f3ed784


          feat(connect): implement THP

118872b


          feat(suite): display modals in WelcomeLayout


          feat(suite): ⚠️ add ThpPairingModal

f4987d4


          chore: ⚠️ resetDevice as LoadDevice

3eb7529


          test(connect): THP e2e tests

e3976a6


          feat(protobuf): load definitions

ea65a43


          feat(transport): load protobuf definitions

fa7ff3b

szymonlesisz force-pushed the feat/thp branch from a0a7f06 to fa7ff3b Compare

December 12, 2024 15:10

github-actions bot commented Dec 12, 2024

🚀 Expo preview is ready!

Project → trezor-suite-preview
Platforms → android, ios
Scheme → trezorsuitelite
Runtime Version → 20
More info

Learn more about 𝝠 Expo Github Action

github-advanced-security bot found potential problems

View reviewed changes

packages/connect/src/device/Device.ts

+                              let newSessionId = 0;
+                              // TODO: what if known.length >= 255?
+                              while (!newSessionId || knownSessions.includes(newSessionId.toString())) {
+                                  newSessionId = getWeakRandomInt(1, 256);

Check failure

Code scanning / CodeQL

Insecure randomness High

This uses a cryptographically insecure random number generated at

Math.random()

in a security context.

Copilot Autofix AI about 18 hours ago

To fix the problem, we need to replace the use of getWeakRandomInt with a function that generates cryptographically secure random integers. We can use the crypto module available in Node.js to achieve this. Specifically, we can use crypto.randomInt to generate a secure random integer within a specified range.

Replace the call to getWeakRandomInt with a new function that uses crypto.randomInt to generate a secure random integer.
Ensure that the new function is imported and used correctly in the relevant parts of the code.

Suggested changeset 1

packages/utils/src/getWeakRandomInt.ts

Outside changed files

@@ -1 +1,3 @@
+            import { randomInt } from 'crypto';
             /**
@@ -11,3 +13,3 @@
-                return Math.floor(Math.random() * (max - min) + min);
+                return randomInt(min, max);
             };

Copilot is powered by AI and may make mistakes. Always verify output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

onvej-sl onvej-sl left review comments

mroz22 mroz22 left review comments

martykan Awaiting requested review from martykan martykan will be requested when the pull request is marked ready for review martykan is a code owner

marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak will be requested when the pull request is marked ready for review marekrjpolak is a code owner

Nodonisko Awaiting requested review from Nodonisko Nodonisko will be requested when the pull request is marked ready for review Nodonisko is a code owner

vdovhanych Awaiting requested review from vdovhanych vdovhanych will be requested when the pull request is marked ready for review vdovhanych is a code owner

At least 1 approving review is required to merge this pull request.

Labels

None yet