diaspora / diaspora_federation

A library that provides functionalities needed for the diaspora* federation protocol.
GNU Affero General Public License v3.0
101 stars 29 forks source link

Quick security review #49

Closed WildCryptoFox closed 7 years ago

WildCryptoFox commented 7 years ago

Hello.

Quick disclaimer: I have only quickly skimmed over some points of interest within the documentation. I am here only to offer some suggestions. I do not have a deep understanding of the diaspora design.

Multiple encoding formats

This protocol relies on at least JSON and XML. Neither are suitable for robust systems. Both require decent parsing efforts and have led to logical bugs. This was critical for Android's APK handling where the signature-checker had a different zip implementation to the unzip tool. What happens when two files have the same name? Each program would see one, or the other. The signature was valid for one instance of the file, while the other was extracted.

It may be worth looking into a better defined serialization method such as https://capnproto.org. Capnproto prevents you from shooting yourself in the foot by preventing you from setting your protocol in stone - it will let the protocol evolve as necessary which should be a design aspect to diaspora. Saying "must use" for things like AES, RSA, base64, etc, is only harming the protocol.

Cryptography

AES

The "Encrypted Magic Envelope" is defined to only support AES. Which of the many modes of AES? Do not restrict this to only AES. Here after I assume it is not an authenticated mode (BAD).

"JSON object containing an encrypted XML object containing signed data" is very poorly constructed. Use Authenticated Encryption, not encrypted signatures. The only authenticated data here is the contents data field in the XML object. What happens if there are multiple entries? See Android vulnerability above.

The AES Key JSON encrypted with the recipients public key and then base64 encoded.

With the recipient's public key? No forward secrecy. If you establish the minimal state to support forward secrecy, then you may as well maintain a notion of a (optionally long-lived) session between contacts.

RSA

RSA keys are large. RSA is slow. RSA may be fast at verifying signatures, but it is slow to create keys, and to sign messages. Is this worth it? No. Again, hard-coded detail, RSA-SHA256 must be used? Considering there is an alg field that "must" looks like a mistake?

Please support Curve25519 for authenticated handshakes and Ed25519 for signatures. Additionally using 3DH you can have an authenticated and forward secret handshake H(DH(alice_ephemeral, bob_ephemeral) | Dh(alice_ephemeral, bob_static) | Dh(bob_ephemeral, alice_static)).

Signatures

Another question is if you need signatures? If each message is encrypted on a per-contact basis, then all you need is authenticated encryption between the two. No signature necessary.

Only use signatures when you are broadcasting a message, to either certify the message or to claim authorship of it. It may be reasonable to expect that within diaspora the sharing model may permit sharing to otherwise unauthorized parties; to such an extent signatures may be suitable.

Double Ratchet (core to the Signal protocol) is a good example for how to employ this; however it covers more problems such as packet reordering, dropped packets, and constantly rotates it's keys. This level of key rotation may or may not be desired for diaspora. The noise protocol may also be of interest here (double ratchet modulo the reliability corrections, it assumes the medium is already reliable).

Use capabilities

Enough said, now go look at pretty delegation graphs. https://github.com/dckc/awesome-ocap

svbergerem commented 7 years ago

This protocol relies on at least JSON and XML.

Because this protocol uses Magic Envelopes.

The "Encrypted Magic Envelope" is defined to only support AES. Which of the many modes of AES?

AES-256-CBC. I think this should be added to the documentation.

No. Again, hard-coded detail, RSA-SHA256 must be used? Considering there is an alg field that "must" looks like a mistake?

Well, that's in the definition of Magic Envelopes.

WildCryptoFox commented 7 years ago

Because this protocol uses Magic Envelopes.

Can it not be defined more generally and not specifically to either JSON or XML? Just fields and types.

AES-256-CBC. I think this should be added to the documentation.

So as I feared. Unauthenticated AES.

Well, that's in the definition of Magic Envelopes.

As linked, that is different to the documentation in this repository. That is indeed better worded, but only specifying one option is poor.

denschub commented 7 years ago

As @svbergerem already said, this wasn't our decision. Instead of implementing stuff on our own, we stick to already defined standards, in this case mostly Salmon and related. As a general point, while we appreciate your effort, we can't get any useful information out if this. Please provide actual examples of possible security issues rather than some abstract points that are hard to follow. If this was an attempt to sell awesome-ocap, you've failed. ;)

This was critical for Android's APK handling where the signature-checker had a different zip implementation to the unzip tool. What happens when two files have the same name? Each program would see one, or the other. The signature was valid for one instance of the file, while the other was extracted.

I absolutely don't see any context to diaspora. Please elaborate if needed.

