Skip to content

Switch plaintext scratch tokens to use hash instead #4331

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

Merged
merged 22 commits into from
Jul 27, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ var migrations = []Migration{
NewMigration("Reformat and remove incorrect topics", reformatAndRemoveIncorrectTopics),
// v69 -> v70
NewMigration("move team units to team_unit table", moveTeamUnitsToTeamUnitTable),
// v70 -> v71
NewMigration("protect each scratch token", addScratchHash),
}

// Migrate database to current version
Expand Down
84 changes: 84 additions & 0 deletions models/migrations/v70.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright 2018 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"crypto/sha256"
"fmt"

"github.com/go-xorm/xorm"
"golang.org/x/crypto/pbkdf2"

"code.gitea.io/gitea/modules/generate"
"code.gitea.io/gitea/modules/util"
)

func addScratchHash(x *xorm.Engine) error {
// TwoFactor see models/twofactor.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might wanna put the commit ID in this comment as well, since that is where it's defined 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this PR is squashed and merged the commit ID will change to one we won't know.

type TwoFactor struct {
ID int64 `xorm:"pk autoincr"`
UID int64 `xorm:"UNIQUE"`
Secret string
ScratchToken string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed in models/twofactor.go 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still needed as below we take the plaintext scratch token, and then transform it into the hash. You'll see at the bottom of this file where the column is dropped.

ScratchSalt string
ScratchHash string
LastUsedPasscode string `xorm:"VARCHAR(10)"`
CreatedUnix util.TimeStamp `xorm:"INDEX created"`
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"`
}

if err := x.Sync2(new(TwoFactor)); err != nil {
return fmt.Errorf("Sync2: %v", err)
}

sess := x.NewSession()
defer sess.Close()

if err := sess.Begin(); err != nil {
return err
}

// transform all tokens to hashes
const batchSize = 100
for start := 0; ; start += batchSize {
tfas := make([]*TwoFactor, 0, batchSize)
if err := x.Limit(batchSize, start).Find(&tfas); err != nil {
return err
}
if len(tfas) == 0 {
break
}

for _, tfa := range tfas {
// generate salt
salt, err := generateSalt()
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a single error of generating a salt fail the complete migration? I think logging the error might work too? What is your opinion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should fail the migration, as if it just logs then a user might miss that and be unable to log into their Gitea install if generating salt fails.

}
tfa.ScratchSalt = salt
tfa.ScratchHash = hashToken(tfa.ScratchToken, salt)

if _, err := sess.ID(tfa.ID).Cols("scratch_salt, scratch_hash").Update(tfa); err != nil {
return fmt.Errorf("couldn't add in scratch_hash and scratch_salt: %v", err)
}

}
}

if err := dropTableColumns(sess, "two_factor", "scratch_token"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropping column on table that has uncommited changes in transaction will raise error in MSSQL. See #4360 for more details

return err
}
return sess.Commit()

}

func generateSalt() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these have to be used outside models/ then they should be Exported, not copied 😱

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

("these"== includes the hashToken-function as well)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are in here as copies because if the logic changes in models/ then it may affect potential other migrations in the future.

return generate.GetRandomString(10)
}

