signAndEncryptTransaction can send unencrypted messages

[mat888]

https://github.com/SaitoTech/saito-lite-rust/blob/292118715bfc68a12de4d28fa84c6f60993b36bb/lib/saito/wallet.ts#L914

If each of these nested conditions fail, the code following the below excerpt will send the original unencrypted tx.msg.

The culprit:

if (recipient == "") {
        if (this.app.keychain.hasSharedSecret(tx.to[0].publicKey)) {
          tx.msg = this.app.keychain.encryptMessage(tx.to[0].publicKey, tx.msg);
        }
      } else {
        if (this.app.keychain.hasSharedSecret(recipient)) {
          tx.msg = this.app.keychain.encryptMessage(recipient, tx.msg);
        }
      }

The proposed fix:

// Empty placeholder protects data in case encryption fails to fire
      let encryptedMessage = ""

      // if recipient input has a shared secret in keychain
      if (this.app.keychain.hasSharedSecret(recipient)) {
        encryptedMessage = this.app.keychain.encryptMessage(recipient, tx.msg);
      }
      // if tx sendee's public address has shared secret
      else if (this.app.keychain.hasSharedSecret(tx.to[0].publicKey)) {
        encryptedMessage = this.app.keychain.encryptMessage(tx.to[0].publicKey, tx.msg);
      }
      // if encryption fails to occur, abort
      else {
        throw new Error('Failed to find shared secret. Aborting.');
      }

      tx.msg = encryptedMessage;

If keychain.hasSharedSecret(r) returning true when given an empty string input is a concern, then that condition should be checked within that function, not functions which call it.

[f8daniel]

Yeah, I think we are fine with the fallback to unencrypted messaging. Better than throwing an error and creating cascading ripples of failures

[mat888]

Could then remove the branch which throws an error, and it will send an empty message instead. BUT it's worth noting that code change is already within a try block.

I understand it's a useful generalization how it is but it's worth thinking about the unintended consequences which can spoil user security.

[f8daniel]

Maybe something to consider in the future. Most transactions are not encrypted, but in game chat relies on the flexibility of encrypting if available but not if unavailable.

On Mon, Dec 11, 2023, 7:45 PM Mat @.***> wrote:

Could then remove the branch which throws an error, and it will send an empty message instead. I understand it's a useful generalization how it is but it's worth thinking about the unintended consequences which can spoil user security.

— Reply to this email directly, view it on GitHub https://github.com/SaitoTech/saito-lite-rust/issues/2219#issuecomment-1850009175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQ3NQAFPLXXPMY4CEI6RZWDYI356XAVCNFSM6AAAAABAN4WIS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJQGAYDSMJXGU . You are receiving this because you commented.Message ID: @.***>