With the recipient's public key? No forward secrecy. If you establish the minimal state to support forward secrecy, then you may as well maintain a notion of a (optionally long-lived) session between contacts.

You did miss that diaspora* and compatible networks do not keep a session alive, nor do we have any kind of handshake. The entire federation is based on unsolicited messages sent to different entities, comparable to PGP signed emails.

In fact, PGP signed emails is probably the closest comparison possible without getting too much into detail here.

RSA keys are large. RSA is slow. RSA may be fast at verifying signatures, but it is slow to create keys, and to sign messages. Is this worth it.

Yes. Mostly for the fact we don't care about performance and the fact RSA is not broken yet.

Another question is if you need signatures? If each message is encrypted on a per-contact basis, then all you need is authenticated encryption between the two. No signature necessary.

Yes, we need signatures. As mentioned previously, we don't have session. We neither can nor plan on exchanging ephemeral keys. Technically, diaspora* is always broadcasting messages and we always will have a similar architecture.

The decision to our current design was made by a number of factors. So far, the standards we depend on seem to work great. Others have implemented compatible suites, and multiple reviews passed without uncovering any issues. If you have actual implementation issues or provably security holes, please send an email to security@diasporafoundation.org and describe them. Until now, I am going to close this issue.

Thanks for your effort, though. However, a "please change everything just because it feels better" is nothing we can discuss about at this point in time. :) As mentioned, we're happy to discuss about individual points, but simply replacing everything with undefined caps and serializes while adding the need to keep complex streams of data alive to exchange messages is, at least on how we see the project right now, neither an option nor required.

VsyachePuz commented 7 years ago

lol

denschub commented 7 years ago

Excuse me?

WildCryptoFox commented 7 years ago

As @svbergerem already said, this wasn't our decision. Instead of implementing stuff on our own, we stick to already defined standards, in this case mostly Salmon and related. As a general point, while we appreciate your effort, we can't get any useful information out if this. Please provide actual examples of possible security issues rather than some abstract points that are hard to follow. If this was an attempt to sell awesome-ocap, you've failed. ;)

My primary point for Salmon then is that it would be better defined as fields and types, not necessarily either JSON or XML (or even capnproto). Or at least define the expected behavior for parsers - see below. As for objcaps, I didn't really try as I doubt the effort to implement them would be accepted here. ;-)

This was critical for Android's APK handling where the signature-checker had a different zip implementation to the unzip tool. What happens when two files have the same name? Each program would see one, or the other. The signature was valid for one instance of the file, while the other was extracted.

I absolutely don't see any context to diaspora. Please elaborate if needed.

The example for diaspora is what happens if you have an envelope like the following:

<me:env xmlns:me="http://salmon-protocol.org/ns/magic-env">
  <me:data type="application/xml">signed data</me:data>
  <me:data type="application/xml">unsigned data</me:data>
  <me:encoding>base64url</me:encoding>
  <me:alg>RSA-SHA256</me:alg>
  <me:sig key_id="valid key id">valid signature</me:sig>
</me:env>

What happens if there are two entries under the me:data field? Depending on the parser the data tags may be considered just children in a list, or it may pick either the first or the second. Next if two parsers read this with different implementation details, one may read the signed data - validating it, while the other just reads out the wrong data without validating it (as validation already occurred).

With the recipient's public key? No forward secrecy. If you establish the minimal state to support forward secrecy, then you may as well maintain a notion of a (optionally long-lived) session between contacts.

Above this and is more important is my mentions for AES being unauthenticated. There is no reason to use unauthenticated encryption!

You did miss that diaspora* and compatible networks do not keep a session alive, nor do we have any kind of handshake. The entire federation is based on unsolicited messages sent to different entities, comparable to PGP signed emails.

