[Fix #10309] Refactor SSH version string parsing. #1697
Conversation
adiroiban
force-pushed
the
10309-conch-ssh-version-exchange
branch
from
bdcce3a to
430a0af
Compare
|
This is part-2 of the SSH version string security issue. In practice, most SSH implementations are sending just their SSH version string and then wait for more data. But, an SSH implementation can also send its SSH version string and the list of supported algorithm all in the same package. Most of the time, the list of algorithms host keys + kex + cipher + hmac + compressions is less than 2K . But, if a peer supports many ciphers ... or aliases , it can send more than 4K in the first package without breaking the SSH protocol. So... maybe we should not release 22.2.0 as it is now and have this merged and do a new release candidate. |
| # SSH-protoversion-softwareversion SP comments CR LF | ||
| self.sendDisconnect( | ||
| DISCONNECT_CONNECTION_LOST, b"Invalid peer version format." | ||
| ) |
There was a problem hiding this comment.
This seems to violate a "MUST" in RFC 4253 section 4.2, in the case where the peer is a server:
The server MAY send other lines of data before sending the version
string. Each line SHOULD be terminated by a Carriage Return and Line
Feed. Such lines MUST NOT begin with "SSH-", and SHOULD be encoded
in ISO-10646 UTF-8 [RFC3629] (language is not specified). Clients
MUST be able to process such lines. Such lines MAY be silently
ignored, or MAY be displayed to the client user. If they are
displayed, control character filtering, as discussed in [SSH-ARCH],
SHOULD be used. The primary use of this feature is to allow TCP-
wrappers to display an error message before disconnecting.
It would be OK to accept lines before the identification string only in the client case, though; and it may be permissible to limit the number of such lines even in the client case. For example, OpenSSH does this, where SSH_MAX_PRE_BANNER_LINES is defined to 1024:
/* Accept lines before banner only on client */
if (ssh->kex->server || ++n > SSH_MAX_PRE_BANNER_LINES) {
bad:
if ((r = sshbuf_put(ssh_packet_get_output(ssh),
mismatch, strlen(mismatch))) != 0)
return r;
return SSH_ERR_NO_PROTOCOL_VERSION;
}Clients don't have the same DoS concerns that servers have.
There was a problem hiding this comment.
Many thanks. I missed that part of the RFC.
I saw that there was an explicit test for this behaviour but I didn't knew why.
I will revert and document this. Thanks!
There was a problem hiding this comment.
It would be OK to accept lines before the identification string only in the client case, though; and it may be permissible to limit the number of such lines even in the client case. For example, OpenSSH does this, where SSH_MAX_PRE_BANNER_LINES is defined to 1024:
previously SSHTransportBase buffered the data then dropped the lines, when restoring this behaviour it would be better if any data that doesn't start b"SSH-" is dropped immediately
| self.sendDisconnect( | ||
| DISCONNECT_CONNECTION_LOST, | ||
| b"Peer version string longer than 4KB. " | ||
| b"Preventing a denial of service attack.", | ||
| b"Invalid peer version. Long value. Preventing a denial of service attack.", |
There was a problem hiding this comment.
| b"Invalid peer version. Long value. Preventing a denial of service attack.", | |
| b"Peer identification string not found in first 255 characters.", |
All other things being equal I prefer using the "identification string" language from the RFC. I don't object to the DoS attack language, but it seems unnecessary - I'm sure there are various errors that serve the purpose of preventing DoS attacks, and we probably shouldn't imply in the logs that only this one does so.
There was a problem hiding this comment.
Agree. I will use your suggestion. Agree that DoS is not needed. Thanks!
| """ | ||
| It will reject the connection when remote peer SSH format doesn't start | ||
| with `SSH-`. | ||
| """ |
There was a problem hiding this comment.
Can you make this test server-specific, and add a client-specific test that ensures that lines before the identification string are allowed?
| def test_dataReceiveVersionNotSentMemoryDOS(self): | ||
| """ | ||
| When the peer is not sending its SSH version but keeps sending data, | ||
| the connection is disconnected after 4KB to prevent buffering too | ||
| the connection is disconnected after 255B to prevent buffering too | ||
| much and running our of memory. |
There was a problem hiding this comment.
Could you fix this typo for "running out of memory" while you're here?
|
|
||
| def test_dataBeforeVersion(self): | ||
| """ | ||
| Test that the transport ignores data sent before the version string. | ||
| Any data sent before the version string, is considered a protocol |
There was a problem hiding this comment.
| Any data sent before the version string, is considered a protocol | |
| Any data sent before the version string is considered a protocol |
|
testing the automatic PR labeling needs-changes |
Scope and purpose
This is a refactoring of the SSH peer version string handling.
In 98387b3 we did a quick and dirty fix.
The goal of this PR is to look at providing a better fix.
The main issue with the previous fix, is that in theory a SSH peer can send with the first package the followings :
Now... the SSH version string is limited to 253 byte... but the supported ciphers can be provided in a packet that is more than 4K in length.
Especially if the peer support a lot of cipher and cipher aliases.
I don't know why conch SSH was implemented to ignore the lines received before 'SSH-`.
I don't thinks that this is OK.
The test was implemented in 775318c
Contributor Checklist:
tox -e lintto format my patch to meet the Twisted Coding Standard#character).reviewto the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.
reviewers: @twisted/twisted-contributors