fix(security): reject replayed TOTP codes
REVIEW-001 (M0) L5s: a code valid for ±90s could be replayed. TOTP is a login fallback (D-005) and the only factor the bootstrap admin has (D-041), so a code observed once (shoulder-surfed, phished, in a log) must not work twice. Track the highest accepted counter per identity and refuse any counter that is not strictly newer. The search is newest-first and compares in constant time; a replay is reported as ErrInvalidCode — indistinguishable from a wrong code, so the response is not an oracle (A3). A fresh secret (re-enrollment) resets the counter.
This commit is contained in:
parent
03a9db03dc
commit
93ec8b8131
2 changed files with 130 additions and 9 deletions
|
|
@ -10,17 +10,24 @@ package totp
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"crypto/subtle"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/pquerna/otp"
|
"github.com/pquerna/otp"
|
||||||
"github.com/pquerna/otp/totp"
|
"github.com/pquerna/otp/totp"
|
||||||
)
|
)
|
||||||
|
|
||||||
// ErrInvalidCode is returned when a code does not match the stored secret.
|
// ErrInvalidCode is returned when a code does not match the stored secret — or
|
||||||
|
// when it does but has already been accepted (a replay, L5s). The two cases are
|
||||||
|
// deliberately indistinguishable: never tell an attacker which.
|
||||||
var ErrInvalidCode = errors.New("totp: invalid code")
|
var ErrInvalidCode = errors.New("totp: invalid code")
|
||||||
|
|
||||||
|
// errNoStore is returned when the service was built without a Store.
|
||||||
|
var errNoStore = errors.New("totp: no store configured")
|
||||||
|
|
||||||
// Store persists an identity's TOTP secret. It is implemented by the auth
|
// Store persists an identity's TOTP secret. It is implemented by the auth
|
||||||
// service, which encrypts at rest through the AEAD sealer (A9) — so this
|
// service, which encrypts at rest through the AEAD sealer (A9) — so this
|
||||||
// package never touches the ciphertext or the key.
|
// package never touches the ciphertext or the key.
|
||||||
|
|
@ -51,6 +58,13 @@ type Generator struct {
|
||||||
store Store
|
store Store
|
||||||
now func() time.Time
|
now func() time.Time
|
||||||
opts totp.ValidateOpts
|
opts totp.ValidateOpts
|
||||||
|
|
||||||
|
// mu guards last, the highest counter accepted per identity (L5s). Replay
|
||||||
|
// defence is in-process: a restart forgets the counters, but the skew
|
||||||
|
// window is only ±90s, so the exposure is bounded and the alternative (a
|
||||||
|
// DB round-trip per verification) is not worth it at M0.
|
||||||
|
mu sync.Mutex
|
||||||
|
last map[int64]uint64
|
||||||
}
|
}
|
||||||
|
|
||||||
var _ Service = (*Generator)(nil)
|
var _ Service = (*Generator)(nil)
|
||||||
|
|
@ -71,6 +85,7 @@ func New(issuer string, store Store) *Generator {
|
||||||
Digits: otp.DigitsSix,
|
Digits: otp.DigitsSix,
|
||||||
Algorithm: otp.AlgorithmSHA1,
|
Algorithm: otp.AlgorithmSHA1,
|
||||||
},
|
},
|
||||||
|
last: make(map[int64]uint64),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -82,10 +97,11 @@ func (g *Generator) SetClock(fn func() time.Time) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// Begin issues a fresh secret, stores it encrypted (A9) and returns the otpauth
|
// Begin issues a fresh secret, stores it encrypted (A9) and returns the otpauth
|
||||||
// URL for the authenticator app.
|
// URL for the authenticator app. A fresh secret resets the identity's replay
|
||||||
|
// counter (L5s): the new secret is a new sequence.
|
||||||
func (g *Generator) Begin(ctx context.Context, identityID int64, account string) (Issued, error) {
|
func (g *Generator) Begin(ctx context.Context, identityID int64, account string) (Issued, error) {
|
||||||
if g.store == nil {
|
if g.store == nil {
|
||||||
return Issued{}, errors.New("totp: no store configured")
|
return Issued{}, errNoStore
|
||||||
}
|
}
|
||||||
key, err := totp.Generate(totp.GenerateOpts{Issuer: g.issuer, AccountName: account})
|
key, err := totp.Generate(totp.GenerateOpts{Issuer: g.issuer, AccountName: account})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
@ -95,29 +111,82 @@ func (g *Generator) Begin(ctx context.Context, identityID int64, account string)
|
||||||
if err := g.store.PutTOTPSecret(ctx, identityID, []byte(key.Secret())); err != nil {
|
if err := g.store.PutTOTPSecret(ctx, identityID, []byte(key.Secret())); err != nil {
|
||||||
return Issued{}, err
|
return Issued{}, err
|
||||||
}
|
}
|
||||||
|
g.forget(identityID)
|
||||||
return Issued{URL: key.URL()}, nil
|
return Issued{URL: key.URL()}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify validates code against the stored secret, then records the
|
// Verify validates code against the stored secret, then records the
|
||||||
// verification. The code and the secret never reach an error string or a log.
|
// verification. The code and the secret never reach an error string or a log.
|
||||||
|
//
|
||||||
|
// A code that matches but whose counter has already been accepted is rejected
|
||||||
|
// as a replay (L5s), so a code observed (shoulder-surfed, logged, phished) in
|
||||||
|
// its 90-second validity cannot be used twice.
|
||||||
func (g *Generator) Verify(ctx context.Context, identityID int64, code string) error {
|
func (g *Generator) Verify(ctx context.Context, identityID int64, code string) error {
|
||||||
if code == "" {
|
if code == "" {
|
||||||
return ErrInvalidCode
|
return ErrInvalidCode
|
||||||
}
|
}
|
||||||
|
if g.store == nil {
|
||||||
|
return errNoStore
|
||||||
|
}
|
||||||
secret, err := g.store.TOTPSecret(ctx, identityID)
|
secret, err := g.store.TOTPSecret(ctx, identityID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
ok, err := totp.ValidateCustom(code, string(secret), g.now().UTC(), g.opts)
|
counter, ok := g.matchCounter(secret, code)
|
||||||
if err != nil {
|
|
||||||
// A malformed code is an invalid code, not a server fault.
|
|
||||||
return ErrInvalidCode
|
|
||||||
}
|
|
||||||
if !ok {
|
if !ok {
|
||||||
return ErrInvalidCode
|
return ErrInvalidCode
|
||||||
}
|
}
|
||||||
|
if !g.acceptCounter(identityID, counter) {
|
||||||
|
return ErrInvalidCode // a replay — indistinguishable from a bad code
|
||||||
|
}
|
||||||
if err := g.store.MarkTOTPVerified(ctx, identityID); err != nil {
|
if err := g.store.MarkTOTPVerified(ctx, identityID); err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// matchCounter finds the time step whose code equals code, searching the skew
|
||||||
|
// window newest-first (L5s: newest-first means the returned counter is the
|
||||||
|
// highest matching step, so an earlier step is never preferred over a later
|
||||||
|
// one). The comparison is constant-time.
|
||||||
|
func (g *Generator) matchCounter(secret []byte, code string) (uint64, bool) {
|
||||||
|
period := int64(g.opts.Period)
|
||||||
|
if period <= 0 {
|
||||||
|
period = 30
|
||||||
|
}
|
||||||
|
skew := int64(g.opts.Skew)
|
||||||
|
base := g.now().UTC().Unix() / period
|
||||||
|
for d := skew; d >= -skew; d-- {
|
||||||
|
step := base + d
|
||||||
|
if step < 0 {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
want, err := totp.GenerateCodeCustom(string(secret), time.Unix(step*period, 0).UTC(), g.opts)
|
||||||
|
if err != nil {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if subtle.ConstantTimeCompare([]byte(want), []byte(code)) == 1 {
|
||||||
|
return uint64(step), true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return 0, false
|
||||||
|
}
|
||||||
|
|
||||||
|
// acceptCounter records counter as the highest accepted for identityID and
|
||||||
|
// reports whether it is strictly newer than the last one (L5s).
|
||||||
|
func (g *Generator) acceptCounter(identityID int64, counter uint64) bool {
|
||||||
|
g.mu.Lock()
|
||||||
|
defer g.mu.Unlock()
|
||||||
|
if prev, ok := g.last[identityID]; ok && counter <= prev {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
g.last[identityID] = counter
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
// forget drops an identity's replay counter (a new secret, L5s).
|
||||||
|
func (g *Generator) forget(identityID int64) {
|
||||||
|
g.mu.Lock()
|
||||||
|
defer g.mu.Unlock()
|
||||||
|
delete(g.last, identityID)
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -80,7 +80,7 @@ func TestVerifyAcceptsAValidCodeAndMarksVerified(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestVerifyRejectsBadOrEmptyCode(t *testing.T) {
|
func TestVerifyRejectsABadOrEmptyCode(t *testing.T) {
|
||||||
st := newMapStore()
|
st := newMapStore()
|
||||||
svc := totp.New("MyDay", st)
|
svc := totp.New("MyDay", st)
|
||||||
if _, err := svc.Begin(context.Background(), 1, "guid"); err != nil {
|
if _, err := svc.Begin(context.Background(), 1, "guid"); err != nil {
|
||||||
|
|
@ -96,3 +96,55 @@ func TestVerifyRejectsBadOrEmptyCode(t *testing.T) {
|
||||||
t.Error("a bad code marked the secret verified")
|
t.Error("a bad code marked the secret verified")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestVerifyRejectsAReplay covers L5s: a code that already worked must not work
|
||||||
|
// again within the skew window.
|
||||||
|
func TestVerifyRejectsAReplay(t *testing.T) {
|
||||||
|
st := newMapStore()
|
||||||
|
svc := totp.New("MyDay", st)
|
||||||
|
ctx := context.Background()
|
||||||
|
if _, err := svc.Begin(ctx, 1, "guid"); err != nil {
|
||||||
|
t.Fatalf("Begin: %v", err)
|
||||||
|
}
|
||||||
|
now := time.Now()
|
||||||
|
svc.SetClock(func() time.Time { return now })
|
||||||
|
secret := string(st.secrets[1])
|
||||||
|
|
||||||
|
code, err := pqotp.GenerateCode(secret, now)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("GenerateCode: %v", err)
|
||||||
|
}
|
||||||
|
if err := svc.Verify(ctx, 1, code); err != nil {
|
||||||
|
t.Fatalf("first Verify = %v, want nil", err)
|
||||||
|
}
|
||||||
|
// The same code, still inside its window, is a replay (L5s).
|
||||||
|
if err := svc.Verify(ctx, 1, code); !errors.Is(err, totp.ErrInvalidCode) {
|
||||||
|
t.Errorf("replayed Verify = %v, want ErrInvalidCode (L5s)", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// A code from an EARLIER step is refused once a newer step was accepted.
|
||||||
|
newer, err := pqotp.GenerateCode(secret, now.Add(30*time.Second))
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("GenerateCode: %v", err)
|
||||||
|
}
|
||||||
|
svc.SetClock(func() time.Time { return now.Add(30 * time.Second) })
|
||||||
|
if err := svc.Verify(ctx, 1, newer); err != nil {
|
||||||
|
t.Fatalf("Verify(newer) = %v, want nil", err)
|
||||||
|
}
|
||||||
|
svc.SetClock(func() time.Time { return now }) // the skew window still covers step N
|
||||||
|
if err := svc.Verify(ctx, 1, code); !errors.Is(err, totp.ErrInvalidCode) {
|
||||||
|
t.Errorf("an older step after a newer one = %v, want ErrInvalidCode (L5s)", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// A fresh secret resets the counter (re-enrollment, L5s).
|
||||||
|
if _, err := svc.Begin(ctx, 1, "guid"); err != nil {
|
||||||
|
t.Fatalf("Begin (re-enroll): %v", err)
|
||||||
|
}
|
||||||
|
fresh, err := pqotp.GenerateCode(string(st.secrets[1]), now)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("GenerateCode: %v", err)
|
||||||
|
}
|
||||||
|
if err := svc.Verify(ctx, 1, fresh); err != nil {
|
||||||
|
t.Errorf("Verify after re-enrollment = %v, want nil", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue