Subject: Re: [Curdle] review of draft-ietf-curdle-gss-keyex-sha2-00

From: Simo Sorce <[email-masked]>
Date: Mon, 05 Jun 2017 05:39:37 -0400

On Sun, 2017-05-21 at 15:25 -0500, Benjamin Kaduk wrote:
> Generally, this document is in good shape, and I think we should move
> it forward.
> 
> My main comment/objection is that the security considerations should
> note that the combination of GSS key exchange, GSS delegated
> credentials, and use of insecure DNS to modify the requested server
> principal name result in a situation where an attacker can easily
> obtain a valid TGT+key for the client principal.  Obviously, RFCs
> 4462, 4120, etc. all implore us to not use insecure DNS in such a
> fashion, but nonetheless major Kerberos libraries continue to do so.
> At MIT we have disabled GSS key exchange for ssh because we default
> to GSS credential delegation.

I am quite hesitant to call additional security considerations in a
document that is substantially just updating an existing document
without introducing any new security related method that needs
additional considerations. However I added a reference to the Security
Considerations of 4120 as well. I do not think we should really do
more, this issue is (or should be) a well know problem and in no way
specific to SSH key exchanges.

> Some other nit-level comments:

meta-nit, your page numbers seem to be off a bit, I hope you're reading
the document I posted in curdle and not a previous version.

> On page 8, in step 5 of the procedure, do we want to give a
> reference or two for the shared-secret computation above the
> existing prose?

What do you have in mind? I think we already have all the relevant
references at the top or in the 1st step.

>   (Also, is the d_U and q_V terminology standard from
> some related document?  I did not see them in RFC 4462.)

No, they are new handles used to avoid repeating the client or service
specific value.
IE, d_U stands for 'd_C for the client or d_S for the server' and
q_V stands for 'q_S for the client or q_C for the server'.
Do you think this is confusing ?

> Also on page 8, step 6 could further clarify that this only occurs
> when the server's final call to GSS_Accept_sec_context() returns
> GSS_S_COMPLETE; anything else is an error condition.

Already explained in step 8, I think that is sufficient.

> Relatedly, the
> way the GSS context negotiation loop's exit conditions are split
> between step 6 and step 2 confused me a little on first read, but it
> seems correct, and along with the RFC 7546 reference I think readers
> will be okay.

It is really hard to condense everything in few steps and maintain
perfect clarity on all fronts, but I think the current text strikes the
right balance, between being more informative than the document we
replace, without being overly verbose and distracting from the actual
core of the matter and has enough references to clarify with original
documents any doubts one may have.

> On page 11, the description of the HASH input has a couple of
> inconsistencies -- in RFC 4462, V_S is the server's "version"
> string, though here we have it as the server's "identification"
> string.  I don't think that's a problem per se, but wanted to note
> it just in case.  Also, V_S excludes CR and LF, but V_C excludes CR
> and NL; we should probably be consistent about NL vs. LF.

Thanks for pointing these out, I am fixing these to say server version
for consistency with 4462 and will use NL again for consistency with
4462 which is itself inconsistent as it uses CRLF in some places ...

> In sections 5.2.2 and later we continue to include refeferences for
> MD5, DER, and Base64; RFC 4462 only included those references in the
> first corresponding subsection.  At this point, it's probably not
> worth making any changes, though.

I agree it is not worth making changes here.

> And one editorial note:
> 
> On page 6, "non- zero" should not have a space.

Thanks.
I will submit -01 with these corrections shortly.

Simo.

---

From: Simo Sorce <[email-masked]>
Date: Mon, 05 Jun 2017 05:56:43 -0400

> specific to SSH key exchanges.

I posted a new draft with the corrections noted below but with the
exception of adding 4120 as I stated here.
 
Although krb5 is the most (or only ?) mechanism RFC 4462 and this draft
are definitely not mechanism specific (they only recommend against
using SPNEGO not any other mechanism), and it doesn't feel right to me
to add security considerations to a specific mechanism (delegation is a
krb5 mechanism feature in this case, not a feature of the SSH Key
exchange) in an amendment document that is not changing anything with
regard to the initial RFC 4462 security properties.

If you do not agree, please provide language that you think would be
appropriate.

If anyone else has comments on whether we should add or not additional,
mechanism specific, security considerations, please let me know.

Simo.

> _______________________________________________
> Curdle mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/curdle

---

From: Daniel Migault <[email-masked]>
Date: Sun, 11 Jun 2017 13:44:47 -0400

Hi,

Does  the current version reached consensus ? If anyone  thinks otherwise
please raise your concern as soon as possible. We would like to have this
draft in WGLC next week.

Yours,
Daniel

---

From: Benjamin Kaduk <[email-masked]>
Date: Sun, 11 Jun 2017 20:20:19 -0500

Sorry for the slow reply; I'll try to catch both of your messages
inline.

