Bisq 2: Security Module Review

[alvasw]

I spent the last couple of days reviewing the Misq crypto code in the security module. I didn't find anything serious. Some things like the deriveKeyMaterial(...) (HybridEncryption.java) looked weird, but the code is generally ok. But I have a couple of questions/discussion points.

1. Is there a specific reason why CBC was chosen as the block cipher mode? CBC encryption isn't parallelized. Why not use an ADEAD [1] (Authenticated encryption) cipher like AES-GCM. This would simplify the design by not explicitly dealing with authenticity while keeping the security. Galois/Counter Mode can be parallelized because it encrypts an incrementing counter (derived from IV).

2. @chimp1984 's proposal states that each trade should have its own onion/i2p address [2]. Why not allow a user to post the same offer to both networks? We can bind the offers to a static identity key (per offer/user id).

3. How do we want to track the reputation of a user? One approach would be to use a key pair per identity in addition to the ephemeral keys. When needed, the keys linked to an identity can authenticate oneself to the other peer. This would separate the identities from the network layer (One user can have multiple identities).

4. In the source code, I did find references to clearnet. I think it's mainly used to relay traffic, but is the traffic encrypted (transport encryption)?

5. A general question is why we're not building on top of the Noise Protocol Framework [3]? It's widely used (WhatsApp, WireGuard, Lightning, and I2P) and simple.
   We get a lot for free:

   • Static and ephemeral key pairs
   • ADEAD (see 1.)
   • Supports identity hiding
   • Supports forward secrecy (using REKEY)
   • We can add the PoW authentication scheme to the protocol using channel binding.
   • Protocols build using Noise are easier to verify by researchers. The modeling and verification can be automated as well [4].

6. We should think about versioning the encryption ciphers to switch to another one later if needed. Who knows what happens in a couple of years.

I attacked, designed, and implemented security protocols before. I can help and work on this after completing the wallet integration if needed.

[1] Morris Dworkin. "Recommendation for Block Cipher Modes of Operation: Galois/Counter Mode (GCM) and GMAC". NIST Special Publication 800-38D
[2] @chimp1984 . "Bisq 2.0 - A multi-protocol DEX (working title Misq)". bisq-network/proposals#330
[3] Trevor Perrin. "The Noise Protocol Framework". http://www.noiseprotocol.org/noise.pdf
[4] Kobeissi, Nadim, Georgio Nicolas, and Karthikeyan Bhargavan. "Noise Explorer: Fully automated modeling and verification for arbitrary Noise protocols." 2019 IEEE European Symposium on Security and Privacy (EuroS&P). IEEE, 2019.

[chimp1984]

Thanks for the review!
To your questions:

1. No specific reason. We can change to what you recommend. I read a bit about vulnerabilities with paddings and CBC seems to be safe in that regards.
2. Yes that is the plan. Each offer will have its key pair. If the user supports multiple networks it gets published on those networks. If a taker contacts the maker they will use the same pub key independent of the network.
3. The user will have the choice between maximal privacy (each trade use a new key pair) or make trade offs to gain other features like the local reputation (e.g. use one global key pair for all trades, or one key pair for all fiat trades, another for all altcoin trades...).
4. Clear-net is the third network type we support (next to Tor and I2P). For traders it will make probably no sense to use it, but for some network nodes like seed nodes it might have some limited usefulness: To have faster and more reliable connections thus syncing data in the network better. If we find some more meaningful use case like for instance for mobile apps connecting to a relay node we would need some security layer for that as well. I left it out so far to reduce complexity, but could be added if we see a need. Ah, and for dev testing its very useful (similar as we use regtest/localhost in bisq dev testing).
5. I looked a bit into it but was not clear how to use it. Could you make a prototype for the encryption scheme using the noise protocol framework?
6. Good point. Maybe the noise protocol framework does support that as well?

Yes would be great if you can take over that part! I am not a cryptographer...

Regarding the deriveKeyMaterial method: It was an attempt to derive 2 keys from the shared key, but probably not a good one... feel free to replace it with some more standard way how to do that.

[alvasw]

Thank you for answering my questions. This helped me to understand the code.
I can make a prototype for the encryption scheme using the noise protocol framework after I'm done with the wallet work.