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

From: Jeffrey Hutzelman <[email-masked]>
Date: Mon, 20 Jan 2014 16:07:24 -0500

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

---

From: [email-masked] (Martin Rex)
Date: Tue, 21 Jan 2014 02:10:57 +0100 (CET)

> pretty clearly says it modifies its output.

I disagree.

Potentially we're fighting about the difference of
"call-by-value" vs. "call-by-reference" in C, and what exactly 
is the meaning of "same foo" when call-by-reference is used.

Does "same same" apply when the value is the same. or only when both
value and reference/address are the same?

With that in mind, a comment like

>
> /* It is safe to call gss_release_buffer twice on the same buffer. */

is ambiguous and wrong.  Discounting empty buffers, calling
gss_release_buffer with the same buffer (=same value) more than
once is very very probably unsafe. 

And the fact that gss_release_buffer() is supposed to *CHANGE* the
buffer (struct) value should make it clear that the concept of
"same buffer" is somewhat unnatural to the GSS-API C-Bindings.

What should be safe is calling gss_release_buffer() with an empty
buffer.  So the important guidance would be to *ALWAYS* create
gss_buffer_desc structures initialized with {0, 0} in C.

The notion that gss_unwrap() could return a zero-length token is
also somewhat flawed.  gss_unwrap() returns a message (=app data)
if anything. There are no such things as zero-length tokens anywhere
throughout GSS-API.  And a gssapi mechanism should definitely not
rely on the application caller to call gss_release_buffer() on a
zero-length message returned from gss_unwrap().  The only sane
representation of an empty buffer _output_ value from a gssapi mech
is {0, 0}, i.e. be conservative in what you send (=give out).

> tokens impractical, but if you can find text that prohibits them, please

As mentioned above, the GSS-API does not have the concept of
zero-length _tokens_.  Either there is a token, or there is no token.
The reason is simple: you can not securely convey a zero-length token
over an insecure communication channel.  This is why gss_get_mic()
and gss_wrap() must produce non-zero tokens even for zero-length
messages.

-Martin