"any kind of handshake" how about two contacts? "Add friend" "accept friend" This is at least a logical handshake. A long lived session may be derived based on these identity keys. (don't even need to store it if you don't have any ephemerals; but that's weak too). This at least enables better than RSA-encrypted symmetric keys, to access encrypted data. Note that you're only using RSA-encrypted symmetric keys because you don't have another suitable key. ECDH gives you such a suitable key (after a single round of a decent hash function).

In fact, PGP signed emails is probably the closest comparison possible without getting too much into detail here.

Indeed. FWIW PGP is mostly suitable only for code signing, and rare other cases. Encrypting without forward secrecy is very weak and should not be used in any modern cryptographic protocols. At least IMHO, and many other cypherpunks will agree.

RSA keys are large. RSA is slow. RSA may be fast at verifying signatures, but it is slow to create keys, and to sign messages. Is this worth it.

Yes. Mostly for the fact we don't care about performance and the fact RSA is not broken yet.

Really? How about mobile clients? They may not have AES available either. RSA doesn't need to be broken to know it has problems. Generating keys is a VERY slow process. ECC keys are instantly "generated", key = random(32). There is NO REASON to use RSA. There are MANY reasons to use the alternatives. I think this needs at least more discussion than "not broken, won't fix".

Another question is if you need signatures? If each message is encrypted on a per-contact basis, then all you need is authenticated encryption between the two. No signature necessary.

Yes, we need signatures. As mentioned previously, we don't have session. We neither can nor plan on exchanging ephemeral keys. Technically, diaspora* is always broadcasting messages and we always will have a similar architecture.

That really depends on the distribution model. If it doesn't fit, then okay. Is it possible to reconsider the model? If not, oh well. Just no deniability.

The decision to our current design was made by a number of factors. So far, the standards we depend on seem to work great. Others have implemented compatible suites, and multiple reviews passed without uncovering any issues. If you have actual implementation issues or provably security holes, please send an email to security@diasporafoundation.org and describe them. Until now, I am going to close this issue.

Again more "not broken, won't fix" is harmful to innovation and keeping up with modern practices.

Thanks for your effort, though. However, a "please change everything just because it feels better" is nothing we can discuss about at this point in time. :) As mentioned, we're happy to discuss about individual points, but simply replacing everything with undefined caps and serializes while adding the need to keep complex streams of data alive to exchange messages is, at least on how we see the project right now, neither an option nor required.

I am not asking you to change everything. I have simply pointed out some needless protocol lock-ins preventing evolution, restricting performance, and not using the modern best-practices. (unauthenticated encryption is bad!)

FWIW: I personally have very little interest in diaspora and now even less. I have high expectations and I am working on my own projects, one of which was designed to prevent applications from making bad cryptographic or protocol decisions.

Please do not take any of this as hostile. I am always happy to see open source projects moving people away from centralized solutions - but I am disappointed to the lack of future-proofing and attempts to innovate.

Final request

Good luck with diaspora and happy hacking. =)

denschub commented 7 years ago

How about mobile clients?

?!

The communication we're talking about happens only between servers and other servers. Mobile clients are not, not even in the slightest form, involved in signing or encryption the payload exchanged between messages.

(unauthenticated encryption is bad!) [...] Encrypting without forward secrecy is very weak and should not be used in any modern cryptographic protocols

No. Not at all. Communication between nodes happens only between nodes. In addition to the crypto above, we transfer the data using SSL, so we get PFS for free. The only hole in this system is the theoretical chance of someone getting control over the endpoints, in which case you're encryption is broken anyway.

Again more "not broken, won't fix" is harmful to innovation and keeping up with modern practices.

If this world would be full of unicorns and rainbows, yeah. We're compatible with multiple software implementations and we're pretty happy we have a working and secure solutions. The only holes we have would be issues in which encryption would be worthless anyway (if your endpoints are corrupt, you're doomed), so there is absolute zero room for improvement here.

Good luck with your projects, though. Thanks for your input.

WildCryptoFox commented 7 years ago

The communication we're talking about happens only between servers and other servers. Mobile clients are not, not even in the slightest form, involved in signing or encryption the payload exchanged between messages.

So nothing is end to end. Why are we even encrypting these messages then? If users aren't typing in passphrases to unlock their keys, then the keys are used by the server too? That is pretty useless. It may work fine for signatures, but there is no advantage to encrypt in this case - especially if you already trust TLS between nodes.

No. Not at all. Communication between nodes happens only between nodes. In addition to the crypto above, we transfer the data using SSL, so we get PFS for free. The only hole in this system is the theoretical chance of someone getting control over the endpoints, in which case you're encryption is broken anyway.

TLS* I hope... As for forward secrecy. That is a TLS1.2 feature, which is also broken by another TLS1.2 feature - session resumption. If you want forward secrecy then you had better be using TLS1.3! It does this by correcting session resumption to not cancel out forward secrecy. Alternatively make sure that session resumption under TLS1.2 is disabled.

If this world would be full of unicorns and rainbows, yeah. We're compatible with multiple software implementations and we're pretty happy we have a working and secure solutions. The only holes we have would be issues in which encryption would be worthless anyway (if your endpoints are corrupt, you're doomed), so there is absolute zero room for improvement here.

You can do better than that... Oh well. You seem happy with the current state of diaspora. I won't destroy that for you. =)

Good luck with your projects, though. Thanks for your input.

Thanks! Feel free to poke me if you have any further questions or would like me to review anything else. Happy hacking.