func hashToken(token, salt string) string {
tempHash := pbkdf2.Key([]byte(token), []byte(salt), 10000, 50, sha256.New)
return fmt.Sprintf("%x", tempHash)
}
29 changes: 22 additions & 7 deletions models/twofactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ import (
"crypto/cipher"
"crypto/md5"
"crypto/rand"
"crypto/sha256"
"crypto/subtle"
"encoding/base64"
"errors"
"fmt"
"io"

"github.com/pquerna/otp/totp"
"golang.org/x/crypto/pbkdf2"

"code.gitea.io/gitea/modules/generate"
"code.gitea.io/gitea/modules/setting"
Expand All @@ -26,28 +29,40 @@ type TwoFactor struct {
ID int64 `xorm:"pk autoincr"`
UID int64 `xorm:"UNIQUE"`
Secret string
ScratchToken string
ScratchSalt string
ScratchHash string
LastUsedPasscode string `xorm:"VARCHAR(10)"`
CreatedUnix util.TimeStamp `xorm:"INDEX created"`
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"`
}

// GenerateScratchToken recreates the scratch token the user is using.
func (t *TwoFactor) GenerateScratchToken() error {
func (t *TwoFactor) GenerateScratchToken() (string, error) {
token, err := generate.GetRandomString(8)
if err != nil {
return err
return "", err
}
t.ScratchToken = token
return nil
t.ScratchSalt, _ = generateSalt()
t.ScratchHash = hashToken(token, t.ScratchSalt)
return token, nil
}

func generateSalt() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very sure we have this function (and hashToken) defined somewhere else (related to hashing user passwords...). Find those and use them (Export if necessary)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have hashPassword which is pretty much the same as this, but with the name hashPassword it is a bit misleading as the token is not a password.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest moving the "crypto" part for hashing into the module package and reuse it for every hash generation we need!?

return generate.GetRandomString(10)
}

func hashToken(token, salt string) string {
tempHash := pbkdf2.Key([]byte(token), []byte(salt), 10000, 50, sha256.New)
return fmt.Sprintf("%x", tempHash)
}

// VerifyScratchToken verifies if the specified scratch token is valid.
func (t *TwoFactor) VerifyScratchToken(token string) bool {
if len(token) == 0 {
return false
}
return subtle.ConstantTimeCompare([]byte(token), []byte(t.ScratchToken)) == 1
tempHash := hashToken(token, t.ScratchSalt)
return subtle.ConstantTimeCompare([]byte(t.ScratchHash), []byte(tempHash)) == 1
}

func (t *TwoFactor) getEncryptionKey() []byte {
Expand Down Expand Up @@ -118,7 +133,7 @@ func aesDecrypt(key, text []byte) ([]byte, error) {

// NewTwoFactor creates a new two-factor authentication token.
func NewTwoFactor(t *TwoFactor) error {
err := t.GenerateScratchToken()
_, err := t.GenerateScratchToken()
if err != nil {
return err
}
Expand Down
9 changes: 7 additions & 2 deletions routers/user/auth.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// Copyright 2014 The Gogs Authors. All rights reserved.
// Copyright 2018 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

Expand Down Expand Up @@ -305,7 +306,11 @@ func TwoFactorScratchPost(ctx *context.Context, form auth.TwoFactorScratchAuthFo
// Validate the passcode with the stored TOTP secret.
if twofa.VerifyScratchToken(form.Token) {
// Invalidate the scratch token.
twofa.ScratchToken = ""
_, err = twofa.GenerateScratchToken()
if err != nil {
ctx.ServerError("UserSignIn", err)
return
}
if err = models.UpdateTwoFactor(twofa); err != nil {
ctx.ServerError("UserSignIn", err)
return
Expand All @@ -320,7 +325,7 @@ func TwoFactorScratchPost(ctx *context.Context, form auth.TwoFactorScratchAuthFo

handleSignInFull(ctx, u, remember, false)
ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used"))
ctx.Redirect(setting.AppSubURL + "/user/settings/two_factor")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this incorrect prior to this change? 🤔 (If so we need to sort these routes out...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missed when the user settings pages were refactored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will open a new PR for this line.

ctx.Redirect(setting.AppSubURL + "/user/settings/security")
return
}

Expand Down
9 changes: 5 additions & 4 deletions routers/user/setting/security_twofa.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ func RegenerateScratchTwoFactor(ctx *context.Context) {
return
}

if err = t.GenerateScratchToken(); err != nil {
token, err := t.GenerateScratchToken()
if err != nil {
ctx.ServerError("SettingsTwoFactor", err)
return
}
Expand All @@ -42,7 +43,7 @@ func RegenerateScratchTwoFactor(ctx *context.Context) {
return
}

ctx.Flash.Success(ctx.Tr("settings.twofa_scratch_token_regenerated", t.ScratchToken))
ctx.Flash.Success(ctx.Tr("settings.twofa_scratch_token_regenerated", token))
ctx.Redirect(setting.AppSubURL + "/user/settings/security")
}

Expand Down Expand Up @@ -170,7 +171,7 @@ func EnrollTwoFactorPost(ctx *context.Context, form auth.TwoFactorAuthForm) {
ctx.ServerError("SettingsTwoFactor", err)
return
}
err = t.GenerateScratchToken()
token, err := t.GenerateScratchToken()
if err != nil {
ctx.ServerError("SettingsTwoFactor", err)
return
Expand All @@ -183,6 +184,6 @@ func EnrollTwoFactorPost(ctx *context.Context, form auth.TwoFactorAuthForm) {

ctx.Session.Delete("twofaSecret")
ctx.Session.Delete("twofaUri")
ctx.Flash.Success(ctx.Tr("settings.twofa_enrolled", t.ScratchToken))
ctx.Flash.Success(ctx.Tr("settings.twofa_enrolled", token))
ctx.Redirect(setting.AppSubURL + "/user/settings/security")
}