Multisig: Connect request and SignMultisigTransaction request #458

sisou · 2022-12-01T15:36:40Z

This PR adds two requests:

Connect
SignMultisigTransaction

Connect

For this request, I am adding deterministic RSA key generation from the user's Key's secret. The public RSA key is then shared with the connected app, to be able to encrypt data that only the Keyguard can decrypt again. Encryption and decryption uses the window.crypto.subtle APIs.

SignMultisigRequest

Nothing special added, this uses the code from the @nimiq/core SDK to do multisig signatures.

sisou · 2022-12-01T15:46:21Z

@danimoh Do you maybe have a better idea of how to solve the double KeyguardCommand.KeyguardCommand in this commit?
2532190

from participants' public keys

Missing multisig-specific UI elements

Because CSP doesn't allow inline-scripts. This required getting the types for node-forge correct, too.

Requires passing the parameters in the request to ensure generation of the correct decryption key.

danimoh

In my opinion, quite a bit too many shortcuts have been taken, for example regarding missing i18n, unencrypted storage of private keys and omission of of relevant information in the UI.

There were also several serious security issues:

generated RSA keys are orders of magnitude weaker than they should be, due to the way they are generated.
(almost) arbitrary Nimiq messages are blindly signed, which can be used to impersonate the user.
malicious apps are able to trick the Keyguard into storing and using RSA private keys known to them.

Additionally, this PR points out opportunities for further security hardening, code cleanup, code deduplication, smaller fixes, stricter parsing etc..

danimoh · 2025-02-02T21:18:12Z

.eslintignore

@@ -1,3 +1,4 @@
 *.min.*
 src/lib/bitcoin/BitcoinJS.js
 src/lib/polygon/OpenGSN.js
+src/lib/multisig/wasm/pkg


This folder has been removed again in fbbcf06.

danimoh · 2025-02-02T21:57:04Z

client/src/PublicRequest.ts

+export type SignMultisigTransactionRequestCommon = Transform<SignTransactionRequestCommon, 'keyLabel' | 'senderLabel', {
+    keyLabel: string, // Not optional
+    senderLabel: string, // Not optional
+}> & {
+    multisigConfig: MultisigConfig,
+};


Could also be

Suggested change

export type SignMultisigTransactionRequestCommon = Transform<SignTransactionRequestCommon, 'keyLabel' | 'senderLabel', {

keyLabel: string, // Not optional

senderLabel: string, // Not optional

}> & {

multisigConfig: MultisigConfig,

};

export type SignMultisigTransactionRequestCommon = Transform<

SignTransactionRequestCommon,

'keyLabel' | 'senderLabel',

Required<Pick<SignTransactionRequestCommon, 'keyLabel' | 'senderLabel'>>

> & {

multisigConfig: MultisigConfig,

};

This ensures that there are no discrepancies if the original types of keyLabel or senderLabel were to change in SignTransactionRequestCommon, or if they were to be removed there. Obviously, in this case this benefit is of a rather theoretical nature. I just wanted to mention this alternative.

danimoh · 2025-02-02T22:00:14Z

client/src/PublicRequest.ts

@@ -94,6 +94,7 @@ export type BitcoinTransactionInfo = {
 };

 export type SignTransactionRequestLayout = 'standard' | 'checkout' | 'cashlink';
+export type SignMultisigTransactionRequestLayout = 'standard';


Are you planning to add other layouts?

danimoh · 2025-02-02T22:28:07Z

client/src/PublicRequest.ts

@@ -593,8 +652,11 @@ export type Result = RedirectResult | IFrameResult;
 // Derived Result types

 export type ResultType<T extends RedirectRequest> =
-    T extends Is<T, SignMessageRequest> | Is<T, SignTransactionRequest> ? SignatureResult :
+    T extends Is<T, SignMessageRequest> ? SignatureResult :


Should SignMessageRequest also receive an alias SignMessageResult for SignatureResult, similar to SignMultisigTransactionResult? Also in RedirectResult above and ResultByCommand below.

danimoh · 2025-02-03T08:43:05Z

types/Keyguard.d.ts

+type EncryptionKeyParams = {
+    kdf: string
+    iterations: number
+    keySize: number
+}


Can be used as KeyguardRequest.EncryptionKeyParams without copying it from PublicRequest.ts.

Suggested change

type EncryptionKeyParams = {

kdf: string

iterations: number

keySize: number

}

type EncryptionKeyParams = KeyguardRequest.EncryptionKeyParams;

danimoh · 2025-02-23T17:33:48Z

src/components/AddressInfo.js

