[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] _gsasl_gssapi_server_step: don't overwrite maj_stat
From: |
Andreas Oberritter |
Subject: |
Re: [PATCH 1/2] _gsasl_gssapi_server_step: don't overwrite maj_stat |
Date: |
Tue, 18 Oct 2011 17:07:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 |
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.
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.
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.
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;
>