From 7413539965622ba07e8cd3719fc9d545fb52c5fd Mon Sep 17 00:00:00 2001 From: mik2k2 <44849223+mik2k2@users.noreply.github.com> Date: Mon, 31 May 2021 16:08:30 +0200 Subject: [PATCH] main, sshd: Refactor authentication, add IP throttling, improve passphrase auth * Move password authentication handling into sshd/auth (fixes #394). Password authentication is now completely handeled in Auth. The normal keyboard-interactive handler checks if passwords are supported and asks for them, removing the need to override the callbacks. Brute force throttling is removed; I'd like to base it on IP address banning, which requires changes to the checks. I'm not sure, but I think timing attacks against the password are fixed: - The hashing of the real password happens only at startup. - The hashing of a provided password is something an attacker can do themselves; It doesn't leak anything about the real password. - The hash comparison is constant-time. * refactor checks, IP-ban incorrect passphrases, renames - s/assword/assphrase/, typo fixes - bans are checked separately from public keys - an incorrect passphrase results in a one-minute IP ban - whitelists no longer override bans (i.e. you can get banned if you're whitelisted) * (hopefully) final changes --- auth.go | 77 +++++++++++++++++++++++++++++++++------------ auth_test.go | 40 +++++++++++++++++++++-- cmd/ssh-chat/cmd.go | 44 +++----------------------- sshd/auth.go | 47 ++++++++++++++++++++++----- sshd/client_test.go | 13 +++++++- 5 files changed, 150 insertions(+), 71 deletions(-) diff --git a/auth.go b/auth.go index 8a15367..1de0087 100644 --- a/auth.go +++ b/auth.go @@ -1,6 +1,8 @@ package sshchat import ( + "crypto/sha256" + "crypto/subtle" "encoding/csv" "errors" "fmt" @@ -17,9 +19,12 @@ import ( // when whitelisting is enabled. var ErrNotWhitelisted = errors.New("not whitelisted") -// ErrBanned is the error returned when a key is checked that is banned. +// ErrBanned is the error returned when a client is banned. var ErrBanned = errors.New("banned") +// ErrIncorrectPassphrase is the error returned when a provided passphrase is incorrect. +var ErrIncorrectPassphrase = errors.New("incorrect passphrase") + // newAuthKey returns string from an ssh.PublicKey used to index the key in our lookup. func newAuthKey(key ssh.PublicKey) string { if key == nil { @@ -43,12 +48,14 @@ func newAuthAddr(addr net.Addr) string { } // Auth stores lookups for bans, whitelists, and ops. It implements the sshd.Auth interface. +// If the contained passphrase is not empty, it complements a whitelist. type Auth struct { - bannedAddr *set.Set - bannedClient *set.Set - banned *set.Set - whitelist *set.Set - ops *set.Set + passphraseHash []byte + bannedAddr *set.Set + bannedClient *set.Set + banned *set.Set + whitelist *set.Set + ops *set.Set } // NewAuth creates a new empty Auth. @@ -62,23 +69,30 @@ func NewAuth() *Auth { } } -// AllowAnonymous determines if anonymous users are permitted. -func (a *Auth) AllowAnonymous() bool { - return a.whitelist.Len() == 0 +// SetPassphrase enables passphrase authentication with the given passphrase. +// If an empty passphrase is given, disable passphrase authentication. +func (a *Auth) SetPassphrase(passphrase string) { + if passphrase == "" { + a.passphraseHash = nil + } else { + hashArray := sha256.Sum256([]byte(passphrase)) + a.passphraseHash = hashArray[:] + } } -// Check determines if a pubkey fingerprint is permitted. -func (a *Auth) Check(addr net.Addr, key ssh.PublicKey, clientVersion string) error { - authkey := newAuthKey(key) +// AllowAnonymous determines if anonymous users are permitted. +func (a *Auth) AllowAnonymous() bool { + return a.whitelist.Len() == 0 && a.passphraseHash == nil +} - if a.whitelist.Len() != 0 { - // Only check whitelist if there is something in it, otherwise it's disabled. - whitelisted := a.whitelist.In(authkey) - if !whitelisted { - return ErrNotWhitelisted - } - return nil - } +// AcceptPassphrase determines if passphrase authentication is accepted. +func (a *Auth) AcceptPassphrase() bool { + return a.passphraseHash != nil +} + +// CheckBans checks IP, key and client bans. +func (a *Auth) CheckBans(addr net.Addr, key ssh.PublicKey, clientVersion string) error { + authkey := newAuthKey(key) var banned bool if authkey != "" { @@ -98,6 +112,29 @@ func (a *Auth) Check(addr net.Addr, key ssh.PublicKey, clientVersion string) err return nil } +// CheckPubkey determines if a pubkey fingerprint is permitted. +func (a *Auth) CheckPublicKey(key ssh.PublicKey) error { + authkey := newAuthKey(key) + whitelisted := a.whitelist.In(authkey) + if a.AllowAnonymous() || whitelisted { + return nil + } else { + return ErrNotWhitelisted + } +} + +// CheckPassphrase determines if a passphrase is permitted. +func (a *Auth) CheckPassphrase(passphrase string) error { + if !a.AcceptPassphrase() { + return errors.New("passphrases not accepted") // this should never happen + } + passedPassphraseHash := sha256.Sum256([]byte(passphrase)) + if subtle.ConstantTimeCompare(passedPassphraseHash[:], a.passphraseHash) == 0 { + return ErrIncorrectPassphrase + } + return nil +} + // Op sets a public key as a known operator. func (a *Auth) Op(key ssh.PublicKey, d time.Duration) { if key == nil { diff --git a/auth_test.go b/auth_test.go index 485af4d..a561f92 100644 --- a/auth_test.go +++ b/auth_test.go @@ -28,7 +28,7 @@ func TestAuthWhitelist(t *testing.T) { } auth := NewAuth() - err = auth.Check(nil, key, "") + err = auth.CheckPublicKey(key) if err != nil { t.Error("Failed to permit in default state:", err) } @@ -44,7 +44,7 @@ func TestAuthWhitelist(t *testing.T) { t.Error("Clone key does not match.") } - err = auth.Check(nil, keyClone, "") + err = auth.CheckPublicKey(keyClone) if err != nil { t.Error("Failed to permit whitelisted:", err) } @@ -54,8 +54,42 @@ func TestAuthWhitelist(t *testing.T) { t.Fatal(err) } - err = auth.Check(nil, key2, "") + err = auth.CheckPublicKey(key2) if err == nil { t.Error("Failed to restrict not whitelisted:", err) } } + +func TestAuthPassphrases(t *testing.T) { + auth := NewAuth() + + if auth.AcceptPassphrase() { + t.Error("Doesn't known it won't accept passphrases.") + } + auth.SetPassphrase("") + if auth.AcceptPassphrase() { + t.Error("Doesn't known it won't accept passphrases.") + } + + err := auth.CheckPassphrase("Pa$$w0rd") + if err == nil { + t.Error("Failed to deny without passphrase:", err) + } + + auth.SetPassphrase("Pa$$w0rd") + + err = auth.CheckPassphrase("Pa$$w0rd") + if err != nil { + t.Error("Failed to allow vaild passphrase:", err) + } + + err = auth.CheckPassphrase("something else") + if err == nil { + t.Error("Failed to restrict wrong passphrase:", err) + } + + auth.SetPassphrase("") + if auth.AcceptPassphrase() { + t.Error("Didn't clear passphrase.") + } +} diff --git a/cmd/ssh-chat/cmd.go b/cmd/ssh-chat/cmd.go index d5dca7c..a13e7ff 100644 --- a/cmd/ssh-chat/cmd.go +++ b/cmd/ssh-chat/cmd.go @@ -2,7 +2,6 @@ package main import ( "bufio" - "errors" "fmt" "io/ioutil" "net/http" @@ -10,7 +9,6 @@ import ( "os/signal" "os/user" "strings" - "time" "github.com/alexcesaro/log" "github.com/alexcesaro/log/golog" @@ -123,44 +121,6 @@ func main() { config.ServerVersion = "SSH-2.0-Go ssh-chat" // FIXME: Should we be using config.NoClientAuth = true by default? - if options.Passphrase != "" { - if options.Whitelist != "" { - logger.Warning("Passphrase is disabled while whitelist is enabled.") - } - if config.KeyboardInteractiveCallback != nil { - fail(1, "Passphrase authentication conflicts with existing KeyboardInteractive setup.") // This should not happen - } - - // We use KeyboardInteractiveCallback instead of PasswordCallback to - // avoid preventing the client from including a pubkey in the user - // identification. - config.KeyboardInteractiveCallback = func(conn ssh.ConnMetadata, challenge ssh.KeyboardInteractiveChallenge) (*ssh.Permissions, error) { - answers, err := challenge("", "", []string{"Passphrase required to connect: "}, []bool{true}) - if err != nil { - return nil, err - } - if len(answers) == 1 && answers[0] == options.Passphrase { - // Success - return nil, nil - } - // It's not gonna do much but may as well throttle brute force attempts a little - time.Sleep(2 * time.Second) - - return nil, errors.New("incorrect passphrase") - } - - // We also need to override the PublicKeyCallback to prevent rando pubkeys from bypassing - cb := config.PublicKeyCallback - config.PublicKeyCallback = func(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) { - perms, err := cb(conn, key) - if err == nil { - err = errors.New("passphrase authentication required") - } - return perms, err - } - - } - s, err := sshd.ListenSSH(options.Bind, config) if err != nil { fail(4, "Failed to listen on socket: %v\n", err) @@ -174,6 +134,10 @@ func main() { host.SetTheme(message.Themes[0]) host.Version = Version + if options.Passphrase != "" { + auth.SetPassphrase(options.Passphrase) + } + err = fromFile(options.Admin, func(line []byte) error { key, _, _, _, err := ssh.ParseAuthorizedKey(line) if err != nil { diff --git a/sshd/auth.go b/sshd/auth.go index a140413..ebcf6c6 100644 --- a/sshd/auth.go +++ b/sshd/auth.go @@ -5,17 +5,26 @@ import ( "encoding/base64" "errors" "net" + "time" "github.com/shazow/ssh-chat/internal/sanitize" "golang.org/x/crypto/ssh" ) -// Auth is used to authenticate connections based on public keys. +// Auth is used to authenticate connections. type Auth interface { // Whether to allow connections without a public key. AllowAnonymous() bool - // Given address and public key and client agent string, returns nil if the connection should be allowed. - Check(net.Addr, ssh.PublicKey, string) error + // If passphrase authentication is accepted + AcceptPassphrase() bool + // Given address and public key and client agent string, returns nil if the connection is not banned. + CheckBans(net.Addr, ssh.PublicKey, string) error + // Given a public key, returns nil if the connection should be allowed. + CheckPublicKey(ssh.PublicKey) error + // Given a passphrase, returns nil if the connection should be allowed. + CheckPassphrase(string) error + // BanAddr bans an IP address for the specified amount of time. + BanAddr(net.Addr, time.Duration) } // MakeAuth makes an ssh.ServerConfig which performs authentication against an Auth implementation. @@ -25,7 +34,11 @@ func MakeAuth(auth Auth) *ssh.ServerConfig { NoClientAuth: false, // Auth-related things should be constant-time to avoid timing attacks. PublicKeyCallback: func(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) { - err := auth.Check(conn.RemoteAddr(), key, sanitize.Data(string(conn.ClientVersion()), 64)) + err := auth.CheckBans(conn.RemoteAddr(), key, sanitize.Data(string(conn.ClientVersion()), 64)) + if err != nil { + return nil, err + } + err = auth.CheckPublicKey(key) if err != nil { return nil, err } @@ -34,11 +47,31 @@ func MakeAuth(auth Auth) *ssh.ServerConfig { }} return perm, nil }, + + // We use KeyboardInteractiveCallback instead of PasswordCallback to + // avoid preventing the client from including a pubkey in the user + // identification. KeyboardInteractiveCallback: func(conn ssh.ConnMetadata, challenge ssh.KeyboardInteractiveChallenge) (*ssh.Permissions, error) { - if !auth.AllowAnonymous() { - return nil, errors.New("public key authentication required") + err := auth.CheckBans(conn.RemoteAddr(), nil, sanitize.Data(string(conn.ClientVersion()), 64)) + if err != nil { + return nil, err + } + if auth.AcceptPassphrase() { + var answers []string + answers, err = challenge("", "", []string{"Passphrase required to connect: "}, []bool{true}) + if err == nil { + if len(answers) != 1 { + err = errors.New("didn't get passphrase") + } else { + err = auth.CheckPassphrase(answers[0]) + if err != nil { + auth.BanAddr(conn.RemoteAddr(), time.Second*2) + } + } + } + } else if !auth.AllowAnonymous() { + err = errors.New("public key authentication required") } - err := auth.Check(conn.RemoteAddr(), nil, sanitize.Data(string(conn.ClientVersion()), 64)) return nil, err }, } diff --git a/sshd/client_test.go b/sshd/client_test.go index 1c0ead4..5ac4399 100644 --- a/sshd/client_test.go +++ b/sshd/client_test.go @@ -4,6 +4,7 @@ import ( "errors" "net" "testing" + "time" "golang.org/x/crypto/ssh" ) @@ -15,9 +16,19 @@ type RejectAuth struct{} func (a RejectAuth) AllowAnonymous() bool { return false } -func (a RejectAuth) Check(net.Addr, ssh.PublicKey, string) error { +func (a RejectAuth) AcceptPassphrase() bool { + return false +} +func (a RejectAuth) CheckBans(addr net.Addr, key ssh.PublicKey, clientVersion string) error { return errRejectAuth } +func (a RejectAuth) CheckPublicKey(ssh.PublicKey) error { + return errRejectAuth +} +func (a RejectAuth) CheckPassphrase(string) error { + return errRejectAuth +} +func (a RejectAuth) BanAddr(net.Addr, time.Duration) {} func TestClientReject(t *testing.T) { signer, err := NewRandomSigner(512)