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

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 14 Jan 2014 16:55:08 -0500 (EST)

I made some minor edits to the body text and trimmed down the sample code 
to be more consistent with Greg's suggestions.  I think Greg wants the 
sample code trimmed even further (i.e., to not have anything in the bodies 
of the {send,receive}_token functions, and possibly not have them at all, 
but I prefer to be more concrete.

Is it reasonable to adopt this document as a working-group draft at this 
time?

I am becoming more convinced that we should have Java sample code as well 
as C, but due to some computer issues I don't have a java development 
environment available at the moment.  My Java is also quite rusty, so if 
someone else wants to chip in, that would be great.

Finally, the C sample code has a comment (in two places, actually):
/* It is safe to call gss_release_buffer twice on the same buffer. */
This could potentially be misleading, as I believe our conclusion was that 
2743/2744 do not explicitly require this to be safe.  However, we believe 
that it is safe in all known implementations, and that an implementation 
where it was not safe would be very difficult to use correctly.  Is it 
reasonable to leave this comment in place under the reasoning that it is 
describing the assumptions made by the sample code (as opposed to 
describing the behavior mandated by the specification)?

Thanks,

Ben

---------- Forwarded message ----------
Date: Tue, 14 Jan 2014 13:27:45 -0800
From: [email-masked]
To: Benjamin Kaduk <[email-masked]>, Benjamin Kaduk <[email-masked]>
Subject: New Version Notification for draft-kaduk-kitten-gss-loop-02.txt

A new version of I-D, draft-kaduk-kitten-gss-loop-02.txt
has been successfully submitted by Benjamin Kaduk and posted to the
IETF repository.

Name:		draft-kaduk-kitten-gss-loop
Revision:	02
Title:		Structure of the GSS Negotiation Loop
Document date:	2014-01-14
Group:		Individual Submission
Pages:		17
URL:            http://www.ietf.org/internet-drafts/draft-kaduk-kitten-gss-loop-02.txt
Status:         https://datatracker.ietf.org/doc/draft-kaduk-kitten-gss-loop/
Htmlized:       http://tools.ietf.org/html/draft-kaduk-kitten-gss-loop-02
Diff:           http://www.ietf.org/rfcdiff?url2=draft-kaduk-kitten-gss-loop-02

Abstract:
    This document specifies the generic structure of the negotiation loop
    to establish a GSS security context between initiator and acceptor.
    The control flow of the loop is indicated for both parties, including
    error conditions, and indications are given for where application-
    specific behavior must be specified.

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

The IETF Secretariat

---

From: Greg Hudson <[email-masked]>
Date: Fri, 17 Jan 2014 16:06:43 -0500

On 01/14/2014 04:55 PM, Benjamin Kaduk wrote:
> I made some minor edits to the body text and trimmed down the sample
> code to be more consistent with Greg's suggestions.  I think Greg wants
> the sample code trimmed even further (i.e., to not have anything in the
> bodies of the {send,receive}_token functions, and possibly not have them
> at all, but I prefer to be more concrete.

My reasoning is that we cannot be concrete in a way which will be useful
to very many applications.  Currently your stub send_token() and
receive_token() functions accept fd arguments, for instance, while a
typical caller would have some more complicated connection state object.

On a similar note, typical callers probably won't want to use warnx().

> Is it reasonable to adopt this document as a working-group draft at this

I think it is reasonable.  To recap, the goal here is to reduce the
amount of verbiage that protocol standards (AFS, LDAP, etc.) must use to
describe how implementors should use GSSAPI, by giving them something
they can refer to.

> I am becoming more convinced that we should have Java sample code as
> well as C

I think that would be a fine thing, but shouldn't block the draft from
progressing.

> This could potentially be misleading, as I believe our conclusion was
> that 2743/2744 do not explicitly require this to be safe.  However, we
> believe that it is safe in all known implementations, and that an
> implementation where it was not safe would be very difficult to use
> correctly.  Is it reasonable to leave this comment in place under the
> reasoning that it is describing the assumptions made by the sample code
> (as opposed to describing the behavior mandated by the specification)?

I think it's reasonable to retroactively standardize this behavior of
gss_release_buffer, if it's a property of every implementation we're
aware of.

But if we're not going to do so (because gss-loop isn't a normative
document and we're not prepared to issue a 2744 update), then I don't
think it's appropriate for the sample code in gss-loop to be making that
assumption.  Unfortunately, I don't know of a nice elegant way to
restructure the code so that the assumption isn't necessary.

---

From: [email-masked] (Martin Rex)
Date: Sat, 18 Jan 2014 02:01:11 +0100 (CET)

> gss_release_buffer, if it's a property of every implementation we're

I hadn't notice this strange comment about gss_release_buffer() before,
and I'm voilently opposed, please remove that misleading comment.

None of the GSS-API calls makes any kind of promise that calling
it twice would be safe.  Actually, if you look at how the C-Bindings
were designed, just the opposite has to be assumed.

The convention to pass the pointers (and gss_buffer_desc) by reference
to the release function, so that the function could zero out the
references/pointers to resources that were allocated by the library
appears specifically designed to make it less likely that a program
will crash when calling a function twice -- _provided_ that it passes
the _original_ reference to the release function (which can then be zeroed),
rather than a mere copy of it (which could not possibly protect from
being called again with other non-zero copies of the original).

The only thing that a release function can be expected to recognize
and handle safely is being called with an already zero-ed handle
or already-zeroed value pointer in gss_buffer_desc.

-Martin

---

From: Nico Williams <[email-masked]>
Date: Fri, 17 Jan 2014 19:18:17 -0600

Martin,

The C bindings release/delete functions generally take a pointer to a
variable that holds the thing being released precisely so they can set
that variable to the appropriate GSS_C_NO_* value.

Now, I realize that RFC2744 isn't the model of completeness -- it
evidently doesn't say this one really important thing (from a quick
skim before running off to dinner).  I don't really care that it
doesn't say that (if indeed it doesn't).  It should have, as it's very
clearly the intended behavior (otherwise why on earth take that
pointer instead of just the value to be released/deleted?) and in fact
the behavior that the implementations I'm familiar with have.  An
update should say it.  I'd be OK with a note that there might be
implementations that don't do this.

