Fixes for S3 DCE/RPC GSSAPI with Heimdal

simo idra at samba.org
Sun Apr 17 17:08:55 MDT 2011


On Mon, 2011-04-18 at 06:59 +1000, Andrew Bartlett wrote:
> Luke,
> 
> I've asked you below about this history of PAC functions in MIT.  Did
> MIT kerberos ever allow PAC access without doing the crypto, authtime
> and principal checking?
> 
> On Sat, 2011-04-16 at 12:16 -0400, simo wrote:
> > On Sat, 2011-04-16 at 19:58 +1000, Andrew Bartlett wrote:
> > > Simo,
> > > 
> > > I've been working to test the Samba3 binaries produced by the top level
> > > build, and this builds against Samba4's Heimdal at this time.
> > > 
> > > When you proposed your DCE/RPC GSSAPI patches, you asked that I check
> > > them against Heimdal, and sadly I only got as far are compiling them,
> > > not running them.
> > > 
> > > These patches makes the DCE/RPC GSSAPI server work with the newly added
> > > ktest tests in Samba3's make test, when run from the top level build. 
> > > 
> > > Can you let me know if these changes are OK, or if you want some further
> > > explaination?
> > > 
> > > http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/krb5-fix
> > > 
> > > In particular I'm referring to:
> > > 
> > > s3-gse: Allow the GSSAPI wrapper to load a keytab using
> > > gss_krb5_import_cred():
> > > http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=abfe0bb3a73a3d00d0e75ae2405bf064f6abbf89
> > 
> > Why did you remove the ability to pass in an arbitrary keytab ?
> 
> Because it was unused, and the Heimdal function involved takes an open
> keytab, not a name.  I just didn't want to open and figure out how to
> reference an additional keytab if no caller used that facility.  Do you
> have plans for this function that involve a keytab that Samba doesn't
> manage?

Technically yes, but I haven't needed it so far. I just don't want to
break Heimdal later if it turns out I need it and revert this change.

> > > s3-gse: Don't release the mech OID from gss_accept_security_context
> > > http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=e5eadad3bce2b1f57ffb01aa65b6880fd5fe20c4
> > 
> > The MIT API seem to require you to free it.
> > MIT's gss_release_oid() internally recognizes if the OID is one of its
> > constants and in that case it just returns without freeing.
> 
> Wow!
> 
> > If Heimdal doesn't do that you'll need an ifdef, or an auxiliary
> > function named gse_release_oid() with the ifdef in it (my preferred way
> > to deal with ifdefs is to bury them into their own wrapper function).
> 
> I'll look into this again, I know the two have API differences, but this
> one takes the cake for subtle breakages!
> 
> If I can show that the MIT API also returns a constant but allows
> (rather than requires) gss_release_oid() to avoid a segfault, would you
> be willing to avoid this call?

I know MIT often returns constants, but it clearly states it may not in
some cases. If you are sure we never hit a case where a non-constant is
returned I guess we can remove this call. I just don't want to have to
chase mem leaks a few years down the road.

> > > s3-gse Use Heimdal gsskrb5_extract_authz_data_from_sec_context when
> > > available:
> > > http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=8b6c1c2b51566f74518bfa7ab4829c011b49cbad
> > 
> > I see the comment on this one, but it looks very strange it doesn't work
> > in Heimdal, this is normal GSSAPI stuff.
> > 
> > I see that the function you use is also available in the MIT code, so
> > perhaps leave gse_get_authz_data() alone and instead create a
> > gse_get_pac_data() that calls gse_get_authz_data() + unwrap_pac() as a
> > fallback.
> > 
> > I want to keep this authz function more neutral as I want later to be
> > able to plug in easily the ability to fetch different authz data in
> > future.
> 
> Sure, I can do that. 
> 
> > > s3-gse Don't get the auth time when validating the PAC:
> > > http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=85350063468dca54aefc4cc905d13d4aaa81ddd0
> > 
> > I don't like this one.
> > If it breaks against Heimdal can you simply ifdef out it's usage only
> > when Heimdal is used, adding a comment that it doesn't work with
> > Heimdal ? (Why wouldn't it ?? If it already does the check, shouldn't
> > this one just work as well ?)
> 
> I think the issue is that in both cases that particular OID isn't
> actually connected.  So the API call (essentially an ioctl) exists, but
> if there isn't an implementation for that particular OID, then we error.
> 
> > I had no problems with this testing with MIT, and I am not completely
> > sure MIT does the checks itself (haven't checked).
> 
> MIT does check, I found this out when chasing other bugs a while ago,
> because they found a DES-related bug a while ago in that code, which
> inspired my fix :-).  
> 
> Luke:  Do you know if there are version of MIT Kerberos with PAC access
> functions that don't do the PAC checks?
> 
> You can't do the check properly anyway (because you don't have the key,
> and the principal name is the part you really need to check to make this
> secure), so why not leave this to the library to do?

If we are sure this is always done for us, I am ok removing it.

> > > I don't yet have the autoconf/waf tests for the new macros (allowing a
> > > build against a system Heimdal), but I'll add those soon.  What I'm
> > > after at the moment is your comment on the meat of these patches.   
> > > 
> > > I can also address the unused variables (in each arm of the #if/#else),
> > > but didn't want the patches to be full of just noise at this stage of
> > > review). 
> > 
> > I am ok leaving unused variables in the code without ifdefs, I prefer a
> > few warnings then ugly code blocks in the middle of a function just to
> > add a couple of variables. But this is just style preference, if people
> > feel strongly about unused vars warnings, we can find a way to deal with
> > properly ifdeffing also unused vars.
> 
> We could finally decide to use C99 and allow inline variables.  We use
> so much of C99 anyway :-)

Not sure I'd really like variables defined in the middle of the code,
makes reading code harder imo, but that's just taste.

> > > The good news is that with these patches, we can successfully test
> > > Samba3 from the top level build, and means we are but a short way away
> > > from testing Samba3 in combination with Samba4 in the combined build. 
> > 
> > That's nice,
> > thanks.
> 
> I'll fix this up and send it back to you. 

Thanks,
Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>



More information about the samba-technical mailing list