Extension Negotiation in Secure Shell (SSH)
draft-ietf-curdle-ssh-ext-info-15

Note: This ballot was opened for revision 12 and is now closed.

Ben Campbell (was Discuss) Yes

Comment (2017-09-13 for -12)
Thanks for addressing my DISCUSS point. I've cleared, with the assumption the discussed change will make it into the final document.

(I leave the non-blocking comments below for reference, although most of them have been addressed in the email discussion.)

Substantive:

-2.3: "This message is sent immediately after SSH_MSG_NEWKEYS, without delay."
That seems to be contradicted in section 2.4, which talks about two other potential times.

-2.4, last paragraph (asterisk note): "The message MUST be sent at this point for the following reasons:"
That's a confusing use of MUST. Do you mean that the message MUST NOT be sent unless the reasons are true? Or that if the reasons are true, the message MUST be sent? Or is this just a statement of fact, in which case the 2119 keyword is not appropriate?

-2.5, 2nd paragraph: "or it MAY be sufficient that only one party includes it"
That seems like a statement of fact rather than a grant of permission. If so, the 2119 MAY is not appropriate.

Editorial:

-2.1, first paragraph: "Applications implementing this mechanism MUST add to the field
  "kex_algorithms", in their KEXINIT packet sent for the first key
  exchange, one of the following indicator names:"

That's hard to parse.  Suggestion: 
"Applications implementing this mechanism MUST add one of the 
 following indicator names to the "kex_algorithms" field for the first 
 key exchange:"

-2.5, 2nd to last paragraph: "... applications MUST
  tolerate any sequence of bytes; including null bytes at any position;
  in an unknown extension’s extension-value."

Redundant to similar normative statement in 2.3.

Spencer Dawkins Yes

Comment (2017-09-12 for -12)
Thanks for a readable document. It was a pleasure to review.

I have a few comments, but "Spencer doesn't know much about SSH" might be a fine response for most of them ;-)

In this text,

  Implementations MUST NOT send an incorrect indicator name for their
  role. Implementations MAY disconnect if the counter-party sends an
  incorrect indicator. If "ext-info-c" or "ext-info-s" ends up being
  negotiated as a key exchange method, the parties MUST disconnect.
  
why would a party that doen't support this extention disconnect?

I ask, because this looks like a MUST requirement that might apply to an implementation that doesn't support this extension. If that's normal SSH behavior, that's all I need to know :-)

In this text,

  If this extension takes effect, the renegotiated compression algorithm
  is activated for the very next SSH message after the trigger message:

  - Sent by the server, the trigger message is SSH_MSG_USERAUTH_SUCCESS.
  - Sent by the client, the trigger message is SSH_MSG_NEWCOMPRESS.

  If this extension takes effect, the client MUST send the following
  message shortly after receiving SSH_MSG_USERAUTH_SUCCESS:

    byte       SSH_MSG_NEWCOMPRESS (value 8)

  The purpose of NEWCOMPRESS is to avoid a race condition where the
  server cannot reliably know whether a message sent by the client was
  sent before or after receiving the server's USERAUTH_SUCCESS.
  
I THINK the point is that the client's SSH_MSG_NEWCOMPRESS is sent after SSH_MSG_USERAUTH_SUCCESS, before the client sends its first SSH message that's compressed using the newly negotiated compression algorithm, but "MUST send ... shortly after receiving SSH_MSG_USERAUTH_SUCCESS" doesn't help me figure that out - I'm mostly guessing, based on "the renegotiated compression algorithm is activated for the very next SSH message after the trigger message". But (1) if I got that wrong, it's at least unclear for TSV ADs, and (2) if I got that right, I'm not understanding what "shortly after receiving" the trigger message is saying - for example, I wondered if there's a timer involved, so that if a client doesn't have messages to send, it should still send SSH_MSG_NEWCOMPRESS, or the server will think something's wrong.