Nico
--

---

From: Russ Allbery <[email-masked]>
Date: Fri, 17 Jan 2014 17:39:58 -0800

> that variable to the appropriate GSS_C_NO_* value.

I think Martin's point is that the following is not safe:

    gss_buffer_desc buf1, buf2;
    gss_OID oid;
    gss_name_t name;
    OM_uint32 major, minor;

    /* ... */

    major = gss_display_name(&minor, name, &buf1, &oid);
    buf2 = buf1;
    gss_release_buffer(&minor, &buf1);
    gss_release_buffer(&minor, &buf2);

Similar sorts of code work with other GSS-API objects.  It doesn't really
matter whether the variable type is actually a struct or a pointer; either
way, once you start making copies, double frees are still a risk.

Now, obviously, you generally should avoid doing that sort of thing in
GSS-API code precisely because it's not safe.  But saying that it's
permissible to pass the same buffer to gss_release_buffer multiple times
implies that this sort of thing is safe, and I'm pretty sure it isn't.

-- 
Russ Allbery ([email-masked])              <http://www.eyrie.org/~eagle/>

---

From: Greg Hudson <[email-masked]>
Date: Fri, 17 Jan 2014 22:28:39 -0500

> and I'm voilently opposed, please remove that misleading comment.

The comment in the draft is trying to say that it's okay to call
gss_release_buffer twice with the same reference to a gss_buffer_desc
structure, trusting that the first call will put the structure into a
state where the second call is a no-op.  Unfortunately, RFC 2744 does
not appear to guarantee this; 5.26 only requires that the length is set
to 0 after the storage is freed (implementations are merely "encouraged"
to zero the pointer field), and does not guarantee that a second call
won't double-free the storage.

If it is okay for the sample code to rely on this behavior, in spite of
RFC 2744 not specifying that it is okay, then perhaps the comment should
be clarified or removed to avoid confusing people like it did Martin.

