carlos / contra Public

web: let the client mint the ids it binds, so a stored wrap can be opened

Found by running the real browser module against the real deployment, which
is the only place it could have been found: the Go tests built the request
struct by hand and so agreed with themselves.

The client seals each wrap under an AAD binding the account id and the wrap
id. The server was minting both itself. Every wrap it stored was therefore
bound to identifiers the client had never seen — a row that decrypts for
nobody, ever, silently, and only at the moment somebody tried to unlock.

So both ids come from the client now, like every other id in this protocol.
That is not laxness: they are bound into ciphertext before the server sees
any of it, so there is no other side of the wire they could come from. A
collision is refused by the primary key. It also removes the zero-filled
account-id placeholder signup was using, which was a weaker binding recorded
as a known gap — the gap is closed rather than deferred.

TestStoredWrapsStillOpen is the regression, and it asserts the property
rather than the plumbing: what comes back out of storage opens under the AAD
it went in with. An id minted on the wrong side of the wire fails it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago f63ef12802518eb31c9294afb2bd89a92ed0afaa parent fb3f054

7 files changed, +137 −49

internal/contra/auth/auth.go +16 −6
internal/contra/static/crypto/account.js +14 −12
internal/contra/store/store.go +19 −15
internal/contratest/auth_test.go +72 −10
internal/contratest/greps_test.go +2 −2
internal/contratest/harness_test.go +13 −3
internal/contratest/jsmodules_test.go +1 −1

Intent

full ledger: web/auth

diff --git a/internal/contra/auth/auth.go b/internal/contra/auth/auth.go

index 786236a..1adf3c1 100644

--- a/internal/contra/auth/auth.go

+++ b/internal/contra/auth/auth.go

@@ -155,14 +155,18 @@ type saltResponse struct {

}