In this text,

  This extension MAY be sent by the client as follows:

    string      "elevation"
    string      choice of: "y" | "n" | "d"

  A client sends "y" to indicate its preference that the session should
  be elevated; "n" to not be elevated; and "d" for the server to use its
  default behavior. The server MAY disconnect if it receives a different
  extension value. If a client does not send the "elevation" extension,
  the server SHOULD act as if "d" was sent.

  The terms "elevation" and "elevated" refer to an operating system
  mechanism where an administrator user's logon session is associated
  with two security contexts: one limited, and one with administrative
  rights. To "elevate" such a session is to activate the security
  context with full administrative rights. For more information about
  this mechanism on Windows, see also [WINADMIN] and [WINTOKEN].
  
  If a client has included this extension, then after authentication, a
  server that supports this extension SHOULD indicate to the client
  whether elevation was done by sending the following global request:

    byte        SSH_MSG_GLOBAL_REQUEST
    string      "elevation"
    boolean     want reply = false
    boolean     elevation performed  
  
I would find this easier to understand, if the paragraph defining the terms was the first paragraph in the section, and then the description of the extension (which uses the term "elevation") followed. It's not bad, the way it is, but the reader has to read past the description to find out what the definition means.

The security considerations are brief and refer to overarching security considerations, which I understand because this is an extension, but 

  Security considerations are discussed throughout this document. This
  document updates the SSH protocol as defined in [RFC4251] and related
  documents. The security considerations of [RFC4251] apply.
  
doesn't say anything about new security considerations to think about, when you add support for elevation, which seems like it would be a new attack surface, but I could imagine that adding elevation is the first step toward fewer SSH server implementations that always run with administrative rights just in case they ever need to use them, so the attack surface is getting smaller? 

... And now I see that Mirja also asked about elevation, and your answer to her was pretty much what I had guessed.  Maybe it's worth summarizing your answer in section 3.4.

Alexey Melnikov (was Discuss) Yes

Comment (2017-09-14 for -13)
Thank you for addressing my DISCUSS and comments.

Kathleen Moriarty Yes

Comment (2017-09-13 for -12)
I agree with the SecDir reviewer that it would be helpful to state that the extension negotiation between the client and server is performed after key exchange with confidentiality in the security considerations section.

Eric Rescorla Yes

Alia Atlas No Objection

Comment (2017-09-12 for -12)
The IESG write-up is for the wrong draft - but it was easy enough to read the detailed shepherd's report.
Thanks for a clear and well-written draft.

Deborah Brungard No Objection

Benoit Claise No Objection

Suresh Krishnan No Objection

Warren Kumari No Objection

Comment (2017-09-11 for -12)
I have only a very minor nit:

Section 2.3.  SSH_MSG_EXT_INFO Message 
"Receivers MUST tolerate any sequence of bytes; including null bytes
  at any position; in an unknown extension’s extension-value."
The semi-colons confused me (I'm easily confused:-)), and I think that this would read much better as a parenthetical.

Hey, I did say it was a nit :-)
W

Mirja Kühlewind No Objection

Comment (2017-09-04 for -12)
1) What happens if the server received more than one (different) EXT_INFO messages or the client receives more than two?

2) One question regarding flow control: I understood that some implementation simply set the max value for the initial window, however, if you don't even have a max window how do you ensure that the receiver is not over loaded and what do you do if you receive more data that you can handle?

3) I'm by far not an expert but I would have expected that there are additional security considerations for elevation and maybe even flow control... no?

4) Thanks for quickly addressing the genart review!

Terry Manderson No Objection

Alvaro Retana No Objection

Adam Roach (was Discuss) No Objection

Comment (2017-09-12 for -13)
Section 2.4:

  (*) The message MUST be sent at this point for the following reasons:

It's not clear how this normative requirement interacts with this MAY:

  The second opportunity is just
  before (*) SSH_MSG_USERAUTH_SUCCESS, as defined in [RFC4252]. The
  server MAY send EXT_INFO at the second opportunity, whether or not it
  sent it at the first.

One seems to merely allow it, while the other seems to demand it. Could you add some text that clarifies this apparent contradiction?