+            $badge.classList.add('multisig-badge', 'nq-blue-bg');
+            $badge.innerHTML = TemplateTags.hasVars(2)`
+                ${this._addressInfo.multisig.signers}
+                <span class="of">of</span>


Missing i18n.

danimoh · 2025-02-23T17:37:08Z

src/components/AddressInfo.css

+    padding: 0.375rem 0.75rem;
+    line-height: 1;
+    border: 0.5rem solid white;
+    word-spacing: -0.071428571em; // 1/14


Invalid CSS comment (should be /* 1/14 */). But probably better as calc(-1em / 14) anyways.

Suggested change

word-spacing: -0.071428571em; // 1/14

word-spacing: calc(-1em / 14);

danimoh · 2025-02-23T17:45:49Z

src/components/LoginFileAccountIcon.js

+
+class LoginFileAccountIcon { // eslint-disable-line no-unused-vars
+    /**
+     * @param {string} [address]


Why is the address allowed to be undefined? It's nowhere used in such a fashion.

danimoh · 2025-02-23T17:57:37Z

tools/build.sh

@@ -365,6 +366,7 @@ cp -v favicon.ico dist
 cp -rv src/assets/* dist/assets/
 cp -v src/lib/QrScannerWorker.js dist/lib/
 cp -v node_modules/@nimiq/style/nimiq-style.icons.svg dist/assets/
+cp -v src/lib/rsa/sandboxed/* dist/lib/rsa/sandboxed/


Integrity hashes on the js files would be good.

danimoh · 2025-02-23T18:14:04Z

demos/index.html

+<div><a href="/demos/SignMultisigTransaction.html">Sign Multisig TX</a></div>
+<div><a href="/demos/RSAKeys.html">RSA Keys</a></div>
+<div><a href="/demos/ConnectAccount.html">Connect Account</a></div>


These demos don't work currently.

sisou requested review from danimoh and mraveux December 1, 2022 15:36

sisou self-assigned this Dec 1, 2022

sisou force-pushed the multisig branch from f924f46 to 2532190 Compare December 1, 2022 15:45

sisou force-pushed the multisig branch 2 times, most recently from 08af4d0 to 08748bb Compare December 5, 2022 17:05

sisou force-pushed the multisig branch 2 times, most recently from 0482b2d to da83212 Compare May 22, 2023 10:10

sisou force-pushed the multisig branch 2 times, most recently from 26cbddd to 58a01c6 Compare October 22, 2023 14:45

sisou mentioned this pull request Nov 14, 2023

Connect & Multisig nimiq/hub#521

Open

sisou force-pushed the multisig branch from 58a01c6 to e2a2743 Compare November 24, 2023 23:12

sisou force-pushed the multisig branch from e2a2743 to a62aa9f Compare August 24, 2024 09:30

sisou force-pushed the multisig branch from a62aa9f to b83cdf4 Compare September 24, 2024 10:05

sisou force-pushed the multisig branch from fb1a238 to 98d7997 Compare January 15, 2025 14:40

sisou added 14 commits January 15, 2025 15:52

Add test for signing a multisig transaction

8cb07e3

Add signPartially method to Key and use in test

a520a0a

MonkeyPatch MerkleTree to verify multisig sender address

ee6a2ba

from participants' public keys

[WIP] Add SignMultisigTransaction request

4b00fdf

Missing multisig-specific UI elements

Add demo for multisig signing

b80eabd

Add multisig config badge to AddressInfo component

7402611

Add user and account name section

07da998

Update title wording

a609892

Demo of generating random RSA keys in a sandboxed iframe

f605504

Use node-forge in sandboxed iframe to generate deterministic RSA key

b10662f

Use ArrayBuffers to exchange key material

8de308a

Extend 32-byte entropy to 1024-byte seed with PBKDF2

35425a4

Refactor Key config into an object and extend with rsaKeyPair option

4259030

Add RSA key computation to Key class

ff780c7

sisou added 17 commits January 15, 2025 15:52

Implement encrypted secret parsing and decryption

f90da33

Add Rust code compiled to WASM for secret aggregation

9fb4580

Update multisig badge UI, show identicon for legacy accounts

e424038

Connect request

59aa873

Extract LoginFileAccountIcon into a component

293763a

Refactor multisig request type structure

36d898e

Add comment about permission additions

7223628

Narrow encryptionKey algorithm type

0593220

Add sandboxed iframe files to dist during build

63bf825

Extract inline-script from RSAKeysIframe

edb4c2d

Because CSP doesn't allow inline-scripts. This required getting the types for node-forge correct, too.

Add explainer tooltip to connect UI

b1ea4a9

Clean-up, improve error handling

79a8eae

Fix types for KeyguardCommand

d5959d5

Allow RSA key generation to use dynamic parameters

7841d8b

Requires passing the parameters in the request to ensure generation of the correct decryption key.

Apply fixes from the Typescript update (PR #467)

820d23f

Allow signing multisig transactions from a vesting contract

9157108

Update SignMultisigTransaction flow to Albatross PoS SDK

fbbcf06

sisou force-pushed the multisig branch from 98d7997 to fbbcf06 Compare January 15, 2025 14:53

danimoh requested changes Mar 10, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multisig: Connect request and SignMultisigTransaction request #458

Multisig: Connect request and SignMultisigTransaction request #458

sisou commented Dec 1, 2022 •

edited

Loading

sisou commented Dec 1, 2022

danimoh left a comment

danimoh Feb 2, 2025

danimoh Feb 2, 2025

danimoh Feb 2, 2025

danimoh Feb 2, 2025

danimoh Feb 3, 2025

danimoh Feb 23, 2025

danimoh Feb 23, 2025

danimoh Feb 23, 2025

danimoh Feb 23, 2025

danimoh Feb 23, 2025

This comment was marked as duplicate.

	word-spacing: -0.071428571em; // 1/14
	word-spacing: calc(-1em / 14);

Multisig: Connect request and SignMultisigTransaction request #458

Are you sure you want to change the base?

Multisig: Connect request and SignMultisigTransaction request #458

Conversation

sisou commented Dec 1, 2022 • edited Loading

Connect

SignMultisigRequest

sisou commented Dec 1, 2022

danimoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

sisou commented Dec 1, 2022 •

edited

Loading