> > specific to SSH key exchanges.

Certainly in the security directorate review process we are not
afraid to call out when the existing security considerations of a
document being updated/reissued are inadequate, or the security
properties of the environment in which it operates has changed since
the initial writing -- it feels like if we leave old-and-incomplete
security considerations in place, it is just because we are lazy.

> exception of adding 4120 as I stated here.

Adding 4120 helps, but of course the admonition it contains to not
use insecure DNS for principal resolution is mostly honored in the
breach, so it only helps so much.

> If you do not agree, please provide language that you think would be

I disagree; this draft does mention setting deleg_req_flag
to request delegation, so concerns relating to security
considerations for delegation seem in scope to me.  You probably
don't even have to explicitly refer to krb5 (though maybe it is
helpful to the reader to do so); something like:

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

When delegating credentials, extra care must be taken to ensure that
the acceptor being authenticated matches the desired ssh target.
This is particularly problematic with Kerberos, as many
implementations defy the prohibition against the use of insecure DNS
to canonicalize service principal names; in these cases, spoofing a
DNS response that points to an attacker-controlled machine with
valid Kerberos credentials results in the user silently
authenticating to and delegating credentials to the attacker, who
can now use the user's TGT at will.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

Hmm, maybe that's still too strong on the Kerberos focus, though.

> > the document I posted in curdle and not a previous version.

I reviewed precisely the document mentioned in the email subject.
Note that the page numbers appear at the bottom of the numbered
page, so it can be confusing which page one is viewing when using a
continuous-scroll viewing mode.

> > references at the top or in the 1st step.

This was basically just the contrary-reader part of my mind thinking
as I read this "compute the shared secret, but *how*?".  So, just
referencing RFC 5656 and draft-ietf-curdle-ssh-curves again would be
enough, or even just saying "shared secret obtained using ECDH key
exchange as defined in the specification for the curve in use".
(Or, really, it might be fine leaving it as-is; only
contrary-reader-me had an issue with it, and it was not confusing to
actual-me.)  It seems that the main content of what this entails is
in the following text, so I am not strongly advocating for a change
here.

> > Do you think this is confusing ?

Not really; I was just wondering if we should expect readers to
already be familiar with it, in which case a nod to the appropriate
reference might be in order.

> > Already explained in step 8, I think that is sufficient.

It probably is, yes.

> > documents any doubts one may have.

Definitely.  I think you did a fine job, and don't see a pressing
need for any changes.

> > 4462 which is itself inconsistent as it uses CRLF in some places ...

Oh, I missed the internal inconsistencies in 4462.  Thanks.

> > I will submit -01 with these corrections shortly.

Thanks.  The diff from the -00 looks good modulo our disagreement
about the delegation security considerations [for Kerberos
implementations that are non-conformant to RFC 4120].

-Ben

---

From: Simo Sorce <[email-masked]>
Date: Mon, 12 Jun 2017 06:18:01 -0400

> Sorry for the slow reply; I'll try to catch both of your messages

Also replying inline and trimming.

> security considerations in place, it is just because we are lazy.

Noted.

> > If you do not agree, please provide language that you think would
> > be appropriate.
> 

I think one of the issues I have with your text and previous comments
is the implied fact that credentials will be delegated. Looking at
openssh's defaults (on Fedora/RHEL at least) delegation is disabled by
default.

Maybe the following slightly modified text:

#######################################################################
GSSAPI mechanisms can optionally delegate credentials to the target
host. In this case extra care must be taken to ensure that the acceptor
being authenticated matches the target the user intended. Some
mechanisms implementations (like commonly used Krb5 libraries) may use
insecure DNS resolution to canonicalize the target name; in these
cases, spoofing a DNS response that points to an attacker-controlled
machine may results in the user silently delegating credentials to the
attacker, who can now impersonate the user at will.
#######################################################################

> in the following text, so I am not strongly advocating for a change

In this case I'll wait for more input on this. I felt that adding
references here was not a big gain and made the text flow less
smoothly.

> implementations that are non-conformant to RFC 4120].

If we can agree on the text above (feel free to add corrections) then I
can add it to the Security Consideration and cut a hopefully final -02

Simo.

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 13 Jun 2017 17:35:31 -0500

> Maybe the following slightly modified text:

Maybe also mention "by setting the deleg_req_flag" to refer back to
step 2 where this is discussed.

> attacker, who can now impersonate the user at will.

But this looks fine to me, thanks.

> references here was not a big gain and made the text flow less

Sure, I am happy to defer to your judgement.

> can add it to the Security Consideration and cut a hopefully final -02

I'm looking forward to having this document move forward.

Thanks again,

Ben

---

From: Daniel Migault <[email-masked]>
Date: Tue, 13 Jun 2017 20:18:10 -0400

Thanks Ben and Simo for the follow-up.

Yours,
Daniel