Re: [Curdle] Spencer Dawkins' Yes on draft-ietf-curdle-ssh-ext-info-12: (with COMMENT)

[denis bider <[e-mail]>]

Hello,

replies below:


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

This language clarifies a corner case that's not expected to happen.
Algorithm negotiation in SSH works like this:

For each algorithm type (e.g. key exchange) and direction (for some
algorithm types, e.g. encryption):

- Server sends a comma-separated namelist of algorithm names in the initial
KEX_INFO message. For example: "foo,bar,baz"
- Client sends a similar list. For example: "baz,bar"
- The negotiated algorithm is (1) the first algorithm in the client's list
that (2) also appears in the server's list.

In the above example, the negotiated algorithm is "baz".

The "ext-info-s" and "ext-info-c" indicators are chosen so that two correct
implementations will never negotiate either, because only clients send
"ext-info-c", and only servers send "ext-info-s". If one of these
algorithms is negotiated, it means either one of the sides is seriously
buggy, or there's an attempt at some kind of attack. Therefore,
implementations should disconnect.

A party that doesn't support EXT_INFO will never see "ext-info-c" or
"ext-info-s" being negotiated because it did not send it in its algorithm
list. Therefore, it cannot be negotiated.

This is unless the party that doesn't support EXT_INFO is sending randomly
generated algorithm names and happens to hit upon that one. I trust no
honest implementation does this, except for fuzzing. If something like
fuzzing results in "ext-info-s" or "ext-info-c" being accidentally
negotiated, the aware implementation - of course - is the one that
disconnects.


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

The implication here is that the client might have existing messages in
flight to the server when the server sends SSH_MSG_USERAUTH_SUCCESS. This
is not often the case, but it is possible: for example, user authentication
takes some time, and the client sends a keep-alive message.

Because of this potential race condition, the server must wait until it
receives SSH_MSG_USERAUTH_SUCCESS before it assumes that compression in the
client-to-server direction is in effect.

The language says "shortly" because it's hard to say what delay is
permissible here. Consider an intentionally pessimized case:

- Suppose the server took extremely long to process the authentication
attempt. It's not unheard of that login processing can take 5 minutes.
- Suppose the client is set up to send frequent keep-alive messages to
prevent routers from disconnecting the TCP session, but it does not require
the server to respond to them.
- Suppose the server blocks and does nothing while it's processing the
login attempt.

In these circumstances, the server may send SSH_MSG_USERAUTH_SUCCESS, only
to find in its TCP input buffer a large number of keep-alive messages from
the client which were sent while the server was processing. The server
needs to be able to handle all of these messages without compression,
before it can expect to receive the client's SSH_MSG_NEWCOMPRESS.

My intent here was to encourage clients to send SSH_MSG_NEWCOMPRESS as soon
as they are able, while at the same time encouraging servers to tolerate
any reasonable number of packets from the client before this message is
received - where "reasonable" may be, to some extent,
implementation-defined.

Based on this feedback, I'm leaning toward changing this language as
follows:

"If this extension takes effect, the client MUST send the following message
within a reasonable number of outgoing messages after receiving
SSH_MSG_USERAUTH_SUCCESS - but not necessarily as the first such outgoing
message:"

Maybe that makes it more clear. The existing, immediately following
paragraph attempts to explain the context without trying to dwell on it too
long:

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

No problem. Will move.


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

Yes, that is the case. If clients can't be expected to implement the
"elevation" extension, Windows servers must elevate administrative users by
default.

In a parable - without "elevation", all sessions from administrative users
have to run as "root" (on Windows), because there's no way to "sudo".


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

I will do so. :-)

denis



On Tue, Sep 12, 2017 at 12:37 PM, Spencer Dawkins <
[e-mail]> wrote:

> Spencer Dawkins has entered the following ballot position for
> draft-ietf-curdle-ssh-ext-info-12: Yes
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-curdle-ssh-ext-info/
>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> 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.
>
>
> _______________________________________________
> Curdle mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/curdle
>