Re: scram-sha-1-plus

[Simon Josefsson]

Thanks for helping me think through this -- at the end of this email I
realize we should do something, but I'm not yet certain of how the code
should look like.

Jeremy Harris <address@hidden> writes:

> On 20/01/2020 23:49, Simon Josefsson wrote:
>> 3) Redesign gsasl somehow.
>> 
>> It is late and I'm not sure there actually is a problem for non-threaded
>> applications.  Is there really any problem?
>
> The application can hack it to work, so long as it only
> has one session for its toplevel library context - but it is ugly.
> And, as you note, multiple sessions are a problem.

It is only a serious problem for threaded applications, right?  Can you
identify a real problem for a non-threaded application?  I don't see it
now.

As far as I can tell, non-threaded applications can get completely
predictable and correct results by making sure a client initialized as

  gsasl_callback_set (ctx, callback);
  gsasl_callback_hook_set (ctx, &global_application_state)

keep the following two calls together:

  strcpy (global_application_state->tls_unique, "foo");
  /* the global callback now has the right tls-unique data for the
  session to start */
  res = gsasl_client_start (ctx, "SCRAM-SHA-256-PLUS", &client);

After these calls, the callback will never be called for the
GSASL_CB_TLS_UNIQUE property again during the SCRAM process.  (However
knowing this for certain is only possible by reading gsasl code...)

It is only if the two last operations are separated and performed in
completely different context that there could be trouble -- consider if
the TLS handshake function write to global_application_state->tls_unique
directly after the handshake, and the application has many concurrent
connections, and there is a chance that another TLS handshake has
finished before gsasl_client_start() is invoked.  Then the callback
can't be sure the tls-unique data belongs to the intended session.  But
this can be avoided by always placing the two logical operations above
close together.

For a threaded application, the answer (today) is to have one
gsasl_init() context per thread, alternatively do the two operations
above under a mutex.

But gsasl is supposed to be thread-safe and not require use of mutexes,
nor per-thread gsasl_init() calls, so I do agree this is a real bug that
needs fixing.  For non-threaded application it is merely a poorly
designed API but not a completely bad situation.

Looking at the code, the GS2 and GSSAPI server plugins have the same
"problem" today: they retrieve the GSASL_SERVICE and GSASL_HOSTNAME
properties during gsasl_server_start().  I suspect there aren't any
applications out there that want to provide different answers to these
callbacks depending on the session, so this was never a problem for
anyone.  They are application-global and static.  But
GSASL_CB_TLS_UNIQUE is different since it really is different for every
session, and the callback need to be able to tell sessions apart.

Is exim threaded?  Will it ever act as a SASL client for concurrent TLS
sessions in the same process?  Even if the answer is no (meaning you are
okay), we should fix this problem anyway.

> To keep the current early check, I think you would have to split the
> session startup into two calls, so the the application gets a session-
> context before the time it needs to provide the channel-binding data.

Yes, this would solve the generic problem, however we still have the
problem with gsasl_client_suggest_mechanism() -- that function should
recommend SCRAM-SHA-1-PLUS (or GS2-KRB5-PLUS) if the TLS channel binding
is available but not otherwise.  How to implement that?

> Alternatively it should be possible to fail the flow later on, if it
> is -PLUS but the binding prop has not been provided at the time
> it is actually required.  After all, other props are required
> for the conversation (eg the authn) and the client application
> is not required to provide them so early...  On that view, the
> library should permit the application to try to do the wrong
> thing early on - only error-checking later.

Yes this is possible but it means SCRAM-SHA-256-PLUS will be attempted
all the time if the application trusts gsasl_client_suggest_mechanism(),
even if it won't even be able to produce the first token for the server
if the application doesn't provide the tls-unique data.

Which of the PLUS and non-PLUS mechanisms are advertised affects the
client/server messages, see RFC 5802:

   The "-PLUS" suffix is used only when the server supports channel
   binding to the external channel.  If the server supports channel
   binding, it will advertise both the "bare" and "plus" versions of
   whatever mechanisms it supports (e.g., if the server supports only
   SCRAM with SHA-1, then it will advertise support for both SCRAM-SHA-1
   and SCRAM-SHA-1-PLUS).  If the server does not support channel
   binding, then it will advertise only the "bare" version of the
   mechanism (e.g., only SCRAM-SHA-1).  The "-PLUS" exists to allow
   negotiation of the use of channel binding.  See Section 6.
...
   o  Servers that support the use of channel binding SHOULD advertise
      both the non-PLUS (SCRAM-<hash-function>) and PLUS-variant (SCRAM-
      <hash-function>-PLUS) mechanism name.  If the server cannot
      support channel binding, it SHOULD advertise only the non-PLUS-
      variant.  If the server would never succeed in the authentication
      of the non-PLUS-variant due to policy reasons, it MUST advertise
      only the PLUS-variant.

   o  If the client supports channel binding and the server does not
      appear to (i.e., the client did not see the -PLUS name advertised
      by the server), then the client MUST NOT use an "n" gs2-cbind-
      flag.

   o  Clients that support mechanism negotiation and channel binding
      MUST use a "p" gs2-cbind-flag when the server offers the PLUS-
      variant of the desired GS2 mechanism.

   o  If the client does not support channel binding, then it MUST use
      an "n" gs2-cbind-flag.  Conversely, if the client requires the use
      of channel binding then it MUST use a "p" gs2-cbind-flag.  Clients
      that do not support mechanism negotiation never use a "y" gs2-
      cbind-flag, they use either "p" or "n" according to whether they
      require and support the use of channel binding or whether they do
      not, respectively.

   o  Upon receipt of the client-first message, the server checks the
      channel binding flag (gs2-cbind-flag).

      *  If the flag is set to "y" and the server supports channel
         binding, the server MUST fail authentication.  This is because
         if the client sets the channel binding flag to "y", then the
         client must have believed that the server did not support
         channel binding -- if the server did in fact support channel
         binding, then this is an indication that there has been a
         downgrade attack (e.g., an attacker changed the server's
         mechanism list to exclude the -PLUS suffixed SCRAM mechanism
         name(s)).

      *  If the channel binding flag was "p" and the server does not
         support the indicated channel binding type, then the server
         MUST fail authentication.

Currently gsasl link availability of GSASL_CB_TLS_UNIQUE together with
advertisement of PLUS variants.

The more I think of this now, the problem is

   a) calling gsasl_client/server_start() should never trigger a
      callback, or at least not a callback for a session-specific
      property (because the application will be in a difficult situation
      to answer it correctly without per-session hooks).  Therefor, we
      should move the GSASL_CB_TLS_UNIQUE callback to
      gsasl_client/server_step().

   b) gsasl_client_suggest_mechanism() was designed assuming the current
      broken design and has to be redesigned.  Maybe we can live with it
      returning SCRAM-*-PLUS which may not complete if the client does
      not provide GSASL_CB_TLS_UNIQUE?

   c) if we fix a) we should review SCRAM client/server code so that it
      does the right thing wrt PLUS.  The logic is a bit complicated.

I'll take dinner and let this sink in and return later...

/Simon

signature.asc
Description: PGP signature