TelepathyIM / wocky

Wocky XMPP library
GNU Lesser General Public License v2.1
3 stars 3 forks source link

Consider removing SCRAM-SHA-384,512 #15

Open SamWhited opened 3 years ago

SamWhited commented 3 years ago

Hi,

While looking into wocky I noticed the following lines:

https://github.com/TelepathyIM/wocky/blob/master/wocky/wocky-auth-registry.c#L279-L283

The SCRAM-SHA1 and SCRAM-SHA-256 SASL mechanisms are standardized by the IETF. However, there is no such mechanism as SCRAM-SHA-512 or SCRAM-SHA-384. Since they are not supported by any XMPP clients, and do not provide any known security benefit over either of the other SCRAM mechanisms (since the hash is just used in an HMAC), please consider removing these mechanisms.

If the mechanisms are left in, and eventually a SCRAM-SHA-512 mechanism is created by the IETF but it differs somehow from the other mechanisms, you will have an incompatible version. This also may encourage other developers to implement the non-standard mechanism and/or to not support the actual standardized mechanisms out of some misguided idea that bigger numbers means that it is somehow "more secure". We don't want to have to clean up a mess later, or encourage other XMPP stacks to invent their own mechanisms which may only work with one or two clients and servers when safe, standardized, mechanisms have already been thought through by a group with expertise in these matters.

TL;DR let's not make up our own crypto, it's dangerous. Please trust the experts and wait until the IETF reviews and standardizes a new mechanism before implementing it.

Thanks for your consideration.

rufferson commented 3 years ago

Hi Sam, Thanks for looking into this. According to RFC5802

  1. SCRAM Mechanism Names

    A SCRAM mechanism name is a string "SCRAM-" followed by the uppercased name of the underlying hash function taken from the IANA "Hash Function Textual Names" registry (see http://www.iana.org), optionally followed by the suffix "-PLUS" (see below).

So the mechanisms are perfectly fine from this perspective. I always wondered why RFC7677 even exists but I do agree that that particular RFC puts additional restrictions on name usage by setting up a registry and mandating only registered mechanisms to be used.

As for 'does not add any security' - this is arguable, sha512 has better collision resistance (even though there's no known practical algorithm to produce collision for either sha256 or sha512 and they are used for HMAC) and higher complexity (means with same iteration count pbkdf2 is more resistant to brute force with 512) - which is handy for securing passwords at rest (salted).

Finally for "no one uses it" - it is defined in Cyrus SASL hence any server/client using that library is potentially using those mechanisms. I've also found references to it in WildFly Security SCRAM SASL Implementation. So I'm afraid it's a bit late to ban its existence.

SamWhited commented 3 years ago

On October 29, 2020 9:08:59 AM UTC, ruff notifications@github.com wrote:

So the mechanisms are perfectly fine from this perspective. I always wondered why RFC7677 even exists but I do agree that that particular RFC puts additional restrictions on name usage by setting up a registry and mandating only registered mechanisms to be used.

This does not define those mechanisms or allow arbitrary hashes, it just defines a template for naming new mechanisms.

As for 'does not add any security' - this is arguable, sha512 has better collision resistance (even though there's no known practical algorithm to produce collision for either sha256 or sha512 and they are used for HMAC) and higher complexity (means with same iteration count pbkdf2 is more resistant to brute force with 512) - which is handy for securing passwords at rest (salted).

This is not true. SCRAM passwords are not hashed with the hash, it's just used in an hmac. Collisions wouldn't matter, only preimage attacks (of which there are no known ones even for SHA1). Crypto is subtle this way which is why we shouldn't invent our own.

Finally for "no one uses it" - it is defined in Cyrus SASL hence any server/client using that library is potentially using those mechanisms. I've also found references to it in WildFly Security SCRAM SASL Implementation. So I'm afraid it's a bit late to ban its existence.

I'll follow up with them too, but those also support standard mechanisms and no serious service uses it, so the point is that it's not adding any compatibility and could cause harm later.

rufferson commented 3 years ago

This does not define those mechanisms or allow arbitrary hashes, it just defines a template for naming new mechanisms.

It does define mechanisms [family] and specifies how they should be named and which registry they should use. But yes it also has the same clause in section 10

Note to future SCRAM-mechanism designers: each new SCRAM-SASL mechanism MUST be explicitly registered with IANA and MUST comply with SCRAM-mechanism naming convention defined in Section 4 of this document.

so I agree that using non-registered names may potentially lead to poor interoperability.

This is not true. SCRAM passwords are not hashed with the hash, it's just used in an hmac. Collisions wouldn't matter, only preimage attacks (of which there are no known ones even for SHA1).

They are using PBKDF2 with HMAC as PRF to be precise. Which protects against collision but not against brute-force. The only protection against brute-force for static string is computational complexity. For PBKDF2 computational complexity is On where O is HMAC complexity and n is iterations. So computational complexity of SHA512 HMAC is higher than that of SHA256 hence it is more resistant to brute-force. 512b hash also requires slightly more memory to compute which is another step up in computing resources requirements.

I'll follow up with them too, but those also support standard mechanisms and no serious service uses it, so the point is that it's not adding any compatibility and could cause harm later.

Thanks, I do understand your drive and I will remove the implementation (from default build, will be enabled by explicit build option)

SamWhited commented 3 years ago

Thanks, I do understand your drive and I will remove the implementation (from default build, will be enabled by explicit build option)

Thank you, I appreciate this.

Neustradamus commented 3 years ago

SCRAM-SHA3-512(-PLUS):

SCRAM-SHA-512(-PLUS):

SamWhited commented 3 years ago

As I said in your other post, those are I-Ds, not accepted RFCs. If they are accepted one day, we should use them then, but only in their final form after they have had expert review. I hope that they will be accepted one day, but we don't know what they'll look like when they are or if expert review will turn up other problems before then.

Neustradamus commented 3 years ago

@SamWhited: It will be validated soon :) A good job from @aamelnikov. In several RFCs, there are some links to draft before RFC.