WCIP-2: Refactor exchangeKey flow

[pedrouid]

Current implementation of exchangeKey is insecure. I first designed it as a pragmatic approach to recycle persisted sessions on browser's localstorage. However you can't securely exchange symmetric keys using previously compromised keys. This is something that I've been wanting to revisit and change for months but haven't prioritized in the whole scope of things that needed to be tackled.

A couple of weeks ago, me and @ligi brainstormed a better selection to ensure that the key exchange was secure and guaranteed to only be known by two parties. WalletConnect protocol is intended to only be used 1-on-1 therefore anyone else with access to the symmetry key exposed through URI (displayed by either a QR Code or deep link) shouldn't be able to decrypt these communications between these parties.

The first proposed change is that exchangeKey should only be triggered after the connection is established, therefore no unnecessary computation is used unless the session is approved by the counter-party (which currently is exchange prior approval).

The second proposed change is that the exchangeKey flow should be initiated by a signing challenge through a JSON-RPC request with the method wc_signingChallenge. For example, after connection approval the Dapp will request a signing challenge (which should be specific for WalletConnect to be auto-signed). This is very analogous to a similar proposal that I made on ETH Magicians here. The Wallet would receive this challenge and trigger an event for the signing challenge which can be handled with a callback as follows.

walletConnector.on("signing_challenge", (error, payload) => {
  if (error) {
    throw error;
  }

  wallet.sign(payload.params[1])
    .then((signature) => {
      walletConnector.approveSigningChallenge(signature)
    })
    .catch((error) => {
      walletConnector.rejectSigningChallenge(error.message)
    });
});

The reason we included this signing challenge is to be used by the Dapp to recover the public key to be used to encrypted the contents of the following exchangKey request. Hence after the Wallet has successfully signed the challenged, the Dapp will follow with a new request that will contain an encrypted payload with the next symmetric key to be used in the communications. We won't reuse the wc_exchangeKey JSON RPC method in order to keep backwards compatibility with previous v1.0.0-beta versions. The JSON-RPC request should have a method of wc_updateKey and should include a single parameter that includes a hexadecimal encoded string of the next key encrypted with the public key recovered from the signing challenge. This key can be decrypted by subscribing to an event for the key update which can be handled with a callback as follows.

walletConnector.on("key_update", (error, payload) => {
  if (error) {
    throw error;
  }

  wallet.decrypt(payload.params[1])
    .then((nextKey) => {
      walletConnector.approveKeyUpdate(nextKey)
    })
    .catch((error) => {
      walletConnector.rejectKeyUpdate(error.message)
    });
});

Additionally, we could provide an easier integration within the SDK configuration options that include a field for a private key to be used for the new exchangeKey flow. This will make it easier for Wallet integrations instead of subscribing to both events individually, yet both options should be available.

Finally, the private/public key pair doesn't need to be the same as the wallet keys which hold funds, meaning that can be generated separately and potentially could be generated within the SDK instead of passing this responsibility to the Wallet developer

[pedrouid]

The comment above mostly focus on the protocol specification but regarding the client SDK specifically, we could make the options to handle the key pairs used for the exchangeKey hierarchical. First we prioritize the private key provide through the configuration option field, then we check for existing event callbacks and finally we default to generating internally the key pairs used for the exchange. This way, it provides both options for the developers to use their own cryptography in case of security and performance concerns but also allows us to provide fallbacks and make sure the full exchangeKey is completed and the rest of the connection uses a secure symmetric key for encryption.

[pedrouid]

After sleeping on it, I think the best approach is to simply default to generate the key pair internally. The added benefit of allowing the wallet developer to handle the signing challenge and the key update doesn't seem very beneficial for the added overhead. As long as the key pair used is the same for the two events, it doesn't necessarily have to be persisted or match the wallet accounts. Therefore I would suggest to use ephemeral key pairs for the exchange flow that are created when the client first receives the signing challenge request.

[ptsayli]

I am getting different key in DApp and app after an app refresh. After refreshing an app, the app's call_request never gets called. It's really painful!

I also noticed a huge amount of un-connected socket objects on the bridge, which I think is bad for a bridge when we go in production.

