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.

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

Only allow local login if password is non-empty #5906

Merged
merged 2 commits into from Jan 30, 2019

Conversation

@zeripath
Copy link
Contributor

zeripath commented Jan 30, 2019

Prohibit local login if password is empty. This protects oauth2 users created by
an admin with an empty password and with must_change_password set to false.

With thanks to @petercolberg

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 30, 2019

@techknowlogick Does this look like the correct approach?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 30, 2019

Codecov Report

Merging #5906 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5906   +/-   ##
======================================
  Coverage      38%     38%           
======================================
  Files         328     328           
  Lines       48343   48343           
======================================
  Hits        18375   18375           
+ Misses      27329   27328    -1     
- Partials     2639    2640    +1
Impacted Files Coverage Δ
modules/lfs/server.go 44.27% <0%> (ø) ⬆️
models/login_source.go 26.3% <100%> (ø) ⬆️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️
models/unit.go 14.28% <0%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80098bd...760f6cd. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Jan 30, 2019

@lafriks lafriks merged commit 0f295ab into go-gitea:master Jan 30, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 30, 2019

@zeripath please backport

@zeripath zeripath deleted the zeripath:reject-empty-passwords branch Jan 30, 2019

zeripath added a commit to zeripath/gitea that referenced this pull request Jan 30, 2019

lafriks added a commit that referenced this pull request Jan 30, 2019

@petercolberg

This comment has been minimized.

Copy link
Contributor

petercolberg commented Jan 31, 2019

Thank you for addressing this issue!

I was pondering a bit about the difference between your and my patch.

The committed solution introduces a subtle timing issue, where a third party may query whether a user has an unset password (IsPasswordSet() == false yields one call to ConstantTimeCompare()) or a set password (IsPasswordSet() == true and subsequent, second ValidatePassword() yield two calls to ConstantTimeCompare()). Whereas my patch rejects invalid input that is provided by the attacker. This does not seem a problem per se, but it would be nice to address this subtle issue in a future version.

From dbcc3df481ed1af490fe6243c8aabb60dd1b8caa Mon Sep 17 00:00:00 2001
From: Peter Colberg <peter@colberg.org>
Date: Thu, 24 Jan 2019 23:29:39 -0500
Subject: [PATCH] Reject empty password on validation

Prohibit unauthorized API access through an oauth2 user that was created by
an admin with an empty password and with must_change_password set to false.

Signed-off-by: Peter Colberg <peter@colberg.org>
---
 models/user.go      | 13 ++++++++++---
 models/user_test.go | 17 +++++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/models/user.go b/models/user.go
index 4ab78ec04..10e034da9 100644
--- a/models/user.go
+++ b/models/user.go
@@ -433,15 +433,22 @@ func (u *User) HashPassword(passwd string) {
        u.Passwd = hashPassword(passwd, u.Salt)
 }

-// ValidatePassword checks if given password matches the one belongs to the user.
-func (u *User) ValidatePassword(passwd string) bool {
+func validatePassword(u *User, passwd string) bool {
        tempHash := hashPassword(passwd, u.Salt)
        return subtle.ConstantTimeCompare([]byte(u.Passwd), []byte(tempHash)) == 1
 }

+// ValidatePassword checks if given password matches the one belongs to the user.
+func (u *User) ValidatePassword(passwd string) bool {
+       if (passwd == "") {
+               return false
+       }
+       return validatePassword(u, passwd);
+}
+
 // IsPasswordSet checks if the password is set or left empty
 func (u *User) IsPasswordSet() bool {
-       return !u.ValidatePassword("")
+       return !validatePassword(u, "")
 }
  
 // UploadAvatar saves custom avatar for user.
diff --git a/models/user_test.go b/models/user_test.go
index 9d011f32a..c44bbe48a 100644
--- a/models/user_test.go
+++ b/models/user_test.go
@@ -161,6 +161,23 @@ func BenchmarkHashPassword(b *testing.B) {
        }
 }

+func TestValidatePassword(t *testing.T) {
+       bs := make([]byte, 16)
+       rand.Read(bs)
+       u := &User{Salt: string(bs)}
+
+       u.HashPassword("")
+       assert.False(t, u.IsPasswordSet())
+       assert.False(t, u.ValidatePassword(""))
+       assert.False(t, u.ValidatePassword("password1337"))
+
+       u.HashPassword("rvpwt4wdrbaxdknm")
+       assert.True(t, u.IsPasswordSet())
+       assert.False(t, u.ValidatePassword(""))
+       assert.False(t, u.ValidatePassword("password1337"))
+       assert.True(t, u.ValidatePassword("rvpwt4wdrbaxdknm"))
+}
+
 func TestGetOrgRepositoryIDs(t *testing.T) {
        assert.NoError(t, PrepareTestDatabase())
        user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
--
2.20.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment