On Mon, 9 Feb 2015, Isaac Boukris wrote:

> > From my knowledge in this area that doesn't seem quite right and if we
> > can save an extra round trip, and it is valid to do so, then great ;-)
>
> It works well in my tests and according to RFC 4559 (SPNEGO) it should be
> perfectly valid.

It seems logical to me - but I will read the RFC as well to double check ;-)

> > > I've run quite several tests (with valgrind) of NTLM and Kerberos
> > > with Heimdal Kerberos GSSAPI library (also tested NTLM inside
> > > Negotiate with Heimdal's NTLMSSP).
> >
> > Was that your own tests or did you run the curl Test Suite?
>
> I've run my own tests against a Windows 2003 server (I'll checkout
> about curl's test suite).

Note: If you plan to run the test suite then you'll need to run it under Linux or msys on Windows. Marc and I got a prototype of the test suite running natively under Visual Studio whilst at FOSDEM but that is still work in progress at the moment.

> > > When investigating, I noticed the new code seem not to add '+1' to
> > > 'spn_token.length' compare to the old code.
> >
> > That might have been me :( but which commit are you referring to?
>
> I think it is commit 1cbc8fd3d1a95b90a1585d57d7af37c73cda2fc1 but can't
> tell for sure.

Yep that was me!

> The old code was:
> token.length = strlen(service) + 1 + strlen(proxy ? conn->proxy.name :
> conn->host.name) + 1;

Yeah - I couldn't quite work out which part of that code was right and which was wrong - especially as the length assignment seems to contradict the length check underneath it.

For example: If the SPN happened to be 2047 characters long (I know this is unlikely and hypothetical but please bear with me here)...

So let's use a 4 character service name such as "HTTP", then there is 1 character for the at sign, 2042 characters for the hostname and 1 character for the null terminator. This would fill the 2048 character buffer and token.length would equal 2048 with the old code. However, the following "if check" would then fail as "2048 + 1 > 2048" would be true.

That coupled with a) what we already do in socks_gssapi.c Line 151 (Not my code) and b) my SASL based GSS-API code (that formed the basis for that commit) seemed to tell me this code is wrong. I also appreciate there appears to be some contradictory code in socks_gssapi.c if I have understood the code correctly - in the instance where "serviceptr" contains a '/' as it doesn't allocate space for the null terminator and then uses memcpy to copy the string without the null terminator where as the code when "serviceptr" doesn't contain a "/" includes the null terminator in the buffer (using snprintf()) but doesn't include it in the descriptors length.

However, I'm no GSS-API expert and sasl_gssapi.c was my first attempt at programming against a GSS-API library - so I will quite happily bow down to anyone with greater experience and knowledge than me here ;-)

> > Given this, would you prefer to attempt a fix in the SSPI code, fixing
> > both at the same time, or wait for my changes in March which should
> > mean a single fix?
>
> As the logic of the changes should be the same I'll give it a try in the
> following days (and use the opportunity to practice some windows
> programming).

No problem.

Kind Regards

Steve

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2015-02-11