I am not feeling confident on the whole socket approach on mobile devices (socket works fine browser side though). I did some debugging when the app is in the background - which yields socket readyState as 1 or sometimes yields 3. Is there anyway we cannot use the socket and use simple HTTP or GRPC APIs?

And, did anyone integrate WalletConnect v1.0 successfully with multiple DApps connections? I would love to know.

[pedrouid]

I just implemented the exchangeKey flow on the walletconnect-monorepo browser package. Here is the source code that handles the key management.

   // -- eventSubscriptions ------------------------------------------------------- //   
     this.on('wc_signingChallenge', (error, payload) => {
      if (error) {
        this._eventManager.trigger({
          event: 'error',
          params: [
            {
              code: 'SIGNING_CHALLENGE_ERROR',
              message: error.toString()
            }
          ]
        })
      }

       this._handleSigningChallenge(payload)
    })

     this.on('wc_keyUpdate', (error, payload) => {
      if (error) {
        this._eventManager.trigger({
          event: 'error',
          params: [
            {
              code: 'KEY_UPDATE_ERROR',
              message: error.toString()
            }
          ]
        })
      }

       this._handleKeyUpdate(payload)
    })
  }   

   // -- keyManager ------------------------------------------------------- //   
   private async _requestSigningChallenge () {
    const message = `Exchange Key Signing Challenge: ${uuid()}`

     const request = this._formatRequest({
      method: 'wc_signingChallenge',
      params: [convertUtf8ToHex(message)]
    })

     const result = await this._sendCallRequest(request)

     const publicKey = await this.cryptoLib.recoverPublicKey(
      convertHexToArrayBuffer(result),
      convertUtf8ToArrayBuffer(message)
    )

     return publicKey
  }

   private async _requestKeyUpdate (publicKey: ArrayBuffer) {
    this._nextKey = await this.cryptoLib.generateKey()

     const messageString = JSON.stringify({ nextKey: this.nextKey })

     const message = convertUtf8ToArrayBuffer(messageString)

     const encryptedMessage = await this.cryptoLib.encryptWithPublicKey(
      publicKey,
      message
    )

     const request = this._formatRequest({
      method: 'wc_keyUpdate',
      params: [encryptedMessage]
    })

     const result = await this._sendCallRequest(request)

     return result
  }

   private async _initKeyExchange () {
    const publicKey = await this._requestSigningChallenge()

     const result = await this._requestKeyUpdate(publicKey)

     if (result) {
      this._key = this._nextKey
      this._nextKey = null
    }
  }

   private async _handleSigningChallenge (payload: IJsonRpcRequest) {
    if (!this._keyPair) {
      this._keyPair = await this.cryptoLib.generateKeyPair()
    }
    const message = payload.params[0]

     const signature = await this.cryptoLib.sign(
      convertHexToArrayBuffer(this._keyPair.privateKey),
      convertHexToArrayBuffer(message)
    )

     const response = this._formatResponse({
      id: payload.id,
      result: convertArrayBufferToHex(signature)
    })

     this._sendResponse(response)
  }

   private async _handleKeyUpdate (payload: IJsonRpcRequest) {
    if (!this._keyPair) {
      const response = this._formatResponse({
        id: payload.id,
        error: {
          message: 'Failed to execute key update'
        }
      })

       this._sendResponse(response)
    } else {
      const encryptedMessage = payload.params[0]

       const message = await this.cryptoLib.decryptWithPrivateKey(
        convertHexToArrayBuffer(this._keyPair.privateKey),
        encryptedMessage
      )

       const messageString = convertArrayBufferToUtf8(message)

       if (messageString) {
        const { nextKey } = JSON.parse(messageString)

         if (nextKey) {
          const response = this._formatResponse({
            id: payload.id,
            result: true
          })

           await this._sendResponse(response)

           this.key = nextKey
          this._nextKey = null
        } else {
          const response = this._formatResponse({
            id: payload.id,
            error: {
              message: 'Failed to execute key update'
            }
          })

           this._sendResponse(response)
        }
      } else {
        const response = this._formatResponse({
          id: payload.id,
          error: {
            message: 'Failed to execute key update'
          }
        })

         this._sendResponse(response)
      }
    }
  }

