[crypto] Correctly ignore pre-banner lines
Damien Miller (Gerrit)
Damien Miller has uploaded the change for review![]( "Open in Gerrit")
Commit message
Correctly ignore pre-banner lines
Reading the peer's benner in `readVersion()` was incorrectly counting lines sent by the server before the actual `SSH-2.0` banner against the line limit for the banner itself.
This is incorrect per [RFC 4253 section 4.2](https://datatracker.ietf.org/doc/html/rfc4253#section-4.2), where the limit of 255 characters is written to apply only to the `SSH-` banner string itself.
Change diff
diff --git a/ssh/transport.go b/ssh/transport.go
index fa3dd6a..1dc6fe1 100644
--- a/ssh/transport.go
+++ b/ssh/transport.go
@@ -350,6 +350,7 @@
// except the one containing the SSH version (provided that
// all the lines do not exceed 255 bytes in total).
versionString = versionString[:0]
+ length = 0
continue
}
ok = true
Change information
- M ssh/transport.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller added 1 comment![]( "Open in Gerrit")
FWIW OpenSSH sets a maximum of 1024 pre-banner lines so there is a cap on how much junk a peer can send before it gets to the important bit
Related details
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda added 3 comments![]( "Open in Gerrit")
FWIW OpenSSH sets a maximum of 1024 pre-banner lines so there is a cap on how much junk a peer can send before it gets to the important bit
Yeah I think we need to add something like that before we make the 255 bytes cap not apply to the rest.
Is there a limit on how long each of those 1024 lines are allowed to be?
Correctly ignore pre-banner lines```suggestion
ssh: correctly ignore pre-banner lines
```
// except the one containing the SSH version (provided that
// all the lines do not exceed 255 bytes in total).- Damien Miller
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Miller
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Miller
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller added 3 comments![]( "Open in Gerrit")
Filippo ValsordaFWIW OpenSSH sets a maximum of 1024 pre-banner lines so there is a cap on how much junk a peer can send before it gets to the important bit
Yeah I think we need to add something like that before we make the 255 bytes cap not apply to the rest.
Is there a limit on how long each of those 1024 lines are allowed to be?
OpenSSH applies the same length limit to the pre-version lines as to the version line. In our case we accept up to 8192 characters, but I think it's reasonable to accept 255 characters as a limit for them too.
I've revised the patch with a 1024 line limit.
```suggestion
ssh: correctly ignore pre-banner lines
```
Done
// except the one containing the SSH version (provided that
// all the lines do not exceed 255 bytes in total).Sounds like this comment needs fixing?
- Filippo Valsorda
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda voted and added 2 comments![]( "Open in Gerrit")
Votes added by Filippo Valsorda
| Code-Review | +2 |
| Commit-Queue | +1 |
Filippo ValsordaFWIW OpenSSH sets a maximum of 1024 pre-banner lines so there is a cap on how much junk a peer can send before it gets to the important bit
Damien MillerYeah I think we need to add something like that before we make the 255 bytes cap not apply to the rest.
Is there a limit on how long each of those 1024 lines are allowed to be?
OpenSSH applies the same length limit to the pre-version lines as to the version line. In our case we accept up to 8192 characters, but I think it's reasonable to accept 255 characters as a limit for them too.
I've revised the patch with a 1024 line limit.
Sweet, thank you.
```suggestion
ssh: correctly ignore pre-banner lines
```
- Damien Miller
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Filippo Valsorda
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Miller
- Filippo Valsorda
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller added 1 comment![]( "Open in Gerrit")
This should fix the tests, and cover the new cases relating to pre-banner lines
Related details
- Filippo Valsorda
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicola Murino (Gerrit)
Nicola Murino added 2 comments![]( "Open in Gerrit")
Thank you. I also see in OpenSSH that the lines before the version string are accepted from a server only which matches RFC 4253. Here readVersion is shared by both side, I'll send a separate CL for this.
Related details
- Damien Miller
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicola Murino (Gerrit)
Nicola Murino voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
Related details
- Damien Miller
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicola Murino (Gerrit)
Nicola Murino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Damien Miller (Gerrit)
Damien Miller uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Nicola Murino
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Miller
- Filippo Valsorda
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Miller (Gerrit)
Damien Miller added 1 comment![]( "Open in Gerrit")
Should this be `length = -1`?
Probably simpler to get rid of the separate length and just check the length of the buffer directly.
Marked as resolved.
Related details
- Filippo Valsorda
- Nicola Murino
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |