Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Many bad symmetric cryptography implementations in include/crypto.php #1655

Closed
paragonie-scott opened this issue Jun 4, 2015 · 13 comments
Closed
Milestone

Comments

@paragonie-scott
Copy link

@paragonie-scott paragonie-scott commented Jun 4, 2015

https://github.com/friendica/friendica/blob/master/include/crypto.php#L189-L212

  • ECB mode
  • I don't even know what is going on with that $ky loop. You're xoring it with null bytes so it literally does nothing.
  • Code duplication
  • Unauthenticated encryption

https://github.com/friendica/friendica/blob/master/include/crypto.php#L229-L245

  • NULL IV for CBC mode, when MCRYPT_DEV_URANDOM was being used for ECB mode? (It gets discarded in ECB mode, but you NEED an unpredictable and random IV for CBC mode.)
  • Unauthenticated encryption
@annando
Copy link
Collaborator

@annando annando commented Jun 4, 2015

Thank you very much for this analysis.

Good thing is: The functions "AES256CBC_decrypt" and "AES256CBC_encrypt" aren't in use.

Bad thing: The functions "aes_encrypt" and "aes_decrypt" is used in the central communication process between the servers. What do you suggest?

@paragonie-scott
Copy link
Author

@paragonie-scott paragonie-scott commented Jun 4, 2015

One of the following:

  1. libsodium
  2. defuse/php-encryption

I would also ask one or more of @ircmaxell @defuse @joepie91 @jedisct1 @CodesInChaos @enygma for their thoughts.

@annando
Copy link
Collaborator

@annando annando commented Jun 4, 2015

Thanks for the links! The huge question will be: How can we change something in the communication process without breaking communication with the other servers? We can't change only one side. And especially this "Unauthenticated encryption" sounds to me like a fundamental problem with the protocol.

I haven't invented this protocol and I'm no specialist in encryption.

We really have to think it over.

@joepie91
Copy link

@joepie91 joepie91 commented Jun 4, 2015

I'd say that the steps should be as follows:

  • 1. Implement a migration path; eg. have a version handshake that determines which protocol is to be used (if no such handshake protocol exists yet, assume that 'no handshake' means 'version 1', and implement a handshake from now on for the v2 protocol and upwards).
  • 2. Release an updated version, explicitly as an 'urgent security release', that switches to the new protocol.
  • 3. Never again use homebrew crypto code; the old code could stick around for a while to communicate with legacy servers, but should eventually be phased out, and should no longer be used anywhere in new versions.

These kind of migrations are not impossible; for example, XMPP, which is a much more distributed protocol (as there are many, many implementations of it) fairly recently switched to requiring TLS between federated XMPP servers.

Practically every provider, except for Google (who treat Talk as a deprecated service anyway), has switched to this. The transition was almost entirely smooth. With Friendica being a more centrally controlled codebase, this should be even easier.

Whatever you do, please make sure to pick an implementation that is widely available on many different platforms/languages (eg. libsodium would qualify here). This ensures interoperability.

EDIT: To clarify; XMPP itself doesn't require TLS, but it has been a community decision to do so, and it has been deployed widely.

@annando
Copy link
Collaborator

@annando annando commented Jun 5, 2015

@joepie91 Thanks for your reply! The biggest question for me is: Will it be enough to change the protocol with exchanging the crypto functions or is the whole protocol a problem? I haven't invented it and I never touched that parts of the code by now. I really don't want to do a change - and then have to realize that we should discard the whole protocol.

@redmatrix
Copy link

@redmatrix redmatrix commented Jun 5, 2015

That function is only used for RINO. I discarded that bit long ago and don't use it anymore. the function itself was reputedly 100% compatible with mysql encryption, and is the ony reason I used it ; thinking at the time we might save some resources and let the db deal with the encrypted data. DFRN has a huge number of problems. This is precsiely why I abandoned it some years ago and started over. At this point I would recommend activitypump as a long term federation protocol for friendica, since zot's nomadic identities don't federate well with non-nomadic services. However there is protocol versioning in DFRN, and RINO is optional. You can disable it, remove it, or replace it with something else. Personally I'd probably go with aes256cbc because it's well supported. Iirc in openssl you still need pkcs5 padding if you use the libopenssl version, and key,iv padding if you use mcrypt. Not everybody will be able to compile or use php modules that aren't supported in debian (for instance), although the integrity of the available aes implementations is questionable in the face of the Snowden revelations.

@fabrixxm
Copy link
Collaborator

@fabrixxm fabrixxm commented Jun 9, 2015

RINO is used to encrypt communications, I'm right? Many people now use tsl to talk to diaspora.
Can we remove RINO and offending functions altogether?

@fabrixxm fabrixxm modified the milestone: 3.4.1 Jun 9, 2015
@redmatrix
Copy link

@redmatrix redmatrix commented Jun 9, 2015

Correct. The protocol is abandoned as far as I'm concerned, and there are no compatible implementations, so you're free to do what you wish with it. There's a flag that's sent when you create a connection rino=1 which says you can accept encryption. We didn't always have it. The other side responds rino=1 if it actually sends encrypted text. You could set this to 0 to say you don't want encryption, or you might change the number to 2 to specify another crypto algorithm.

@joepie91
Copy link

@joepie91 joepie91 commented Jun 9, 2015

@annando I have no particular opinion on the protocol, as I haven't looked into it; I was simply suggesting a course of action after @paragonie-scott paged me here :)

@wmiltenburg
Copy link

@wmiltenburg wmiltenburg commented Jun 10, 2015

Is this the only encryption used between the server at the moment (besides SSL/TLS if supported)?

@fabrixxm
Copy link
Collaborator

@fabrixxm fabrixxm commented Jun 11, 2015

@wmiltenburg Yes, if enabled

@annando
Copy link
Collaborator

@annando annando commented Jun 29, 2015

Can we close the issue?

@fabrixxm
Copy link
Collaborator

@fabrixxm fabrixxm commented Jun 29, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.