Re: [PATCH 1/2] _gsasl_gssapi_server_step: don't overwrite maj_stat

[Simon Josefsson]

Andreas Oberritter <address@hidden> writes:

> Hi Simon,
>
> On 18.10.2011 15:26, Simon Josefsson wrote:
>> Hi Andreas.  Thank you!  The code definitely looks buggy.  I'm not
>> convinced this is exploitable though since GSSAPI means Kerberos V5 and
>> the code will in the next steps unwrap/wrap a token which will fail
>> unless the context hasn't been negotiated successfully.  Do you agree,
>> or do you have some other analysis?
>> 
>> Your patch makes a direct comparison against GSS_S_COMPLETE which is
>> poor style in general, and using GSS_ERROR is preferred.  How about the
>> following patch instead?
>
> I considered your proposal before, but dismissed it, because if
> gss_release_buffer() fails, sasl->step would already have been
> incremented. I'm not sure whether this would be a problem, though.

No it wouldn't, if gss_release_buffer fails, the entire authentication
attempt will fail, and then it doesn't matter which state it is in.  Ok
to push revised patch?

> Because gss_release_buffer() doesn't allow any other successful return
> value than GSS_S_COMPLETE, I decided that it would be sufficient to
> compare with this value.

Yeah, it would probably work fine in practice.

> Btw.: I'm implementing a custom authentication mechanism on top of
> GSSAPI. Even though Kerberos is probably the only public user of GSSAPI,
> more unknown users might exist out there.

You really want GS2 then!  It is the generic GSS-API bridge to SASL.
The SASL mech GSSAPI only supports Kerberos V5.  If you are designing a
secure GSS mech and publish it as free software, I could add the
necessary glue to make it work via GS2 in GNU SASL.  Your mech needs to
support mutual authentication and channel binding.

/Simon

>
> Regards,
> Andreas
>
>> diff --git a/lib/gssapi/server.c b/lib/gssapi/server.c
>> index dc05a6f..2cedc2a 100644
>> --- a/lib/gssapi/server.c
>> +++ b/lib/gssapi/server.c
>> @@ -168,13 +168,13 @@ _gsasl_gssapi_server_step (Gsasl_session * sctx,
>>        memcpy (*output, bufdesc2.value, bufdesc2.length);
>>        *output_len = bufdesc2.length;
>>  
>> +      if (maj_stat == GSS_S_COMPLETE)
>> +    state->step++;
>> +
>>        maj_stat = gss_release_buffer (&min_stat, &bufdesc2);
>>        if (GSS_ERROR (maj_stat))
>>      return GSASL_GSSAPI_RELEASE_BUFFER_ERROR;
>>  
>> -      if (maj_stat == GSS_S_COMPLETE)
>> -    state->step++;
>> -
>>        res = GSASL_NEEDS_MORE;
>>        break;
>>