From aff83c9a6a835a7849792d89159550c7ff0e4ef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=2E=20R=C3=B8dseth?= Date: Thu, 21 Jun 2018 11:47:54 +0200 Subject: [PATCH] Add a test the measures the timing of the bcrypt algo --- TODO.md | 5 ++-- userstate.go | 4 ++- userstate_test.go | 72 ++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/TODO.md b/TODO.md index f5b80a8..7f1aef6 100644 --- a/TODO.md +++ b/TODO.md @@ -4,11 +4,10 @@ TODO Priority -------- -- [ ] Write tests for timing attacks (the way it is currently done should be safe, but write tests to be sure, now and for future versions). - [ ] Document how to add a custom role (like admin/user/public). -For the next version --------------------- +For the next major version +-------------------------- - [ ] Let `HashPassword` return an error instead of panic if bcrypt should fail. - [ ] Let `NewUserState` return an error instead of the user having to check the Redis connection first. diff --git a/userstate.go b/userstate.go index d188437..295d146 100644 --- a/userstate.go +++ b/userstate.go @@ -20,7 +20,9 @@ const ( var ( minConfirmationCodeLength = 20 // minimum length of the confirmation code - ErrNotFound = errors.New("Not found") + + // ErrNotFound is used as an error if not finding what is being searched for + ErrNotFound = errors.New("Not found") ) // Used for dealing with the user state, users and passwords. diff --git a/userstate_test.go b/userstate_test.go index 51b250d..7b0b29d 100644 --- a/userstate_test.go +++ b/userstate_test.go @@ -90,6 +90,8 @@ func TestPasswordBackward(t *testing.T) { if !userstate.HasUser("bob") { t.Error("Error, user bob should exist") } + defer userstate.RemoveUser("bob") + userstate.SetPasswordAlgo("sha256") if !userstate.CorrectPassword("bob", "hunter1") { t.Error("Error, the sha256 password really is correct") @@ -104,8 +106,6 @@ func TestPasswordBackward(t *testing.T) { if !userstate.CorrectPassword("bob", "hunter1") { t.Error("Error, the sha256 password is not correct when checking with bcrypt+") } - - userstate.RemoveUser("bob") } // Check if the functionality for backwards compatible hashing works @@ -116,6 +116,8 @@ func TestPasswordNotBackward(t *testing.T) { if !userstate.HasUser("bob") { t.Error("Error, user bob should exist") } + defer userstate.RemoveUser("bob") + userstate.SetPasswordAlgo("sha256") if userstate.CorrectPassword("bob", "hunter1") { t.Error("Error, the password is stored as bcrypt, should not be okay with sha256") @@ -125,8 +127,6 @@ func TestPasswordNotBackward(t *testing.T) { if !userstate.CorrectPassword("bob", "hunter1") { t.Error("Error, the password should be correct when checking with bcrypt") } - - userstate.RemoveUser("bob") } func TestPasswordAlgoMatching(t *testing.T) { @@ -173,6 +173,7 @@ func TestChangePassword(t *testing.T) { if !userstate.HasUser("bob") { t.Error("Error, user bob should exist") } + defer userstate.RemoveUser("bob") // Check that the password is "hunter1" if !userstate.CorrectPassword("bob", "hunter1") { @@ -209,8 +210,6 @@ func TestChangePassword(t *testing.T) { if userstate.CorrectPassword("bob", "hunter2") { t.Error("Error, password is incorrect: should not be hunter2!") } - - userstate.RemoveUser("bob") } func TestTokens(t *testing.T) { @@ -221,6 +220,7 @@ func TestTokens(t *testing.T) { if !userstate.HasUser("bob") { t.Error("Error, user bob should exist") } + defer userstate.RemoveUser("bob") // Set a token that will expire in 200 milliseconds userstate.SetToken("bob", "asdf123", time.Millisecond*200) @@ -242,9 +242,6 @@ func TestTokens(t *testing.T) { if err == nil || retval != "" { t.Error("Error, token is incorrect: should be gone!") } - - // Remove bob - userstate.RemoveUser("bob") } func TestEmail(t *testing.T) { @@ -258,7 +255,62 @@ func TestEmail(t *testing.T) { t.Error("Error, the e-mail address should belong to bob, but belongs to:", username) } username, err = userstate.HasEmail("rob@zombo.com") - if err != ErrNotFound { // NOTE: == + if err != ErrNotFound { t.Error("Error, the e-mail should not exist: " + username) } } + +func TestTiming(t *testing.T) { + userstate := NewUserStateSimple() + + userstate.SetPasswordAlgo("bcrypt") + userstate.AddUser("bob", "hunter1", "bob@zombo.com") + if !userstate.HasUser("bob") { + t.Error("Error, user bob should exist") + } + defer userstate.RemoveUser("bob") + + // First run takes a bit longer + _ = userstate.CorrectPassword("bob", "asdf") + + start1 := time.Now() + if !userstate.CorrectPassword("bob", "hunter1") { + t.Error("Error, the password IS correct") + } + elapsed1 := time.Since(start1) + + start2 := time.Now() + if userstate.CorrectPassword("bob", "_") { + t.Error("Error, the password is NOT correct") + } + elapsed2 := time.Since(start2) + + start3 := time.Now() + if userstate.CorrectPassword("bob", "abc123asfdasdfasfasdfqwertyqwertyqwerty") { + t.Error("Error, the password is NOT correct") + } + elapsed3 := time.Since(start3) + + start4 := time.Now() + if userstate.CorrectPassword("bob", "abc123asfdasdfasfasdfqwertyqwertyqwertyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy") { + t.Error("Error, the password is NOT correct") + } + elapsed4 := time.Since(start4) + + // The tolerance is +- 1/30th of the first elapsed time + base_min := elapsed1 - time.Duration(elapsed1 / 30) + base_max := elapsed1 + time.Duration(elapsed1 / 30) + + if elapsed2 < base_min || elapsed2 > base_max { + t.Error("Checking passwords of different lengths should not take much longer or shorter") + } + + if elapsed3 < base_min || elapsed3 > base_max { + t.Error("Checking passwords of different lengths should not take much longer or shorter") + } + + if elapsed4 < base_min || elapsed4 > base_max { + t.Error("Checking passwords of different lengths should not take much longer or shorter") + } + //fmt.Println(base_min, base_max, elapsed1, elapsed2, elapsed3, elapsed4) +}