Bug 970718

Description of problem:

Based on the following Debian bug:
  [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=709552

in cyrus-sasl of version 2.1.25 and later the following can happen (note quote from Debian bug report mentions 2.1.24 but IILC the change was introduced for the first time in v2.1.25):

<quote>

A bug in Cyrus-SASL 2.1.24 and up causes PLAIN to be chosen over SCRAM-SHA-1
when the server ordered PLAIN before SCRAM-SHA-1 (the order in which the
server specifies the mechanisms should be ignored by the client).

This can cause a client using Cyrus-SASL to disclose its password in plain,
while it doesn't need to. Here is an example of it happening in Pidgin,
compiled with Cyrus-support:

(01:39:37) jabber: Recv (ssl)(497): <?xml version='1.0'?><stream:stream xmlns:stream='http://etherx.jabber.org/streams' version='1.0' from='xnyhps.nl' id='9531b529-e769-4c00-a17f-fae4221436da' xml:lang='en' xmlns='jabber:client'><stream:features><register xmlns='http://jabber.org/features/iq-register'/><auth xmlns='http://jabber.org/features/iq-auth'/><mechanisms xmlns='urn:ietf:params:xml:ns:xmpp-sasl'><mechanism>PLAIN</mechanism><mechanism>SCRAM-SHA-1</mechanism><mechanism>DIGEST-MD5</mechanism></mechanisms></stream:features>
(01:39:37) sasl: Mechs found: PLAIN SCRAM-SHA-1 DIGEST-MD5
(01:39:37) sasl: No worthy mechs found
(01:39:41) sasl: Mechs found: PLAIN SCRAM-SHA-1 DIGEST-MD5
(01:39:41) jabber: Sending (ssl) (test): <auth xmlns='urn:ietf:params:xml:ns:xmpp-sasl' mechanism='PLAIN' xmlns:ga='http://www.google.com/talk/protocol/auth' ga:client-uses-full-bind-result='true'>password removed</auth>
(01:39:41) jabber: Recv (ssl)(60): <success xmlns='urn:ietf:params:xml:ns:xmpp-sasl'></success>

</quote>

Version-Release number of selected component (if applicable):
cyrus-sasl/2.1.26/7.fc19

How reproducible:
Always

Steps to Reproduce:
1. Specify PLAIN SCRAM-SHA-1 DIGEST-MD5 as the mechanisms order in cyrus-sasl client
2. Watch how PLAIN is preferred before the stronger ones

Actual results:
PLAIN method is preferred before the stronger ones

Expected results:
Stronger ones should be preferred regardless of the mechanisms order specified by client (behaviour prior to 2.1.25)

Additional info:
================
Seems to have been introduced by the following upstream commit:
  http://git.cyrusimap.org/cyrus-sasl/commit/?id=80d79113041b216e5b8a6687d9d3bc26f2f1553b

Related (mentioned) ChangeLog entry (to see) is as follows:
============================================================

2011-01-21  Alexey Melnikov <alexey.melnikov>
        * include/saslplug.h, lib/client.c: Fixed handling of channel bindings
          on the client side. The client side was failing to select a suitable
          SASL mechanism when the application specified channel bindings, but
          didn't make them mandatory to use. In such a configuration, if a
          non channel binding capable mechanism was selected through
          "client_mech_list" SASL option, sasl_client_start would fail.
          For example if the server supports both SCRAM-SHA-1[-PLUS] and
          PLAIN and "client_mech_list" was set to "PLAIN", authentication
          would never work.

Relevant source code part (lib/client.c sasl_client_start()) in 2.1.23:
=======================================================================

    520 #ifdef PREFER_MECH
    521             if (strcasecmp(m->m.plug->mech_name, PREFER_MECH) &&
    522                 bestm && m->m.plug->max_ssf <= bestssf) {
    523                 /* this mechanism isn't our favorite, and it's no better
    524                    than what we already have! */
    525                 break;
    526             }
    527 #else
    528             if (bestm && m->m.plug->max_ssf <= bestssf) {
    529                 /* this mechanism is no better than what we already have! */
    530                 break;
    531             }
    532 #endif
    533 
    534             /* compare security flags, only take new mechanism if it has
    535              * all the security flags of the previous one.
    536              *
    537              * From the mechanisms we ship with, this yields the order:
    538              *
    539              * SRP
    540              * GSSAPI + KERBEROS_V4
    541              * DIGEST + OTP
    542              * CRAM + EXTERNAL
    543              * PLAIN + LOGIN + ANONYMOUS
    544              *
    545              * This might be improved on by comparing the numeric value of
    546              * the bitwise-or'd security flags, which splits DIGEST/OTP,
    547              * CRAM/EXTERNAL, and PLAIN/LOGIN from ANONYMOUS, but then we
    548              * are depending on the numeric values of the flags (which may
    549              * change, and their ordering could be considered dumb luck.
    550              */

Relevant source code part / situation (lib/client.c sasl_client_start()) in 2.1.26:
=======================================================================================

    807             /* compare security flags, only take new mechanism if it has
    808              * all the security flags of the previous one.
    809              *
    810              * From the mechanisms we ship with, this yields the order:
    811              *
    812              * SRP
    813              * GSSAPI + KERBEROS_V4
    814              * DIGEST + OTP
    815              * CRAM + EXTERNAL
    816              * PLAIN + LOGIN + ANONYMOUS
    817              *
    818              * This might be improved on by comparing the numeric value of
    819              * the bitwise-or'd security flags, which splits DIGEST/OTP,
    820              * CRAM/EXTERNAL, and PLAIN/LOGIN from ANONYMOUS, but then we
    821              * are depending on the numeric values of the flags (which may
    822              * change, and their ordering could be considered dumb luck.
    823              */
    824 
    825             if (bestm &&
    826                 ((m->m.plug->security_flags ^ bestm->m.plug->security_flags) &
    827                  bestm->m.plug->security_flags)) {
    828                 break;
    829             }
    830 
    831             if (SASL_CB_PRESENT(c_conn->cparams) && plus) {
    832                 cur_cbindingdisp = SASL_CB_DISP_USED;
    833             } else {
    834                 cur_cbindingdisp = cbindingdisp;
    835             }
    836 
    837             if (bestm && (best_cbindingdisp > cur_cbindingdisp)) {
    838                 break;
    839             }
    840 
    841 #ifdef PREFER_MECH
    842             if (strcasecmp(m->m.plug->mech_name, PREFER_MECH) &&
    843                 bestm && m->m.plug->max_ssf <= bestssf) {
    844                 /* this mechanism isn't our favorite, and it's no better
    845                    than what we already have! */
    846                 break;
    847             }
    848 #else
    849             if (bestm && m->m.plug->max_ssf <= bestssf) {
    850                 /* this mechanism is no better than what we already have! */
    851                 break;
    852             }
    853 #endif

IOW PREFER_MECH check has been moved after the compare security flags check, it was / has been in earlier versions.

While the current patch form might work for use case described in the upstream ChangeLog case (SCRAM-SHA-1[-PLUS] and PLAIN mechanisms list), I am not sure
scenario / use case described in the Debian bug report [1] has been considered too (own purpose of this bug report).

Upstream cyrus-sasl bug is at:
  https://bugzilla.cyrusimap.org/show_bug.cgi?id=3793