type signupRequest struct {

- Email string `json:"email"`

- SaltHex string `json:"salt_hex"`

- LoginKeys []struct {

+ Email string `json:"email"`

+ // AccountIDHex is minted by the client, because the client binds it into

+ // the AAD of the wraps below before the server has seen any of them.

+ AccountIDHex string `json:"account_id_hex"`

+ SaltHex string `json:"salt_hex"`

+ LoginKeys []struct {

Branch string `json:"branch"`

PublicKeyHex string `json:"public_key_hex"`

} `json:"login_keys"`

Wraps []struct {

Kind string `json:"kind"`

+ WrapIDHex string `json:"wrap_id_hex"`

KDFParams string `json:"kdf_params"`

Version uint64 `json:"version"`

KCVHex string `json:"kcv_hex"`

@@ -253,6 +257,11 @@ func (a *Auth) Signup(w http.ResponseWriter, r *http.Request) {

writeJSON(w, http.StatusBadRequest, map[string]string{"error": "bad salt"})

return

}

+ accountID, err := hex.DecodeString(req.AccountIDHex)

+ if err != nil || len(accountID) != 16 {

+ writeJSON(w, http.StatusBadRequest, map[string]string{"error": "bad account id"})

+ return

+ }

for _, wr := range req.Wraps {

if err := CheckParams(wr.KDFParams); err != nil {

writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()})

@@ -273,7 +282,7 @@ func (a *Auth) Signup(w http.ResponseWriter, r *http.Request) {

return

}

- acct, err := a.store.CreateAccount(ctx, index, salt)

+ acct, err := a.store.CreateAccount(ctx, accountID, index, salt)

if err != nil {

writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "could not create"})

return

@@ -298,11 +307,12 @@ func (a *Auth) Signup(w http.ResponseWriter, r *http.Request) {

nonce, e2 := hex.DecodeString(wr.NonceHex)

ct, e3 := hex.DecodeString(wr.CiphertextHex)

credSalt, e4 := hex.DecodeString(wr.CredSaltHex)

- if e1 != nil || e2 != nil || e3 != nil || e4 != nil {

+ wrapID, e5 := hex.DecodeString(wr.WrapIDHex)

+ if e1 != nil || e2 != nil || e3 != nil || e4 != nil || e5 != nil {

writeJSON(w, http.StatusBadRequest, map[string]string{"error": "bad wrap"})

return

}

- if err := a.store.PutWrap(ctx, acct.ID, wr.Kind, wr.KDFParams, wr.Version,

+ if err := a.store.PutWrap(ctx, acct.ID, wrapID, wr.Kind, wr.KDFParams, wr.Version,

kcv, credSalt, store.Sealed{Nonce: nonce, Ciphertext: ct}); err != nil {

writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()})

return

diff --git a/internal/contra/static/crypto/account.js b/internal/contra/static/crypto/account.js

index c7f112c..bbe3195 100644

--- a/internal/contra/static/crypto/account.js

+++ b/internal/contra/static/crypto/account.js

@@ -30,11 +30,14 @@ async function postJSON(path, body) {

// A wrap row as the server stores it: a format version, the KDF parameters

// (authenticated as AAD), a KCV, a 96-bit nonce and the ciphertext.

-async function makeWrap(kind, kek, rk, accountIdPlaceholder, kdfParams, credSalt) {

+//

+// The wrap id is minted here and sent, because it is bound into the AAD

+// before the server sees the ciphertext. A server that minted its own would

+// store a row whose binding never matches — a wrap that can never be opened.

+async function makeWrap(kind, kek, rk, accountId, kdfParams, credSalt) {

const wrapId = randomBytes(16);

const nonce = randomBytes(12);

- const ciphertext = await seal(

- kek, nonce, aad(accountIdPlaceholder, kind, wrapId, 1), rk);

+ const ciphertext = await seal(kek, nonce, aad(accountId, kind, wrapId, 1), rk);

return {

kind,

kdf_params: kdfParams,

@@ -87,21 +90,20 @@ export async function signup(email, passphrase, onProgress = () => {}) {

const lkRC = await deriveP256(seedRC);

const kekRC = await hkdf(recoveryCode, null, LABEL_RC_KEK, 32);

- // The AAD binds the account id, which the server has not minted yet. A

- // signup-time placeholder of zeroes keeps the two sides agreeing; when the

- // account id becomes known the wraps are rewritten under it. Recorded here

- // rather than hidden: this is the one place the binding is weaker than it

- // will be, and web/4 closes it by minting the id client-side.

- const placeholder = new Uint8Array(16);

+ // The account id is minted here, like every other id in this protocol, so

+ // that the AAD binding is real from the first byte rather than filled in

+ // later. The server refuses a collision through the primary key.

+ const accountId = randomBytes(16);

onProgress("sealing your vault key");

const wraps = [

- await makeWrap(OBJECT_WRAP_PW, kekPW, rk, placeholder, kdfParams, null),

- await makeWrap(OBJECT_WRAP_RC, kekRC, rk, placeholder, kdfParams, null),

+ await makeWrap(OBJECT_WRAP_PW, kekPW, rk, accountId, kdfParams, null),

+ await makeWrap(OBJECT_WRAP_RC, kekRC, rk, accountId, kdfParams, null),

];

await postJSON("/auth/signup", {

email,

+ account_id_hex: toHex(accountId),

salt_hex: toHex(salt),

login_keys: [

{ branch: "pw", public_key_hex: toHex(lkPW.publicKey) },

@@ -110,7 +112,7 @@ export async function signup(email, passphrase, onProgress = () => {}) {

wraps,

});

- return { accountKey, recoveryCode, rk };

+ return { accountId, accountKey, recoveryCode, rk };

}

// signIn runs the ceremony: ask for a challenge, stretch, derive, sign.

diff --git a/internal/contra/store/store.go b/internal/contra/store/store.go

index 9752e50..0e59374 100644

--- a/internal/contra/store/store.go

+++ b/internal/contra/store/store.go

@@ -74,16 +74,18 @@ type Sealed struct {

func (s Sealed) valid() bool { return len(s.Nonce) == 12 && len(s.Ciphertext) > 16 }

// CreateAccount records an identity. emailIndex is the blind index —

-// HMAC-SHA-256 under the server-held pepper — and this function neither

-// takes nor could compute an address.

-func (s *Store) CreateAccount(ctx context.Context, emailIndex, kdfSalt []byte) (*model.Account, error) {

- if len(emailIndex) != 32 || len(kdfSalt) != 16 {

+// HMAC-SHA-256 under the server-held pepper — and this function neither takes

+// nor could compute an address.

+//

+// The id comes from the client, like every other id in this protocol. That is

+// not laxness: the client binds the account id into the AAD of the wraps it

+// seals before the server has seen any of them, so a server-minted id would

+// produce wraps whose binding never matches and which therefore never open. A

+// collision is refused by the primary key.

+func (s *Store) CreateAccount(ctx context.Context, id, emailIndex, kdfSalt []byte) (*model.Account, error) {

+ if len(id) != 16 || len(emailIndex) != 32 || len(kdfSalt) != 16 {

return nil, ErrShortField

}

- id, err := NewID()

- if err != nil {

- return nil, err

- }

a := &model.Account{

ID: id,

EmailIndex: emailIndex,

@@ -114,21 +116,23 @@ func (s *Store) PutLoginKey(ctx context.Context, accountID []byte, branch string

// PutWrap stores one peer wrap of the root key.

+// wrapID is the client's, for the same reason the account id is: it is bound

+// into the AAD before the server sees the ciphertext, so minting a different

+// one here would store a row that can never be opened. This is exactly the

+// bug that a Go test building the request struct by hand does not catch and

+// the real client does.

+//

// kdfParams is canonical JSON, authenticated as AAD by the client, and kept

// so parameters can be raised later with a transparent re-wrap — and so a

// client can see and refuse a set below its floor. credSalt is the passkey

// credential's non-secret 32 bytes, nil on the other two branches.

-func (s *Store) PutWrap(ctx context.Context, accountID []byte, kind, kdfParams string,

+func (s *Store) PutWrap(ctx context.Context, accountID, wrapID []byte, kind, kdfParams string,

version uint64, kcv, credSalt []byte, sealed Sealed) error {

- if !sealed.valid() || len(kcv) != 16 {

+ if !sealed.valid() || len(kcv) != 16 || len(wrapID) != 16 {

return ErrShortField

}

- id, err := NewID()

- if err != nil {

- return err

- }

return s.db.WithContext(ctx).Create(&model.Wrap{

- ID: id,

+ ID: wrapID,

AccountID: accountID,

Kind: kind,

FormatVersion: FormatVersion,

diff --git a/internal/contratest/auth_test.go b/internal/contratest/auth_test.go

index fcee8f1..a5a41f5 100644

--- a/internal/contratest/auth_test.go

+++ b/internal/contratest/auth_test.go

@@ -15,6 +15,7 @@ import (

"github.com/carlosframework/contra/internal/contra/auth"

"github.com/carlosframework/contra/internal/contra/crypto"

+ "github.com/carlosframework/contra/internal/contra/model"

)

// The signup and sign-in ceremonies, driven the way a browser drives them.

@@ -30,10 +31,20 @@ import (

type account struct {

email string

passphrase string

+ id []byte

salt []byte

accountKey []byte

recovery []byte

rk []byte

+ // wraps records what the client sealed, so a test can prove the row that

+ // comes back out of storage still opens under the AAD it went in with.

+ wraps map[string]sealedWrap

+type sealedWrap struct {

+ id []byte

+ kek []byte

+ nonce []byte

}

func postJSON(t *testing.T, h http.Handler, path string, body any) *httptest.ResponseRecorder {

@@ -62,8 +73,8 @@ func decode(t *testing.T, rec *httptest.ResponseRecorder, v any) {

func doSignup(t *testing.T, h http.Handler, email, passphrase string) account {

t.Helper()

- a := account{email: email, passphrase: passphrase}

- for _, dst := range []*[]byte{&a.salt, &a.accountKey, &a.recovery} {

+ a := account{email: email, passphrase: passphrase, wraps: map[string]sealedWrap{}}

+ for _, dst := range []*[]byte{&a.id, &a.salt, &a.accountKey, &a.recovery} {

b := make([]byte, 16)

if _, err := rand.Read(b); err != nil {

t.Fatal(err)

@@ -104,6 +115,7 @@ func doSignup(t *testing.T, h http.Handler, email, passphrase string) account {

type wrapJSON struct {

Kind string `json:"kind"`

+ WrapIDHex string `json:"wrap_id_hex"`

KDFParams string `json:"kdf_params"`

Version uint64 `json:"version"`

KCVHex string `json:"kcv_hex"`

@@ -111,7 +123,6 @@ func doSignup(t *testing.T, h http.Handler, email, passphrase string) account {

NonceHex string `json:"nonce_hex"`

CiphertextHex string `json:"ciphertext_hex"`

}

- placeholder := make([]byte, 16)

seal := func(kind string, kek []byte) wrapJSON {

wrapID := make([]byte, 16)

nonce := make([]byte, 12)

@@ -121,20 +132,25 @@ func doSignup(t *testing.T, h http.Handler, email, passphrase string) account {

if _, err := rand.Read(nonce); err != nil {

t.Fatal(err)

}

- ct, err := crypto.Seal(kek, nonce, crypto.AAD(placeholder, kind, wrapID, 1), a.rk)

+ // The account id and the wrap id are both the client's, and both are

+ // bound into the AAD here — before the server has seen any of it.

+ ct, err := crypto.Seal(kek, nonce, crypto.AAD(a.id, kind, wrapID, 1), a.rk)

if err != nil {

t.Fatal(err)

}

+ a.wraps[kind] = sealedWrap{id: wrapID, kek: kek, nonce: nonce}

return wrapJSON{

- Kind: kind, KDFParams: auth.DefaultKDFParams, Version: 1,

+ Kind: kind, WrapIDHex: hex.EncodeToString(wrapID),

+ KDFParams: auth.DefaultKDFParams, Version: 1,

KCVHex: hex.EncodeToString(crypto.KCV(kek)),

NonceHex: hex.EncodeToString(nonce), CiphertextHex: hex.EncodeToString(ct),

}

rec := postJSON(t, h, "/auth/signup", map[string]any{

- "email": email,

- "salt_hex": hex.EncodeToString(a.salt),

+ "email": email,

+ "account_id_hex": hex.EncodeToString(a.id),

+ "salt_hex": hex.EncodeToString(a.salt),

"login_keys": []map[string]string{

{"branch": "pw", "public_key_hex": hex.EncodeToString(lkPW.PublicKey().Bytes())},

{"branch": "rc", "public_key_hex": hex.EncodeToString(lkRC.PublicKey().Bytes())},

@@ -354,11 +370,13 @@ func TestServerRefusesDowngradedKDFParameters(t *testing.T) {

// And the endpoint refuses them, not just the helper.

h := newApp(t)

rec := postJSON(t, h, "/auth/signup", map[string]any{

- "email": "weak@example.invalid",

- "salt_hex": hex.EncodeToString(make([]byte, 16)),

- "login_keys": []map[string]string{},

+ "email": "weak@example.invalid",

+ "account_id_hex": hex.EncodeToString(make([]byte, 16)),

+ "salt_hex": hex.EncodeToString(make([]byte, 16)),

+ "login_keys": []map[string]string{},

"wraps": []map[string]any{{

"kind": crypto.ObjectWrapPW,

+ "wrap_id_hex": hex.EncodeToString(make([]byte, 16)),

"kdf_params": `{"alg":"argon2id","m_kib":8,"t":1,"p":1}`,

"version": 1,

"kcv_hex": hex.EncodeToString(make([]byte, 16)),

@@ -412,3 +430,47 @@ func TestAuthRefusesAWeakPepper(t *testing.T) {

}

+// The bug this test exists for: the server minted its own wrap id while the

+// client had already bound *its* id into the AAD, so every wrap it stored was

+// one no client could ever open.

+//

+// Nothing caught it, because the Go tests built the request struct by hand and

+// so agreed with themselves. It surfaced the first time the real browser

+// module talked to the real server. This asserts the property rather than the

+// plumbing: what comes back out of storage still opens under the AAD it went

+// in with, so an id minted on the wrong side of the wire fails here.

+func TestStoredWrapsStillOpen(t *testing.T) {

+ h, d := newAppWithDB(t)

+ a := doSignup(t, h, "vector-person@example.invalid", "a generated test passphrase 8f3a1c")

+ if len(a.wraps) == 0 {

+ t.Fatal("the ceremony sealed no wraps, so this test is checking nothing")

+ }

+ for kind, w := range a.wraps {

+ var row model.Wrap

+ if err := d.Where("kind = ?", kind).First(&row).Error; err != nil {

+ t.Errorf("%s: not stored: %v", kind, err)

+ continue

+ }

+ if !bytes.Equal(row.ID, w.id) {

+ t.Errorf("%s: stored id %x, but the client sealed against %x — this "+

+ "wrap can never be opened", kind, row.ID, w.id)

+ continue

+ }

+ if !bytes.Equal(row.AccountID, a.id) {

+ t.Errorf("%s: stored account id %x, client sealed against %x",

+ kind, row.AccountID, a.id)

+ continue

+ }

+ got, err := crypto.Open(w.kek, row.Nonce,

+ crypto.AAD(row.AccountID, kind, row.ID, row.Version), row.Ciphertext)

+ if err != nil {

+ t.Errorf("%s: the stored wrap does not open: %v", kind, err)

+ continue

+ }

+ if !bytes.Equal(got, a.rk) {

+ t.Errorf("%s: opened to something other than RK", kind)

+ }

diff --git a/internal/contratest/greps_test.go b/internal/contratest/greps_test.go

index abe4ae7..0375104 100644

--- a/internal/contratest/greps_test.go

+++ b/internal/contratest/greps_test.go

@@ -130,7 +130,7 @@ func writeEverything(t *testing.T, s *store.Store) secrets {

mac.Write([]byte(canaries["email address"]))

emailIndex := mac.Sum(nil)

- acct, err := s.CreateAccount(ctx, emailIndex, randBytes(t, 16))

+ acct, err := s.CreateAccount(ctx, randBytes(t, 16), emailIndex, randBytes(t, 16))

if err != nil {

t.Fatalf("CreateAccount: %v", err)

}

@@ -168,7 +168,7 @@ func writeEverything(t *testing.T, s *store.Store) secrets {

if err != nil {

t.Fatalf("Seal(%s): %v", w.kind, err)

}

- if err := s.PutWrap(ctx, acct.ID, w.kind, params, 1, crypto.KCV(w.kek),

+ if err := s.PutWrap(ctx, acct.ID, wrapID, w.kind, params, 1, crypto.KCV(w.kek),

w.credSalt, store.Sealed{Nonce: nonce, Ciphertext: ct}); err != nil {

t.Fatalf("PutWrap(%s): %v", w.kind, err)

}

diff --git a/internal/contratest/harness_test.go b/internal/contratest/harness_test.go

index de1296a..f6cebff 100644

--- a/internal/contratest/harness_test.go

+++ b/internal/contratest/harness_test.go

@@ -16,6 +16,8 @@ import (

"strings"

"testing"

+ "gorm.io/gorm"

"github.com/carlosframework/rastrillo/db"

contra "github.com/carlosframework/contra/internal/contra"

@@ -34,9 +36,17 @@ func testConfig() contra.Config {

return contra.Config{Origin: testOrigin, Pepper: testPepper}

}

-// newApp builds the whole app per test over a fresh temp database,

-// exactly as main.go does.

+// newApp builds the whole app per test over a fresh temp database, exactly as

+// main.go does.

func newApp(t *testing.T) http.Handler {

+ t.Helper()

+ h, _ := newAppWithDB(t)

+ return h

+// newAppWithDB is newApp plus the handle, for the tests that need to look at

+// the row that was actually stored rather than at what a handler said.

+func newAppWithDB(t *testing.T) (http.Handler, *gorm.DB) {

t.Helper()

logger := slog.New(slog.NewTextHandler(io.Discard, nil))

d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger)

@@ -48,7 +58,7 @@ func newApp(t *testing.T) http.Handler {

if err != nil {

t.Fatalf("App: %v", err)

}

- return mux

+ return mux, d.G

}

func get(t *testing.T, h http.Handler, target string) *httptest.ResponseRecorder {

diff --git a/internal/contratest/jsmodules_test.go b/internal/contratest/jsmodules_test.go

index e36ca00..e99e286 100644

--- a/internal/contratest/jsmodules_test.go

+++ b/internal/contratest/jsmodules_test.go

@@ -25,7 +25,7 @@ const jsModuleCap = 300

// cap — a new module in the crypto path is exactly the thing that should not

// arrive without somebody noticing.

var jsHighWater = map[string]int{

- "account.js": 150,

+ "account.js": 152,

"argon2id.js": 292,

"blake2b.js": 99,

"bytes.js": 61,