Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

prevent running key exchange if hello-only flag set #228

Merged
merged 3 commits into from
Nov 2, 2019
Merged

prevent running key exchange if hello-only flag set #228

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
422cdc7
prevent running key exchange if hello-only flag set
thegwan Oct 21, 2019
86e24ff
Attempt to fix CI
dadrian Oct 21, 2019
3164523
reduce code duplication
thegwan Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
language: go
go:
- 1.9
- 1.12
services:
- docker
before_install:
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ github.com/zmap/rc2 v0.0.0-20131011165748-24b9757f5521/go.mod h1:3YZ9o3WnatTIZhu
github.com/zmap/zcertificate v0.0.0-20180516150559-0e3d58b1bac4/go.mod h1:5iU54tB79AMBcySS0R2XIyZBAVmeHranShAFELYx7is=
github.com/zmap/zcrypto v0.0.0-20190729165852-9051775e6a2e h1:mvOa4+/DXStR4ZXOks/UsjeFdn5O5JpLUtzqk9U8xXw=
github.com/zmap/zcrypto v0.0.0-20190729165852-9051775e6a2e/go.mod h1:w7kd3qXHh8FNaczNjslXqvFQiv5mMWRXlL9klTUAHc8=
github.com/zmap/zflags v1.3.0 h1:Pd79SH44p4j54+YADAFiB6dg94DI5GFUMdQkWR5cIL8=
github.com/zmap/zflags v1.3.0/go.mod h1:HXDUD+uue8yeLHr0eXx1lvY6CvMiHbTKw5nGmA9OUoo=
github.com/zmap/zflags v1.4.0-beta.1 h1:jzZ+wKTCksS/ltf9q19gYJ6zJuqRULuRdSWBPueEiZ8=
github.com/zmap/zflags v1.4.0-beta.1/go.mod h1:HXDUD+uue8yeLHr0eXx1lvY6CvMiHbTKw5nGmA9OUoo=
golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
Expand Down
44 changes: 25 additions & 19 deletions lib/ssh/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,29 +180,35 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) {
t.mu.Lock()

firstKex := t.sessionID == nil
if t.config.HelloOnly {
t.sentInitMsg = nil
t.sentInitPacket = nil
t.cond.Broadcast()
t.writtenSinceKex = 0
t.mu.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks correct, but my only concern is the duplication of the cleanup to unblock writers, and the mutex lock taken before a conditional and then unlocked in the if/else branches of the conditional.

Could the same thing be accomplished by inverting the HelloOnly check, like so:

	t.mu.Lock()

	firstKex := t.sessionID == nil
	if !t.config.HelloOnly {
		err = t.enterKeyExchangeLocked(p)
		if err != nil {
			// drop connection
			t.conn.Close()
			t.writeError = err
		}

		if debugHandshake {
			log.Printf("%s exited key exchange (first %v), err %v", t.id(), firstKex, err)
		}
	}
	// Unblock writers.
	t.sentInitMsg = nil
	t.sentInitPacket = nil
	t.cond.Broadcast()
	t.writtenSinceKex = 0
	t.mu.Unlock()

	if err != nil {
		return nil, err
	}

This does create an unnecessary err check at the end iif HelloOnly is true, but reduces code duplication and keeps the mutex lock/unlock balanced outside conditionals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, making the change

} else {
err = t.enterKeyExchangeLocked(p)
if err != nil {
// drop connection
t.conn.Close()
t.writeError = err
}

err = t.enterKeyExchangeLocked(p)
if err != nil {
// drop connection
t.conn.Close()
t.writeError = err
}
if debugHandshake {
log.Printf("%s exited key exchange (first %v), err %v", t.id(), firstKex, err)
}

if debugHandshake {
log.Printf("%s exited key exchange (first %v), err %v", t.id(), firstKex, err)
}
// Unblock writers.
t.sentInitMsg = nil
t.sentInitPacket = nil
t.cond.Broadcast()
t.writtenSinceKex = 0
t.mu.Unlock()

// Unblock writers.
t.sentInitMsg = nil
t.sentInitPacket = nil
t.cond.Broadcast()
t.writtenSinceKex = 0
t.mu.Unlock()

if err != nil {
return nil, err
if err != nil {
return nil, err
}
}

t.readSinceKex = 0

// By default, a key exchange is hidden from higher layers by
Expand Down