Re: WG Last Call: draft-ietf-sasl-gs2-02.txt

[Simon Josefsson <jas@xxxxxxxxxxx>]

Alexey Melnikov <alexey.melnikov@xxxxxxxxx> writes:

> My review comments are below. Some of them you probably already
> addressed and some of them are nitpicking.

Thanks for your detailed review!  I'm sorry it took so long to
respond.

>>4.1.  SASL Tokens
>>
>>   All top-level SASL packets for the GS2 mechanism family follow the
>>   following format:
>
> I would suggest avoid using the term SASL packet, as it usually refers to
> SASL security layer. Something like "SASL authentication exchange packets"
> would be better here.

RFC 4422 seems to use the term "messages" here, so I changed this
into:

<section title="SASL Authentication Exchange Message Format">

  <section title="SASL Messages">

    <t>All top-level SASL authentication exchange messages for the GS2
      mechanism family follow the following format:</t>

>>4.2.  Context Token
>
> [...]
>
>>   The client calls GSS_Init_sec_context, passing in
>>   input_context_handle of 0 (initially), mech_type of the GSSAPI
>>   mechanism for which this SASL mechanism is registered, the
>>   chan_binding is set to NULL, and targ_name equal to output_name from
>>   GSS_Import_Name called with input_name_type of
>>   GSS_C_NT_HOSTBASED_SERVICE and input_name_string of
>
> The GS1 has the note marker (*) after the "GSS_C_NT_HOSTBASED_SERVICE",
> the note reads:
>
>   (*) - Clients MAY use name types other than
>   GSS_C_NT_HOSTBASED_SERVICE to import servers' acceptor names, but
>   only when they have a priori knowledge that the servers support
>   alternate name types.  Otherwise clients MUST use
>   GSS_C_NT_HOSTBASED_SERVICE for importing acceptor names.
>
> This is to address recent Nico's proposal to allow for domain based names.

I've added this.

Is something similar needed for the server side too?

> [...]
>
>>   If the client will be requesting a security
>>   layer, it MUST also supply to the GSS_Init_sec_context a
>>   mutual_req_flag of TRUE, a sequence_req_flag of TRUE, and an
>>   integ_req_flag of TRUE.  If the client will be requesting a security
>>   layer providing confidentiality protection, it MUST also supply to
>>   the GSS_Init_sec_context a conf_req_flag of TRUE.
>
> This text got replaced with the following in GS1:
>
>   When calling the GSS_Init_sec_context the client
>   MUST pass the integ_req_flag of TRUE.  If the client will be
>   requesting a security layer, it MUST also supply to the
>   GSS_Init_sec_context a mutual_req_flag of TRUE, and a
>   sequence_req_flag of TRUE.  If the client will be requesting a
>   security layer providing confidentiality protection, it MUST also
>   supply to the GSS_Init_sec_context a conf_req_flag of TRUE.

Thanks, I had already made that change.  I had also added:

  The client MUST
      verify that the requested flags become enabled in the
      context.

Because, if I understand correctly, setting the flags does not
necessarily mean that they will be set, so the client has to check
that they were indeed enabled.

>>   If GSS_Init_sec_context returns GSS_S_CONTINUE_NEEDED, then the client
>>   expect the server to issue a token in a subsequent challenge or as
>>   additional information to the outcome of the authentication.
>
> I suggest adding before this sentence (as in GS1):
>
>   The client then responds with the resulting output_token.
>
> otherwise it is not clear what happens to the generated token.

Good idea, added.

>>   The server passes the first client response to GSS_Accept_sec_context
>
> I suggest s/first/initial, as "initial client response" is a SASL term.

GS2 should support a missing "initial client response", so it is
actually the first context token received by the server which should
be used.  Does this looks better?

    <t>The server passes the first context token to
      GSS_Accept_sec_context as input_token, setting

>>   as input_token, setting input_context_handle to 0 (initially),
>>   mech_type of the GSSAPI mechanism for which this SASL mechanism is
>>   registered,
>
> Delete description of the mech_type parameter, as it is an output parameter
> of GSS_Accept_sec_context.

