From bfd4e8c66bbbf1a210a0cd9a7ff158373a47102d Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Sun, 12 Jan 2025 11:31:01 -0500 Subject: [PATCH] Check passwords based on hashes rather than plaintext fusion's current password mechanism is vulnerable to a timing attack: https://en.wikipedia.org/wiki/Timing_attack Because fusion checks passwords using simple character-by-character string comparison, a password attempt that begins with the correct characters will take longer to evaluate than one that starts with incorrect characters. For example, if the correct password is 'platypus123' then a password attempt of 'plates' will take longer to evaluate than 'spoons' because 'plates' and 'platypus' share a common prefix. An attacker who attempts the password 'plates' will know that they likely have the correct prefix. To prevent the timing attack, this change hashes the user's password using PBKDF2 and compares hashes using subtle.ConstantTimeCompare, which is specifically designed to prevent timing attacks. --- api/api.go | 7 +++-- api/session.go | 10 ++++-- auth/password.go | 43 ++++++++++++++++++++++++++ auth/password_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++ cmd/server/server.go | 2 +- conf/conf.go | 40 +++++++++++++++++------- 6 files changed, 156 insertions(+), 17 deletions(-) create mode 100644 auth/password.go create mode 100644 auth/password_test.go diff --git a/api/api.go b/api/api.go index a3fa310..3f70463 100644 --- a/api/api.go +++ b/api/api.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/0x2e/fusion/auth" "github.com/0x2e/fusion/conf" "github.com/0x2e/fusion/frontend" "github.com/0x2e/fusion/pkg/logx" @@ -26,7 +27,7 @@ import ( type Params struct { Host string Port int - Password string + PasswordHash auth.HashedPassword UseSecureCookie bool TLSCert string TLSKey string @@ -70,7 +71,7 @@ func Run(params Params) { r.Use(middleware.TimeoutWithConfig(middleware.TimeoutConfig{ Timeout: 30 * time.Second, })) - r.Use(session.Middleware(sessions.NewCookieStore([]byte(params.Password)))) + r.Use(session.Middleware(sessions.NewCookieStore(params.PasswordHash.Bytes()))) r.Pre(middleware.RemoveTrailingSlash()) r.Use(func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { @@ -88,7 +89,7 @@ func Run(params Params) { })) loginAPI := Session{ - Password: params.Password, + PasswordHash: params.PasswordHash, UseSecureCookie: params.UseSecureCookie, } r.POST("/api/sessions", loginAPI.Create) diff --git a/api/session.go b/api/session.go index 4e66916..3047c70 100644 --- a/api/session.go +++ b/api/session.go @@ -3,12 +3,13 @@ package api import ( "net/http" + "github.com/0x2e/fusion/auth" "github.com/labstack/echo-contrib/session" "github.com/labstack/echo/v4" ) type Session struct { - Password string + PasswordHash auth.HashedPassword UseSecureCookie bool } @@ -25,7 +26,12 @@ func (s Session) Create(c echo.Context) error { return err } - if req.Password != s.Password { + attemptedPasswordHash, err := auth.HashPassword(req.Password) + if err != nil { + return echo.NewHTTPError(http.StatusUnauthorized, "Invalid password") + } + + if correctPasswordHash := s.PasswordHash; !attemptedPasswordHash.Equals(correctPasswordHash) { return echo.NewHTTPError(http.StatusUnauthorized, "Wrong password") } diff --git a/auth/password.go b/auth/password.go new file mode 100644 index 0000000..c201792 --- /dev/null +++ b/auth/password.go @@ -0,0 +1,43 @@ +package auth + +import ( + "crypto/sha256" + "crypto/subtle" + "errors" + + "golang.org/x/crypto/pbkdf2" +) + +var ErrPasswordTooShort = errors.New("password must be non-empty") + +type HashedPassword struct { + hash []byte +} + +func (hp HashedPassword) Bytes() []byte { + return hp.hash +} + +func (hp HashedPassword) Equals(other HashedPassword) bool { + return subtle.ConstantTimeCompare(hp.hash, other.hash) != 0 +} + +func HashPassword(password string) (HashedPassword, error) { + if len(password) == 0 { + return HashedPassword{}, ErrPasswordTooShort + } + + // These bytes are chosen at random. It's insecure to use a static salt to + // hash a set of passwords, but since we're only ever hashing a single + // password, using a static salt is fine. The salt prevents an attacker from + // using a rainbow table to retrieve the plaintext password from the hashed + // version, and that's all that's necessary for fusion's needs. + staticSalt := []byte{36, 129, 1, 54} + iter := 100 + keyLen := 32 + hash := pbkdf2.Key([]byte(password), staticSalt, iter, keyLen, sha256.New) + + return HashedPassword{ + hash: hash, + }, nil +} diff --git a/auth/password_test.go b/auth/password_test.go new file mode 100644 index 0000000..9371cb2 --- /dev/null +++ b/auth/password_test.go @@ -0,0 +1,71 @@ +package auth_test + +import ( + "testing" + + "github.com/0x2e/fusion/auth" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHashPassword(t *testing.T) { + for _, tt := range []struct { + explanation string + input string + wantErr error + }{ + { + explanation: "valid password succeeds", + input: "mypassword", + wantErr: nil, + }, + { + explanation: "empty password returns ErrPasswordTooShort", + input: "", + wantErr: auth.ErrPasswordTooShort, + }, + } { + t.Run(tt.explanation, func(t *testing.T) { + got, err := auth.HashPassword(tt.input) + require.Equal(t, tt.wantErr, err) + if tt.wantErr == nil { + assert.NotEmpty(t, got.Bytes()) + } + }) + } +} + +func TestHashedPasswordEquals(t *testing.T) { + for _, tt := range []struct { + explanation string + hashedPassword1 auth.HashedPassword + hashedPassword2 auth.HashedPassword + want bool + }{ + { + explanation: "same passwords match", + hashedPassword1: mustHashPassword("password1"), + hashedPassword2: mustHashPassword("password1"), + want: true, + }, + { + explanation: "different passwords don't match", + hashedPassword1: mustHashPassword("password1"), + hashedPassword2: mustHashPassword("password2"), + want: false, + }, + } { + t.Run(tt.explanation, func(t *testing.T) { + assert.Equal(t, tt.want, tt.hashedPassword1.Equals(tt.hashedPassword2)) + }) + } +} + +func mustHashPassword(password string) auth.HashedPassword { + hashedPassword, err := auth.HashPassword(password) + if err != nil { + panic(err) + } + return hashedPassword +} diff --git a/cmd/server/server.go b/cmd/server/server.go index 89e4855..aae7cfc 100644 --- a/cmd/server/server.go +++ b/cmd/server/server.go @@ -32,7 +32,7 @@ func main() { api.Run(api.Params{ Host: config.Host, Port: config.Port, - Password: config.Password, + PasswordHash: config.PasswordHash, UseSecureCookie: config.SecureCookie, TLSCert: config.TLSCert, TLSKey: config.TLSKey, diff --git a/conf/conf.go b/conf/conf.go index 3cab4af..f51499b 100644 --- a/conf/conf.go +++ b/conf/conf.go @@ -6,6 +6,7 @@ import ( "log" "os" + "github.com/0x2e/fusion/auth" "github.com/caarlos0/env/v11" "github.com/joho/godotenv" ) @@ -17,13 +18,13 @@ const ( ) type Conf struct { - Host string `env:"HOST" envDefault:"0.0.0.0"` - Port int `env:"PORT" envDefault:"8080"` - Password string `env:"PASSWORD"` - DB string `env:"DB" envDefault:"fusion.db"` - SecureCookie bool `env:"SECURE_COOKIE" envDefault:"false"` - TLSCert string `env:"TLS_CERT"` - TLSKey string `env:"TLS_KEY"` + Host string + Port int + PasswordHash auth.HashedPassword + DB string + SecureCookie bool + TLSCert string + TLSKey string } func Load() (Conf, error) { @@ -35,7 +36,15 @@ func Load() (Conf, error) { } else { log.Printf("read configuration from %s", dotEnvFilename) } - var conf Conf + var conf struct { + Host string `env:"HOST" envDefault:"0.0.0.0"` + Port int `env:"PORT" envDefault:"8080"` + Password string `env:"PASSWORD"` + DB string `env:"DB" envDefault:"fusion.db"` + SecureCookie bool `env:"SECURE_COOKIE" envDefault:"false"` + TLSCert string `env:"TLS_CERT"` + TLSKey string `env:"TLS_KEY"` + } if err := env.Parse(&conf); err != nil { panic(err) } @@ -43,8 +52,9 @@ func Load() (Conf, error) { fmt.Println(conf) } - if conf.Password == "" { - return Conf{}, errors.New("password is required") + pwHash, err := auth.HashPassword(conf.Password) + if err != nil { + return Conf{}, err } if (conf.TLSCert == "") != (conf.TLSKey == "") { @@ -54,5 +64,13 @@ func Load() (Conf, error) { conf.SecureCookie = true } - return conf, nil + return Conf{ + Host: conf.Host, + Port: conf.Port, + PasswordHash: pwHash, + DB: conf.DB, + SecureCookie: conf.SecureCookie, + TLSCert: conf.TLSCert, + TLSKey: conf.TLSKey, + }, nil }