---

From: Nico Williams <[email-masked]>
Date: Sat, 18 Jan 2014 00:18:38 -0600

>     gss_release_buffer(&minor, &buf2);

I'm not sure that that's what he meant, but if that's what he meant
then that's fine with me.  No matter what sort of type gss_buffer_desc
might be (a struct here, but suppose it was an int or a pointer), and
considering the side-effects of gss_release_buffer(), that could not
possibly be safe.

But I think Martin meant that this is not supposed to be safe:

    gss_buffer_desc buf;
    gss_OID oid;
    gss_name_t name;
    OM_uint32 major, minor;
     /* ... */
     major = gss_display_name(&minor, name, &buf, &oid);
     gss_release_buffer(&minor, &buf); /* sets buf to {0, NULL} */
     gss_release_buffer(&minor, &buf); /* releasing {0, NULL} is safe,
like free(NULL) */

> way, once you start making copies, double frees are still a risk.

Right, the type could be an integer, a pointer, a struct -- it doesn't matter.

Of course, an implementation could protect against double-release in
some cases.  But they aren't and shouldn't be required to.

What I'm saying is that the design of the C bindings' release/delete
functions is meant to set the variable whose pointer is passed in to
the zero/null/empty value.  No other interpretation is possible.  Why
ever bother taking a pointer to a variable instead of the value
otherwise?

Oh, and see the text in RFC2744, section 3.9.2:

   The minor_status parameter will always be set by a GSS-API routine,
   even if it returns a calling error or one of the generic API errors
   indicated above as fatal, although most other output parameters may
   remain unset in such cases.  However, output parameters that are
   expected to return pointers to storage allocated by a routine must
   always be set by the routine, even in the event of an error, although
   in such cases the GSS-API routine may elect to set the returned
   parameter value to NULL to indicate that no storage was actually
   allocated.  Any length field associated with such pointers (as in a
   gss_buffer_desc structure) should also be set to zero in such cases.

That only leaves the question of whether gss_release_buffer()'s
gss_buffer_t argument is an input/output parameter (it clearly has to
at least be an input parameter, but also clearly no actual output
other than {0, NULL} is intended!).

> GSS-API code precisely because it's not safe.  But saying that it's

Or with just about *any* API.  You shouldn't close(2) the same fildes
twice, for example, nor free(3) the same pointer twice, nor munmap(2)
the same address twice, and so on.

> implies that this sort of thing is safe, and I'm pretty sure it isn't.

This is clearly intended to be safe:

    gss_buffer_desc buf;
    gss_OID oid;
    gss_name_t name;
    OM_uint32 major, minor;
     /* ... */
     major = gss_display_name(&minor, name, &buf, &oid);
     gss_release_buffer(&minor, &buf); /* sets buf to {0, NULL} */
     gss_release_buffer(&minor, &buf); /* releasing {0, NULL} is safe,
like free(NULL) */

Nico
--

---

From: Nico Williams <[email-masked]>
Date: Sat, 18 Jan 2014 00:22:46 -0600

> won't double-free the storage.

