Re: [kitten] New Version Notification for draft-kaduk-kitten-gss-loop-02.txt (fwd)

[Jeffrey Hutzelman <[e-mail]>]

On Mon, 2014-01-20 at 20:32 +0100, Martin Rex wrote:
> Nico Williams wrote:
> > 
> > Nonetheless, it seems clear that the intention is that calling a
> > gss_release/delete_*() function will modify the input/output parameter
> > so that calling it again is safe, so that releasing a zero-length,
> > non-zero valued buffer ought to be safe.
> 
> In theory yes, but practice may differ.
> 
> What I'm afraid of is wrapper functions that look like this:

Well, if an app has such a wrapper, then the _wrapper_ is not safe to
call twice.  That really has nothing to do with whether
gss_release_buffer is safe to call twice, and the spec for the latter
pretty clearly says it modifies its output.

I don't think it's reasonable to make a decision about what is required
of our API on the grounds that some hypothetical person not implementing
our API might decide to not implement our API in such a way that they're
not compliant with our API.


Now, if you're talking about a mechglue, then of course such a thing
must be compliant with our API, since it is an implementation of it.
But neither requiring release to null the value pointer nor requiring
double-release to be safe is incompatible with that, provided the glue
is implemented correctly.  Your example would not be such a correct
implementation, but that doesn't mean one can't exists.




> > The API can output zero-length buffers from gss_unwrap(),
> > so the question comes up of whether such buffers can have
> > non-zero values -- the RFC doesn't say.
> 
> correct -- gss_unwrap() may return a zero-length output buffer.
> The length of the buffer must be zero for obvious reasons.
> The value of the pointer is then irrelevant, and the gssapi
> mechanism must be able to cope safely with whatever it returns
> along with a zero length.

Right.  If the mech returns a buffer with zero length and a non-NULL
value pointer, then it must be able to cope with gss_release_buffer
being called on that buffer.


> 
> 
> >
> > The other places where the API can output a zero-length token in
> > non-error conditions are gss_init/accept_sec_context() and
> > gss_delete_sec_context,
> 
> Nope, neither of these three calls can return a zero-length output token,

You know, we've discussed that question on several occasions, and I
can't remember ever being able to find something in RFC2743 that says
that.  The C bindings happen to make it impossible to tell the
difference between no token and an empty token, which makes using empty
tokens impractical, but if you can find text that prohibits them, please
do point it out.


> > and there the text is not entirely
> > dispositive...  but it is highly suggestive that zero-length tokens
> > are empty, and this, together with the general design of
> > gss_release/delete_*() strongly implies that it should be safe to call
> > any of those twice with the same input/output parameter.
> 
> No, NEVER.
> 
> The only thing that _might_ be safe, when the gssapi mechanism designer
> didn't mess up, is superfluous release calls with a NULL handle,
> i.e. calling it once with a handle that gets zeroed, and then calling
> it again with the zero-ed handle.  The big difference here is that
> the second call is *NOT* with the same handle, but with a handle
> that was zero-ed by a prior release call.  This is why I don't like
> the current language, that would suggest the wrapper describe above
> could be safely called as well (which it can not).
> 
> 
> > 
> > The only reason to assume that releasing a zero-length/non-zero value
> > buffer is that an implementation might have called malloc(0) in the
> > process of outputting a zero-length buffer from gss_unwrap()
> 
> malloc(0) is something that no reasonable gssapi mechanism
> implementation should ever do.

I think Nico needs to stop basing statements about how the API should
work on guesses as to what an implementation might or might not have
done with malloc.  You, in turn, have no cause for making pronouncements
about how an implementation might work.  Let's stick to the actual API,
folks, instead of speculation about what might be going on below the
abstraction boundary.


As it stands, RFC2744 requires that implementations zero the length
field of a released buffer, but does not require that they change the
pointer to NULL.  It requires that it be safe to release any output
buffer returned by a GSS-API routine, regardless of whether it has zero
or non-zero length or whether the value pointer is NULL or not.  It does
not require that it be safe to release a buffer that has already been
released, whether or not the first release changed the value pointer to
NULL.

So, for the purpose of the document this thread is about, assuming that
it is safe to call gss_release_buffer on an already-released buffer is a
bad idea.  The code should be structured so as to avoid this.  That
might mean keeping a flag or something.


As for the future, it's been suggested that we might want to change the
spec so that it is required that gss_release_buffer set the value
pointer to NULL and/or that calling gss_release_buffer more than once on
the same buffer(*) be safe.  If we were designing this from scratch, I'd
say that was a good idea.  However, we're not, and making the change now
adds relatively little value IMHO, since applications would have to work
correctly even with GSS-API library implementations that fail to meet
the new requirement.  As it stands, I think a better improvement would
be to RECOMMEND (but not REQUIRE) that implementations do one or both of
these things, and simultaneously remind application authors that they
cannot depend on double-release to be safe.



(*) That is, the same gss_buffer_desc, not a copy, and without any
    intervening modifications.


-- Jeff