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

Why is PLAIN prefered over SCRAM-SHA-1? #379

Closed
brong opened this issue May 12, 2013 · 5 comments
Closed

Why is PLAIN prefered over SCRAM-SHA-1? #379

brong opened this issue May 12, 2013 · 5 comments
Assignees

Comments

@brong
Copy link
Member

brong commented May 12, 2013

From: Thijs Alkemade
Bugzilla-Id: 3793
Version: 2.1.25
Owner: Alexey Melnikov

@brong
Copy link
Member Author

brong commented May 12, 2013

From: Thijs Alkemade

I'm trying to use Cyrus-SASL version 2.1.26 (previously we were using the library that is included in OS X, so version 2.1.22). Reason for me to update is SCRAM-SHA-1 support on XMPP.

However, I've noticed that when presented with "PLAIN SCRAM-SHA-1 DIGEST-MD5" (on a TLS-encrypted connection), Cyrus 2.1.26's first pick is PLAIN, even though I can confirm that for all three the plugin is available. That is not what I expect, as I consider it the weakest of those mechanisms.

In a debugger I figured out the following is happening:

  • sasl_client_start() gets the list of mechanisms, and sorts them based on *-PLUS or not (which doesn't change the order, as none are PLUS).
  • The for-loop on line 749 of client.c starts with PLAIN, finds the mechanism and stores it in the bestm variable, as it was still NULL.
  • It comes to SCRAM-SHA-1, compares it to bestm, but rejects it on line 828 because SCRAM-SHA-1 does not have the SASL_SEC_PASS_CREDENTIALS 'security flag' that PLAIN does have.
  • Same thing happens for DIGEST-MD5.

I do not fully undertand the meaning of SASL_SEC_PASS_CREDENTIALS, but if I'm correct that it means that the server can derive the credentials from the exchange... then I don't see how that is an advantage over mechanisms that do not have that property. In fact, I would say that the security flag should be inverted (never pick one with SASL_SEC_PASS_CREDENTIALS if the current bestm does not have SASL_SEC_PASS_CREDENTIALS).

@brong
Copy link
Member Author

brong commented Jul 18, 2013

From: Alexey Melnikov

Initial fix: 26dcfb2

Further improvements to the logic will be needed to make the fix more generic.

@brong
Copy link
Member Author

brong commented Dec 11, 2013

From: Thijs Alkemade

Sorry, it's been a while, but I've finally been able to test the changes in 26dcfb2.

The situation is somewhat different, but the basic problem is the same: given the possibilities PLAIN SCRAM-SHA-1 DIGEST-MD5, Cyrus SASL will now prefer DIGEST-MD5, then PLAIN, then SCRAM-SHA-1.

I think the reason it still happens is that SCRAM-SHA-1's max_ssf value is 0, so is PLAIN's, but DIGEST-MD5's value is at least 1 (depending on compile-time flags).

So PLAIN is selected first, it is then compared to SCRAM-SHA-1. The flags are not worse, so it is not rejected there anymore, but the check:

client.c:866:

if (bestm && m->m.plug->max_ssf <= bestssf) {

Will conclude that SCRAM-SHA-1 is not better than the mechanism we already have, so it is rejected. DIGEST-MD5 does succeed this test, so it is picked instead.

I don't know what max_ssf means exactly, but it looks wrong to me that SCRAM-SHA-1 has the same score as PLAIN.

@ksmurchison
Copy link
Contributor

Hopefully fixed by 37ab5a8

@Neustradamus
Copy link
Contributor

Neustradamus commented Jul 25, 2019

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

No branches or pull requests

4 participants