RFC2744, section 5.26 requires nothing.  It doesn't say "MUST", and so
on (because RFC2744 doesn't reference RFC2119, of course).  It doesn't
say that the value field will not be set.  Section 3.9.2 seems to
indicate that the value field ought to be set, but only if the buffer
is an output buffer.

> be clarified or removed to avoid confusing people like it did Martin.

I would just note that RFC2744 can be interpreted as permitting
gss_release_buffer() implementations to set the length field but not
the value field.  But I'd rather just say that RFC2744 just has a
glaring omission... because that's what it is.

Nico
--

---

From: Jeffrey Hutzelman <[email-masked]>
Date: Sat, 18 Jan 2014 02:40:43 -0500

On Sat, 2014-01-18 at 00:22 -0600, Nico Williams wrote:

> on (because RFC2744 doesn't reference RFC2119, of course).

No, it pretty clearly says "the routine will zero the length field...".
The text is descriptive in its phrasing, because the whole document is
written more like API documentation for callers than a standard for
implementors.  However, it is in fact the latter, and so a statement
about what a routine _will_ do is pretty clearly prescriptive in intent.

Similarly, it's pretty clear that releasing a buffer with NULL value is
required to be safe, per the last sentence in that same paragraph.  

>   It doesn't
> say that the value field will not be set.

No.  It fairly clearly leaves that up to the implementor.

>   Section 3.9.2 seems to

Section 3.9.2 requires that the value be set for an output parameter
that is "expected to return pointers to storage allocated by a routine".
That's sane; it means, for example, that after you call a GSS-API
routine that has a buffer output, it is always safe to call
gss_release_buffer on that buffer, whether or not the routine succeeded,
and whether or not it actually allocated any storage.

However, section 3.9.2 has nothing to say about the buffer descriptor
passed to gss_release_buffer, even if we consider that to be an output
buffer.  Why?  Because the purpose of gss_release_buffer is to free
storage, not to allocate it.  More particularly, the buffer parameter to
this routine is not used to return a pointer to allocated storage; it is
used to pass in a pointer to storage to be freed, possibly with the
side-effect that the pointer is set to NULL, but certainly not that it
is set to point to allocated storage!

Perhaps more directly to the point, 3.9.2 is part of a section
describing general calling conventions, while the text in 5.26 pertains
specifically to gss_release_buffer _and_ has something specific to say
about the pointer (namely, that implementations are "encouraged" to set
it to NULL.  This, I thikn, has about the same strength that naive
readers assign to "SHOULD").

> glaring omission... because that's what it is.

Except I wouldn't call it an omission, in that it appears quite
deliberate.  Why say that implementations are "encouraged" to set the
pointer to NULL, rather than either making it required or saying nothing
at all?  Perhaps to avoid rendering some existing implementation(s)
noncompliant?

The reality is that people tend to copy and paste example code, often
without really understanding it.  To some extent, that's the point -- we
want people to be able to get this right _without_ investing months or
years becoming expert enough to work out all of the possible cases.  In
light of that, I think such example code needs to be of the highest
quality, which certainly means it should not make unfounded assumptions
about the interface it purports to document.

-- Jeff

---

From: Nico Williams <[email-masked]>
Date: Sun, 19 Jan 2014 00:06:58 -0600

I'm an idiot.  I missed the "encouraged" part somehow.

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.  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.
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, 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.

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(), and the
malloc() implementation happens to return non-NULL when called with
zero as its argument.  That's clearly a bug.  There should be no
reason to release empty buffers.

At any rate this is all rather incidental to Ben's I-D and doesn't
need to be addressed at this time.  If we ever update RFC2744, I would
insist on changing that "encouraged" to "MUST" with some text about
how earlier versions of the API did not have that requirement,
therefore applications SHOULD zero the value in the buffer after
releasing it.

Nico
--

---

From: [email-masked] (Martin Rex)
Date: Mon, 20 Jan 2014 20:32:25 +0100 (CET)

Nico Williams wrote:
> 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:

OM_uint32 app_wrapper_gss_release_cred(gss_ctx_id_t cred)
{
    OM_uint32      maj_stat;
    OM_uint32      min_stat;
    gss_cred_id_t  l_cred = cred;

    maj_stat = gss_release_cred(&min_stat, &l_cred);
    if ( GSS_S_COMPLETE!=maj_stat ) {
        app_show_error( maj_stat, min_stat);
    }

    return(maj_stat); 
}

And if that app_wrapper_* release function is called twice,
then a non-zero copy of the original cred will be passed on the
second call, and that may result in the undefined behaviour
of double-free().

>
> 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.

> gss_delete_sec_context,

Nope, neither of these three calls can return a zero-length output token,
they either return a token or do not return a token (which means that
a gssapi wrapper mechanism that would blindly copy output parameters
from underlying implementations and use an implementation specific
malloc(0) would be buggy and leak memory for these three functions.

>
> and there the text is not entirely

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).

> process of outputting a zero-length buffer from gss_unwrap()

malloc(0) is something that no reasonable gssapi mechanism
implementation should ever do.

-Martin