Re: scram-sha-1-plus

[Jeremy Harris]

On 21/01/2020 17:03, Simon Josefsson wrote:
> 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);

That's incredibly fragile.  Almost begging to be broken by
code-maintenance five years down the line.


> For a threaded application, the answer (today) is to have one
> gsasl_init() context per thread,

That's... somewhat ugly.  It begs the question as to what the
intent of this level of context.   Possibly documenting the
intent would help, but then the question becomes what the
added value of the session context is?

> alternatively do the two operations
> above under a mutex.

Also ugly.


> 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.

You're probably right about those being application-static - though
I could imagine a application wanting to offer multiple services...
POP and IMAP for a start, if not also SMTP.

>  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.

It is not threaded.  Both in client and server auth modes, it has a
single process for the job.  So it's operational now via what I regard
as hacky coding, and not broken.

>> 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?

I think that facility should be session-dependent, because sessions
differ.  Specifically, it should take a session-context handle not
a toplevel context handle for the API.

Exim doesn't use it - we assume the admin writing the exim config
is prepared to take the hit of errors when he gets it wrong.  So
the automatics offered by "suggestion" are not needed.

However, if you took the "split into two" choice:

1) API call: application gets fresh, bare, session-context
2) application couples that with the TLS channel in app-level data
   struct.
3) Optionally, app sets channel-binding data
4) API call: with session-context, start requested METHOD
   (triggers prop callback if needed for channel-binding data)

- then if the app did indeed do (3), it could use a "suggestions"
call for the session-context before doing (4).

Since I don't need "suggestions" I've not thought too hard about it,
but does that satisfy the need?

>> 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:

Yup.  Exim can do the advertising, because it has, at application level,
knowlege of the availability of binding data.  There's no library
involvement needed.

I don't know if that applies in other applications though.

>    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.
[more on that flag]

Yes.  The library handles all that.

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

I'm not sure where that library does any advertising, though?

> 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().

or the split session-start.

>    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.

-- 
Cheers,
  Jeremy