From 1aef6e1cb3fed2c1206bb5e2cabf31899d4cf663 Mon Sep 17 00:00:00 2001 From: swdee Date: Fri, 13 Mar 2020 22:28:32 +1300 Subject: [PATCH 1/8] added sanity checks on user/pass string length when creating user via RPC API --- api/keystore/service.go | 12 ++++++++- api/keystore/service_test.go | 52 ++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 604ec5d87c9d..027abfaa9476 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -23,8 +23,14 @@ import ( jsoncodec "github.com/ava-labs/gecko/utils/json" ) +const ( + // maxUserPassLen is the maximum length of the username or password allowed + maxUserPassLen = 1024 +) + var ( - errEmptyUsername = errors.New("username can't be the empty string") + errEmptyUsername = errors.New("username can't be the empty string") + errUserPassMaxLength = errors.New(fmt.Sprintf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen)) ) // KeyValuePair ... @@ -114,6 +120,10 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre ks.lock.Lock() defer ks.lock.Unlock() + if len(args.Username) > maxUserPassLen || len(args.Password) > maxUserPassLen { + return errUserPassMaxLength + } + ks.log.Verbo("CreateUser called with %s", args.Username) if args.Username == "" { diff --git a/api/keystore/service_test.go b/api/keystore/service_test.go index ab2a096fd6e9..b1f238505b85 100644 --- a/api/keystore/service_test.go +++ b/api/keystore/service_test.go @@ -5,6 +5,8 @@ package keystore import ( "bytes" + "fmt" + "math/rand" "testing" "github.com/ava-labs/gecko/database/memdb" @@ -56,6 +58,56 @@ func TestServiceCreateUser(t *testing.T) { } } +// genStr returns a string of given length +func genStr(n int) string { + b := make([]byte, n) + rand.Read(b) + return fmt.Sprintf("%x", b)[:n] +} + +// TestServiceCreateUserArgsChecks generates excessively long usernames or +// passwords to assure the santity checks on string length are not exceeded +func TestServiceCreateUserArgsCheck(t *testing.T) { + ks := Keystore{} + ks.Initialize(logging.NoLog{}, memdb.New()) + + { + reply := CreateUserReply{} + err := ks.CreateUser(nil, &CreateUserArgs{ + Username: genStr(maxUserPassLen + 1), + Password: "shortpass", + }, &reply) + + if reply.Success || err != errUserPassMaxLength { + t.Fatal("User was created when it should have been rejected due to too long a Username, err =", err) + } + } + + { + reply := CreateUserReply{} + err := ks.CreateUser(nil, &CreateUserArgs{ + Username: "shortuser", + Password: genStr(maxUserPassLen + 1), + }, &reply) + + if reply.Success || err != errUserPassMaxLength { + t.Fatal("User was created when it should have been rejected due to too long a Password, err =", err) + } + } + + { + reply := ListUsersReply{} + if err := ks.ListUsers(nil, &ListUsersArgs{}, &reply); err != nil { + t.Fatal(err) + } + + if len(reply.Users) > 0 { + t.Fatalf("A user exists when there should be none") + } + } + +} + func TestServiceCreateDuplicate(t *testing.T) { ks := Keystore{} ks.Initialize(logging.NoLog{}, memdb.New()) From c737fb8ab4ac9a279b615440e51146b25da5536b Mon Sep 17 00:00:00 2001 From: swdee Date: Fri, 13 Mar 2020 23:25:42 +1300 Subject: [PATCH 2/8] removed commit from another branch, due to not branching from master when creating this branch --- utils/logging/log.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/logging/log.go b/utils/logging/log.go index 04d69fba2270..a9e6aec40f9d 100644 --- a/utils/logging/log.go +++ b/utils/logging/log.go @@ -171,7 +171,7 @@ func (l *Log) format(level Level, format string, args ...interface{}) string { return fmt.Sprintf("%s[%s]%s %s\n", level, - time.Now().Format("01-02|15:04:05.000"), + time.Now().Format("01-02|15:04:05"), prefix, text) } From 33ed4b5d24218dc9fbbe7f290a922677e2649def Mon Sep 17 00:00:00 2001 From: swdee Date: Sat, 14 Mar 2020 08:29:42 +1300 Subject: [PATCH 3/8] change format of errUserPassMaxLength --- api/keystore/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 027abfaa9476..8ddf3da48aa0 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -30,7 +30,7 @@ const ( var ( errEmptyUsername = errors.New("username can't be the empty string") - errUserPassMaxLength = errors.New(fmt.Sprintf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen)) + errUserPassMaxLength = fmt.Errorf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen) ) // KeyValuePair ... From 946fb662a2b4482de4a74bb8a48832ee634a650a Mon Sep 17 00:00:00 2001 From: swdee Date: Sat, 14 Mar 2020 10:04:57 +1300 Subject: [PATCH 4/8] added in checking of password strength when creating a new user --- api/keystore/service.go | 18 ++++++++++++ api/keystore/service_test.go | 54 +++++++++++++++++++++++++++--------- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 8ddf3da48aa0..472ce757991d 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -21,16 +21,30 @@ import ( "github.com/ava-labs/gecko/vms/components/codec" jsoncodec "github.com/ava-labs/gecko/utils/json" + zxcvbn "github.com/nbutton23/zxcvbn-go" ) const ( // maxUserPassLen is the maximum length of the username or password allowed maxUserPassLen = 1024 + + // requiredPassScore defines the score a password must achieve to be accepted + // as a password with strong characteristics by the zxcvbn package + // + // The scoring mechanism defined is as follows; + // + // 0 # too guessable: risky password. (guesses < 10^3) + // 1 # very guessable: protection from throttled online attacks. (guesses < 10^6) + // 2 # somewhat guessable: protection from unthrottled online attacks. (guesses < 10^8) + // 3 # safely unguessable: moderate protection from offline slow-hash scenario. (guesses < 10^10) + // 4 # very unguessable: strong protection from offline slow-hash scenario. (guesses >= 10^10) + requiredPassScore = 2 ) var ( errEmptyUsername = errors.New("username can't be the empty string") errUserPassMaxLength = fmt.Errorf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen) + errWeakPassword = errors.New("Failed to create user as the given password is to weak. Passwords must be 8 or more characters and contain a combination of UPPER and lowercase letters, numbers, and special characters") ) // KeyValuePair ... @@ -133,6 +147,10 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre return fmt.Errorf("user already exists: %s", args.Username) } + if zxcvbn.PasswordStrength(args.Password, nil).Score < requiredPassScore { + return errWeakPassword + } + usr := &User{} if err := usr.Initialize(args.Password); err != nil { return err diff --git a/api/keystore/service_test.go b/api/keystore/service_test.go index b1f238505b85..0868a29015e8 100644 --- a/api/keystore/service_test.go +++ b/api/keystore/service_test.go @@ -14,6 +14,12 @@ import ( "github.com/ava-labs/gecko/utils/logging" ) +var ( + // strongPassword defines a password used for the following tests that + // scores high enough to pass the password strength scoring system + strongPassword = "N_+=_jJ;^(<;{4,:*m6CET}'&N;83FYK.wtNpwp-Jt" +) + func TestServiceListNoUsers(t *testing.T) { ks := Keystore{} ks.Initialize(logging.NoLog{}, memdb.New()) @@ -35,7 +41,7 @@ func TestServiceCreateUser(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, }, &reply); err != nil { t.Fatal(err) } @@ -75,7 +81,7 @@ func TestServiceCreateUserArgsCheck(t *testing.T) { reply := CreateUserReply{} err := ks.CreateUser(nil, &CreateUserArgs{ Username: genStr(maxUserPassLen + 1), - Password: "shortpass", + Password: strongPassword, }, &reply) if reply.Success || err != errUserPassMaxLength { @@ -105,7 +111,29 @@ func TestServiceCreateUserArgsCheck(t *testing.T) { t.Fatalf("A user exists when there should be none") } } +} + +// TestServiceCreateUserWeakPassword tests creating a new user with a weak +// password to ensure the password strength check is working +func TestServiceCreateUserWeakPassword(t *testing.T) { + ks := Keystore{} + ks.Initialize(logging.NoLog{}, memdb.New()) + + { + reply := CreateUserReply{} + err := ks.CreateUser(nil, &CreateUserArgs{ + Username: "bob", + Password: "weak", + }, &reply) + if err != errWeakPassword { + t.Error("Unexpected error occurred when testing weak password:", err) + } + + if reply.Success { + t.Fatal("User was created when it should have been rejected due to weak password") + } + } } func TestServiceCreateDuplicate(t *testing.T) { @@ -116,7 +144,7 @@ func TestServiceCreateDuplicate(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, }, &reply); err != nil { t.Fatal(err) } @@ -129,7 +157,7 @@ func TestServiceCreateDuplicate(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ Username: "bob", - Password: "launch!", + Password: strongPassword, }, &reply); err == nil { t.Fatalf("Should have errored due to the username already existing") } @@ -142,7 +170,7 @@ func TestServiceCreateUserNoName(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ - Password: "launch", + Password: strongPassword, }, &reply); err == nil { t.Fatalf("Shouldn't have allowed empty username") } @@ -156,7 +184,7 @@ func TestServiceUseBlockchainDB(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, }, &reply); err != nil { t.Fatal(err) } @@ -166,7 +194,7 @@ func TestServiceUseBlockchainDB(t *testing.T) { } { - db, err := ks.GetDatabase(ids.Empty, "bob", "launch") + db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword) if err != nil { t.Fatal(err) } @@ -176,7 +204,7 @@ func TestServiceUseBlockchainDB(t *testing.T) { } { - db, err := ks.GetDatabase(ids.Empty, "bob", "launch") + db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword) if err != nil { t.Fatal(err) } @@ -196,7 +224,7 @@ func TestServiceExportImport(t *testing.T) { reply := CreateUserReply{} if err := ks.CreateUser(nil, &CreateUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, }, &reply); err != nil { t.Fatal(err) } @@ -206,7 +234,7 @@ func TestServiceExportImport(t *testing.T) { } { - db, err := ks.GetDatabase(ids.Empty, "bob", "launch") + db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword) if err != nil { t.Fatal(err) } @@ -218,7 +246,7 @@ func TestServiceExportImport(t *testing.T) { exportReply := ExportUserReply{} if err := ks.ExportUser(nil, &ExportUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, }, &exportReply); err != nil { t.Fatal(err) } @@ -230,7 +258,7 @@ func TestServiceExportImport(t *testing.T) { reply := ImportUserReply{} if err := newKS.ImportUser(nil, &ImportUserArgs{ Username: "bob", - Password: "launch", + Password: strongPassword, User: exportReply.User, }, &reply); err != nil { t.Fatal(err) @@ -241,7 +269,7 @@ func TestServiceExportImport(t *testing.T) { } { - db, err := newKS.GetDatabase(ids.Empty, "bob", "launch") + db, err := newKS.GetDatabase(ids.Empty, "bob", strongPassword) if err != nil { t.Fatal(err) } From 2d0f56bbaf3c510aafd7bbb8900a52cfb40484d5 Mon Sep 17 00:00:00 2001 From: swdee Date: Tue, 17 Mar 2020 08:22:16 +1300 Subject: [PATCH 5/8] log call to createuser even if rejected by username/password length being to long --- api/keystore/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 472ce757991d..597f0b3c705d 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -134,12 +134,12 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre ks.lock.Lock() defer ks.lock.Unlock() + ks.log.Verbo("CreateUser called with %s", args.Username[:maxUserPassLen]) + if len(args.Username) > maxUserPassLen || len(args.Password) > maxUserPassLen { return errUserPassMaxLength } - ks.log.Verbo("CreateUser called with %s", args.Username) - if args.Username == "" { return errEmptyUsername } From d27aa669964b3a8efa04c98aa97e9f7fbb7ee018 Mon Sep 17 00:00:00 2001 From: swdee Date: Tue, 17 Mar 2020 08:31:37 +1300 Subject: [PATCH 6/8] changed error message when password is rejected --- api/keystore/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 597f0b3c705d..12bb8d146b7f 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -44,7 +44,7 @@ const ( var ( errEmptyUsername = errors.New("username can't be the empty string") errUserPassMaxLength = fmt.Errorf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen) - errWeakPassword = errors.New("Failed to create user as the given password is to weak. Passwords must be 8 or more characters and contain a combination of UPPER and lowercase letters, numbers, and special characters") + errWeakPassword = errors.New("Failed to create user as the given password is to weak. A stronger password is one of 8 or more characters containing attributes of upper and lowercase letters, numbers, and/or special characters") ) // KeyValuePair ... From 93d2e66c21956e0faab4b4df97eb3651c648a493 Mon Sep 17 00:00:00 2001 From: swdee Date: Tue, 17 Mar 2020 08:55:52 +1300 Subject: [PATCH 7/8] fix build problem with limiting length of username output when logging --- api/keystore/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 12bb8d146b7f..dd6940f91fbf 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -134,7 +134,7 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre ks.lock.Lock() defer ks.lock.Unlock() - ks.log.Verbo("CreateUser called with %s", args.Username[:maxUserPassLen]) + ks.log.Verbo("CreateUser called with %.*s", maxUserPassLen, args.Username) if len(args.Username) > maxUserPassLen || len(args.Password) > maxUserPassLen { return errUserPassMaxLength From 05a2bcbef5a3cded8eedc53bc5fd4572c313569a Mon Sep 17 00:00:00 2001 From: swdee Date: Wed, 18 Mar 2020 07:53:25 +1300 Subject: [PATCH 8/8] fix spelling typo --- api/keystore/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index dd6940f91fbf..dad7073f8e3c 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -44,7 +44,7 @@ const ( var ( errEmptyUsername = errors.New("username can't be the empty string") errUserPassMaxLength = fmt.Errorf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen) - errWeakPassword = errors.New("Failed to create user as the given password is to weak. A stronger password is one of 8 or more characters containing attributes of upper and lowercase letters, numbers, and/or special characters") + errWeakPassword = errors.New("Failed to create user as the given password is too weak. A stronger password is one of 8 or more characters containing attributes of upper and lowercase letters, numbers, and/or special characters") ) // KeyValuePair ...