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

Consider removing SCRAM-SHA-384,512 #15

Open
SamWhited opened this issue Oct 28, 2020 · 7 comments · May be fixed by #22
Open

Consider removing SCRAM-SHA-384,512 #15

SamWhited opened this issue Oct 28, 2020 · 7 comments · May be fixed by #22

Comments

@SamWhited
Copy link

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
Copy link
Collaborator

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
Copy link
Author

SamWhited commented Oct 29, 2020 via email

@rufferson
Copy link
Collaborator

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
Copy link
Author

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
Copy link

Neustradamus commented Oct 30, 2020

@SamWhited
Copy link
Author

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
Copy link

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

@rufferson rufferson linked a pull request Apr 29, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants