Re: [PATCH 2/2] _gsasl_gssapi_server_step: avoid empty challenge

[Andreas Oberritter]

On 18.10.2011 15:31, Simon Josefsson wrote:
> Andreas Oberritter <address@hidden> writes:
> 
>> - advance to step 2 immediately, if step 1 completes
>>   (maj_stat == GSS_S_COMPLETE) without any output data.
> 
> When is this needed?
> 
> The spec (RFC 4752) says:
> 
>    The server passes the initial client response to
>    GSS_Accept_sec_context as input_token, setting input_context_handle
>    to 0 (initially), chan_binding of NULL, and a suitable
>    acceptor_cred_handle (see below).  If GSS_Accept_sec_context returns
>    GSS_S_CONTINUE_NEEDED, the server returns the generated output_token
>    to the client in challenge and passes the resulting response to
>    another call to GSS_Accept_sec_context, repeating the actions in this
>    paragraph.
> 
> I think your patches alters the logic compared to the spec.  If it is
> needed, we may want to bring it up on the SASL/KITTEN list for
> confirmation.

>From my POV, there are 3 defined possibilities:

1.) GSS_Accept_sec_context fails
    -> abort
2.) GSS_Accept_sec_context returns GSS_S_CONTINUE_NEEDED
    -> send (another) challenge (i.e. return output_token to client)
3.) GSS_Accept_sec_context returns GSS_S_COMPLETE
    -> proceed to next step (i.e. send QOP and max. length)

By testing GSSAPI with a Kerberos setup, I observed that
GSS_Accept_sec_context may also return GSS_S_COMPLETE although an
output_token has been prepared and must be sent to the client (4.). This
might be erroneous behaviour of the underlying GSS library. I'm not
sure, which ones I verified it with, either GNU gss, MIT or both.

The current behaviour of gsasl is:

on 1.) abort
on 2.) return output_token to client
on 3.) return output_token to client (even though output_token may be empty)
on 4.) return output_token to client

With my patch, on 3., it will advance to the next step, because
otherwise an empty challenge will be sent to the client.

After having created the patch and before sending it to you, I verified
that this matches the behaviour of Cyrus SASL.

Regards,
Andreas

> 
> Thanks,
> /Simon
> 
> 
>> Signed-off-by: Andreas Oberritter <address@hidden>
>> ---
>>  lib/gssapi/server.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/gssapi/server.c b/lib/gssapi/server.c
>> index edc62d7..9d8ff13 100644
>> --- a/lib/gssapi/server.c
>> +++ b/lib/gssapi/server.c
>> @@ -174,8 +174,11 @@ _gsasl_gssapi_server_step (Gsasl_session * sctx,
>>        if (maj_stat == GSS_S_COMPLETE)
>>      state->step++;
>>  
>> -      res = GSASL_NEEDS_MORE;
>> -      break;
>> +      if (maj_stat == GSS_S_CONTINUE_NEEDED || *output_len) {
>> +        res = GSASL_NEEDS_MORE;
>> +        break;
>> +      }
>> +      /* fall through */
>>  
>>      case 2:
>>        memset (tmp, 0xFF, 4);