[ligi]

TLDR: I think this is the wrong approach to attack this problem

It is way better than the exchange-key approach that sneaked into the implementation for a short while but I think the whole idea of symmetric->asymmetric-> symmetric is not elegant. Why don't we start with an asymmetric key in the first place? So in the QR-Code the public key is shown.Then the wallet encrypts a symmetric key for this public key and the connection is established. I think this has the following advantages:

• less back and forth/latency
• the asymmetric key is not generated on the mobile, but on the usually more beefier desktop running the dApp
• much cleaner and more elegant

[pedrouid]

TLDR: I think this is the wrong approach to attack this problem

It is way better than the exchange-key approach that sneaked into the implementation for a short while but I think the whole idea of symmetric->asymmetric-> symmetric is not elegant. Why don't we start with an asymmetric key in the first place? So in the QR-Code the public key is shown.Then the wallet encrypts a symmetric key for this public key and the connection is established. I think this has the following advantages:

• less back and forth/latency
• the asymmetric key is not generated on the mobile, but on the usually more beefier desktop running the dApp
• much cleaner and more elegant

The alternative proposal referred in PR WalletConnect/walletconnect-monorepo#118 is technically better than the current proposal for WCIP-2.

However it would require to redesign the WalletConnect v1.0 spec far further than simply introducing a new JSON RPC method.

1. We would need to change the EIP-1328 to include this change
2. SocketMessages would now be both asymmetrically encrypted and symmetrically encrypted. Currently the clients expect all the SocketMessages to be symmetrically encrypted with provided key in the URI schema.
3. This alternative proposal for the exchangeKey flow would no longer be consecutive. The payloads sent would not follow the less error-prone 1-2-1-2 message exchange and instead follow a pattern of 1-2-2-1 which is more likely to introduce race conditions.
4. The separation of concerns between JSON RPC payloads and SocketMessages would break.

These are four considerations around the alternative proposal which need to be more well thought out before introducing such a big breaking change.

I already voiced my concerns on WalletConnect/walletconnect-monorepo#118 about how bad adoption will be hit by such a breaking change.

[ligi]

1-2: agreed 3: can you elaborate? I do not see these problems you see. 4: disagree - WCIP 2 breaks separation of concerns as JSON RPC payloads are now changing how the SocketMessages are encrypted. As far as I see the alternative approach would still separate the concerns of JSON-RPC and the SocketMessages

[pedrouid]

I photographed the whiteboard when we were comparing the flow for the two proposals and regarding point number 3 you can see that proposal 1 always has consecutive requests and responses by the Dapp and Wallet. Meaning that whenever the Dapp sends a message it will only send the next one after receiving the response from the Wallet. Thus the pattern I described as 1-2-1-2. While the alternative proposal has the Wallet send the first request which is followed by the Dapp's response and also a new request sent by the Dapp, meaning that the Wallet has to both expect a response and request at the same time, which could be received in the wrong order, making it harder to track the state of the exchangeKey flow correctly. Thus the pattern I described as 1-2-2-1.

Current Proposal Alternative Proposal

Regarding point number 4, I meant that the separation of concerns break when the SocketMessages need to be encrypted differently depending on the state of the exchangeKey flow which is something should be only of the JSON RPC payload concern. However with the current proposal the SocketMessages only need to know which is the current active key, disregarding of the state of the exchangeKey flow, it will always encrypt it symmetrically using whatever key is active when a send is triggered.

This also exposes a fingerprint in the SocketMessages sent to the Bridge which you are able to detect when something is symmetrical or assymetrically encrypted, meaning a malicious actor can track when a exchangeKey is in process between two peers while if all messages were always symmetrically encrypted it would be impossible to track this from the Bridge.

[ligi]

Now I see what you mean. But I think it is WCIP-2 that has the problems you describe in 3 and 4 - not the alternative proposal. With WCIP-2 the exchange key can happen at any time (when I read the proposal right) - which can lead to all sorts of problems. With the alternative it is always the second step - no more key exchange afterwards. The order is very static and you can reason way better about it. And there is only breaking of separation of concerns in WCIP-2 - not in the alternative.

[pedrouid]

Replaced by #9