Oops!  Good catch.  Fixed.

>> the chan_binding set to NULL, and acceptor_cred_handle
>>   equal to output_cred_handle from GSS_Acquire_cred called with
>>   desired_name equal to output_name from GSS_Import_name with
>>   input_name_type of GSS_C_NT_HOSTBASED_SERVICE and input_name_string
>>   of "service@hostname" where "service" is the service name specified
>>   in the protocol's profile, and "hostname" is the fully qualified host
>>   name of the server.
>
> The text about acceptor_cred_handle got replaced with the following in GS1:
>
>   and a suitable acceptor_cred_handle (see below).
>
> and later:
>
>   Servers SHOULD use a credential obtained by calling GSS_Acquire_cred
>   or GSS_Add_cred for the GSS_C_NO_NAME desired_name and the OID of the
>   GSS-API mechanism for which this SASL mechanism is registered
> (*). Servers MAY use
>   GSS_C_NO_CREDENTIAL as an acceptor credential handle.  Servers MAY
>   use a credential obtained by calling GSS_Acquire_cred or GSS_Add_cred
>   for the server's principal name(s) (**) and the
>   GSS-API mechanism for which this SASL mechanism is registered.
>
>   (*) - Unlike GSS_Add_cred the GSS_Acquire_cred uses an OID set of
>   GSS-API mechanism as an input parameter.  The OID set can be created
>   by using GSS_Create_empty_OID_set and GSS_Add_OID_set_member.  It can
>   be freed by calling the GSS_Release_oid_set.
>
>   (**) - Use of server's principal names having
>   GSS_C_NT_HOSTBASED_SERVICE name type and "service@hostname" format,
>   where "service" is the service name specified in the protocol's
>   profile, is RECOMMENDED.
>
>   Upon successful establishment of the security context (i.e.
>   GSS_Accept_sec_context returns GSS_S_COMPLETE) the server SHOULD
>   verify that the negotiated GSS-API mechanism is indeed the registered
>   one.  This is done by examining the value of the mech_type
>   parameter returned from the GSS_Accept_sec_context call.  If the
>   value differ SASL authentication MUST be aborted.
>
> (I've edited the text not to reference Kerberos)

Thanks, this seems good for GS2, but I wonder about the value of this
in GSSAPI.  It seems to make my RFC 2222 conforming implementation
violate the first SHOULD.  I'll bring this up separately...

In the last paragraph, I changed a SHOULD into MUST, since the last
MUST in that paragraph implies that the first SHOULD is followed.

> And the text quoted above is followed by additional text which is not
> currently in GS2:
>
>   Upon successful establishment of the security context and if the
>   server used GSS_C_NO_NAME/GSS_C_NO_CREDENTIAL to create acceptor
>   credential handle, the server SHOULD also check using the
>   GSS_Inquire_context that the target_name used by the client matches:
>
>   -  the GSS_C_NT_HOSTBASED_SERVICE "service@hostname" name syntax,
>      where "service" is the service name specified in the application
>      protocol's profile.
>
>   When GSS_Accept_sec_context returns GSS_S_COMPLETE, the server
>   examines the context to ensure that it provides a level of protection
>   permitted by the server's security policy.

Thanks, I added this, but changed the SHOULD into MUST, because
otherwise it seems we open up to a mitm.

I didn't include this part:

>  In particular, if the
>   integ_avail flag is not set in the context, then no security layer
>   can be offered or accepted.  If the conf_avail flag is not set in the
>   context, then no security layer with confidentiality can be offered
>   or accepted.

Because I think it interacts badly with the no-integrity cases that
GS2 has to support.  This should likely be revisited later on.

>>4.3.  Wrap Token
>
> [...]
>>   The entity that sends its first Wrap token will have to specify a
>>   bitmap of supported and preferred quality of protection schemes.  The
>>   entity that reply to the Wrap tokens will pick a scheme, based on the
>
> typo: replies

Thanks.

>>   bitmask and local policy.
>
>>   Four input formats to the GSS_Wrap function are defined.
>
> This is confusing. Maybe "two pairs of input formats"?

I changed it to:

    <t>Two pairs of input formats to the GSS_Wrap function are
      defined.  The first pair is used when the client sends the
      GSS_Wrap token first and the server responds.  The other pair is
      used when the server sends the GSS_Wrap token first and the
      client responds.</t>

>>  The first
>>   two input formats are used when the client sends the GSS_Wrap token
>>   first and the server reponds.  The other two input formats are used
>
> typo: responds

Thanks.

>>   when the server sends the GSS_Wrap token first and the client
>>   responds.
>
>>4.3.1.  GSS_Wrap input for client requests
>
> Here and in other 4.3.* sections: using SASL terms, the client sends
> "responses", while the server sends "challenges".
>
> [...]

I tried to use different terms here.  It is somewhat problematic,
because we are talking about requests and responses on a different
level than in SASL terms.

>>   The "client_maxbuf" field indicate the maximum protected buffer size
>
> ... in network byte order
>
>>   the client can receive.


Good catch, added.

>>4.3.2.  GSS_Wrap input for server responses
>
> [...]
>>   The "server_qop" field integer indicate the selected quality of
>>   protection.
>
> I suggest to add:
>
>  It must be one of the values specified by the client in
> "client_qops"/"client_cbqops"
>
> And similar text for "client_qop" in 4.3.4.

Actually, no.  I think it would be useful to be able to specify a
different value, if you can't support any of the proposed values.  It
is then up to the other side to reject the selection or not.

The messages is integrity protected, so there is no downgrade attack,
that I can see.

>>5.  Channel Bindings
>>
>>   The GS2 mechanism provide its own channel binding mechanism, instead
>>   of using the "chan_binding" parameter in the GSS-API context
>>   functions.  The reason for this is that the GS2 mechanism provide an
>>   option to proceed even if the channel bindings does not match.  The
>>   GSS-API framework specifies that authentication cannot proceed if
>>   channel bindings does not match.
>
> typo: do not

Thanks.

>>   Client and servers MUST set the "chan_binding" parameter in the calls
>>   to GSS_Init_sec_context to GSS_Accept_sec_context, respectively, to
>
> missing word "and"?

Yup, I replaced the second "to" to "and".

>>   NULL.
>
>>6.  Protocol Overview
>
> [...]
>
>>   Because GSS-API authentication is initiated by the client, the length
>>   field will be 0 in the initial token from the server to the client
>>   when the protocol profile do not support additional information to be
>
> typo: does not

Thanks.

>>   sent together with the authentication request.
>
>
> You might want to add the following to section 9:
>
>   When decoding any received data with GSS_Unwrap the major_status
>   other than the GSS_S_COMPLETE MUST be treated as a fatal error.

Added.

>>9.1.  Examples
>
> [...]
>
>>   When integrity is negotiated the octet will encode an integer 4 as
>
> I think you meant "confidentiality" here.
>
>>   follows.
>>
>>       0 1 2 3 4 5 6 7
>>      +-+-+-+-+-+-+-+-+
>>      |0|0|0|0|0|0|1|1|
>>      +-+-+-+-+-+-+-+-+
>
> And the example doesn't match the description above, it shows the
> value 3 (no layer + integrity)

Right, the example was wrong, I've changed it.

>>12.  IANA Considerations
>>
>>   The IANA is advised that SASL mechanism names starting with "GS2-"
>>   are reserved for SASL mechanisms which conform to this document.  The
>>   IANA is directed to place a statement to that effect in the sasl-
>>   mechanisms registry.
>>
>>     Subject: Registration of SASL mechanism GS2-*
>>     Family of SASL mechanisms: YES
>
> I think Kurt took out this line from the IANA registration template in
> RFC 4422.

Removed, thanks.

/Simon