amadan

eleven / messenger Public

server: harden push-unsubscribe and key-grant authorization (#97)

Three related authz gaps from the pre-0.0.1 audit:

- unsubscribePush was unauthenticated and deleted by endpoint alone, so anyone
  who learned an endpoint could silently unsubscribe (deny notifications to) that
  device. It now requires a valid participant token and removes only that token's
  own subscription (deleteOwnSubscription); the web client sends its token.
- grantInstanceKey used ON CONFLICT DO UPDATE, so any member could overwrite
  another member's wrapped instance key with junk and brick their avatar
  decryption. Now DO NOTHING — a grant only ever fills a missing holder.
- addThreadMember used INSERT OR REPLACE, so any member could clobber another
  member's wrapped thread key and lock them out of a thread. Now DO NOTHING.
  Legitimate re-keying (invite adoption, adoptUserInvite) is a separate statement
  and is unaffected, as are the new-thread inserts.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell a7cfa1166e40dfa91061427853635be476bbe49f parent 209504b
5 files changed, +122 −6
  • authz_hardening_test.go +86 −0
  • db.go +12 −3
  • main.go +13 −2
  • push.go +10 −0
  • web/static/push.js +1 −1
diff --git a/authz_hardening_test.go b/authz_hardening_test.go
new file mode 100644
index 0000000..bc69347
--- /dev/null
+++ b/authz_hardening_test.go
@@ -0,0 +1,86 @@
+package main
+
+import (
+ "fmt"
+ "net/http"
+ "strings"
+ "testing"
+)
+
+// A push subscription may only be removed by its own token — not by anyone who
+// happens to know the endpoint.
+func TestUnsubscribePushIsOwnerScoped(t *testing.T) {
+ ts, srv := newTestServer(t)
+ a, _ := srv.db.createInvite("A")
+ b, _ := srv.db.createInvite("B")
+ if err := srv.db.addSubscription(PushSub{Endpoint: "https://e/1", Token: a.Token, P256dh: "p", Auth: "a"}); err != nil {
+ t.Fatalf("addSubscription: %v", err)
+ }
+
+ post := func(body string) int {
+ resp, err := http.Post(ts.URL+"/api/push/unsubscribe", "application/json", strings.NewReader(body))
+ if err != nil {
+ t.Fatal(err)
+ }
+ resp.Body.Close()
+ return resp.StatusCode
+ }
+ subCount := func() int { subs, _ := srv.db.listSubscriptions(); return len(subs) }
+
+ // No token → refused, nothing deleted.
+ if got := post(`{"endpoint":"https://e/1"}`); got != http.StatusForbidden {
+ t.Fatalf("no token: want 403, got %d", got)
+ }
+ if subCount() != 1 {
+ t.Fatalf("no-token request deleted a sub")
+ }
+ // Valid but non-owning token → accepted, but the other user's sub survives.
+ if got := post(fmt.Sprintf(`{"token":%q,"endpoint":"https://e/1"}`, b.Token)); got != http.StatusOK {
+ t.Fatalf("non-owner: want 200, got %d", got)
+ }
+ if subCount() != 1 {
+ t.Fatalf("non-owner deleted someone else's subscription")
+ }
+ // Owner token → removed.
+ if got := post(fmt.Sprintf(`{"token":%q,"endpoint":"https://e/1"}`, a.Token)); got != http.StatusOK {
+ t.Fatalf("owner: want 200, got %d", got)
+ }
+ if subCount() != 0 {
+ t.Fatalf("owner unsubscribe left the sub")
+ }
+}
+
+// A second grant must not overwrite a member's existing wrapped instance key.
+func TestGrantInstanceKeyNeverOverwrites(t *testing.T) {
+ db := newTestDB(t)
+ if err := db.grantInstanceKey("u1", "GOOD"); err != nil {
+ t.Fatalf("grant: %v", err)
+ }
+ if err := db.grantInstanceKey("u1", "JUNK"); err != nil {
+ t.Fatalf("re-grant: %v", err)
+ }
+ got, _, _ := db.instanceKeyFor("u1")
+ if got != "GOOD" {
+ t.Fatalf("instance key overwritten: got %q, want GOOD", got)
+ }
+}
+
+// Re-adding an existing member must not clobber their wrapped thread key.
+func TestAddThreadMemberNeverClobbers(t *testing.T) {
+ db := newTestDB(t)
+ if err := db.addThreadMember("t1", "m1", "GOOD"); err != nil {
+ t.Fatalf("add: %v", err)
+ }
+ if err := db.addThreadMember("t1", "m1", "JUNK"); err != nil {
+ t.Fatalf("re-add: %v", err)
+ }
+ var wrapped string
+ if err := db.sql.QueryRow(
+ "SELECT wrapped FROM thread_keys WHERE thread_id = ? AND user_id = ?", "t1", "m1",
+ ).Scan(&wrapped); err != nil {
+ t.Fatalf("read thread key: %v", err)
+ }
+ if wrapped != "GOOD" {
+ t.Fatalf("thread key clobbered: got %q, want GOOD", wrapped)
+ }
+}
diff --git a/db.go b/db.go
index 5d8b048..57aaa08 100644
--- a/db.go
+++ b/db.go
@@ -1105,9 +1105,14 @@ func (db *DB) instanceKeyHolders() ([]string, error) {
return out, rows.Err()
}
+// grantInstanceKey wraps the instance key to a member who doesn't yet hold it.
+// DO NOTHING (not overwrite): any member may call this for any user id, so
+// overwriting would let one member replace another's good wrapped key with junk
+// and brick their avatar decryption. Grants only ever fill a missing holder;
+// there is no re-key path through here.
func (db *DB) grantInstanceKey(userID, wrapped string) error {
_, err := db.sql.Exec(
- "INSERT INTO instance_keys(user_id, wrapped) VALUES(?, ?) ON CONFLICT(user_id) DO UPDATE SET wrapped = excluded.wrapped",
+ "INSERT INTO instance_keys(user_id, wrapped) VALUES(?, ?) ON CONFLICT(user_id) DO NOTHING",
userID, wrapped,
)
return err
@@ -1174,7 +1179,11 @@ func (db *DB) createThread(kind, nameBlob string, members []ThreadMemberKey) (st
return id, tx.Commit()
}
-// addThreadMember adds (or re-keys) one member of an existing thread.
+// addThreadMember adds one member of an existing thread. The thread_keys insert
+// is DO NOTHING, never REPLACE: any member may add another, so replacing would
+// let one member overwrite an existing member's good wrapped key with junk and
+// lock them out of the thread. Legitimate re-keying (invite adoption) is a
+// separate, explicit path (see adoptUserInvite).
func (db *DB) addThreadMember(threadID, userID, wrapped string) error {
tx, err := db.sql.Begin()
if err != nil {
@@ -1185,7 +1194,7 @@ func (db *DB) addThreadMember(threadID, userID, wrapped string) error {
if _, err := tx.Exec("INSERT OR IGNORE INTO thread_members(thread_id, user_id, created_at) VALUES(?, ?, ?)", threadID, userID, now); err != nil {
return err
}
- if _, err := tx.Exec("INSERT OR REPLACE INTO thread_keys(thread_id, user_id, wrapped) VALUES(?, ?, ?)", threadID, userID, wrapped); err != nil {
+ if _, err := tx.Exec("INSERT INTO thread_keys(thread_id, user_id, wrapped) VALUES(?, ?, ?) ON CONFLICT(thread_id, user_id) DO NOTHING", threadID, userID, wrapped); err != nil {
return err
}
return tx.Commit()
diff --git a/main.go b/main.go
index 795a17a..6b9e17f 100644
--- a/main.go
+++ b/main.go
@@ -1726,11 +1726,22 @@ func (s *Server) subscribePush(w http.ResponseWriter, r *http.Request) {
func (s *Server) unsubscribePush(w http.ResponseWriter, r *http.Request) {
var body struct {
+ Token string `json:"token"`
Endpoint string `json:"endpoint"`
}
- json.NewDecoder(r.Body).Decode(&body)
+ if json.NewDecoder(r.Body).Decode(&body) != nil {
+ httpError(w, http.StatusBadRequest, "bad request")
+ return
+ }
+ // Require a valid participant token and only remove that token's own
+ // subscription — otherwise anyone who learned an endpoint could silently
+ // unsubscribe (deny notifications to) someone else's device.
+ if !s.tokenOK(body.Token) {
+ httpError(w, http.StatusForbidden, "not a member")
+ return
+ }
if body.Endpoint != "" {
- _ = s.db.deleteSubscription(body.Endpoint)
+ _ = s.db.deleteOwnSubscription(body.Endpoint, body.Token)
}
writeJSON(w, http.StatusOK, map[string]any{"ok": true})
}
diff --git a/push.go b/push.go
index 29ab691..64c66b8 100644
--- a/push.go
+++ b/push.go
@@ -114,6 +114,16 @@ func (db *DB) deleteSubscription(endpoint string) error {
return err
}
+// deleteOwnSubscription removes a subscription only if it belongs to the given
+// token, so knowing someone else's endpoint isn't enough to silence their device.
+func (db *DB) deleteOwnSubscription(endpoint, token string) error {
+ _, err := db.sql.Exec(
+ "DELETE FROM push_subscriptions WHERE endpoint = ? AND token_hash = ?",
+ endpoint, hashToken(token),
+ )
+ return err
+}
+
func (db *DB) deleteSubscriptionsForToken(token string) error {
_, err := db.sql.Exec("DELETE FROM push_subscriptions WHERE token_hash = ?", hashToken(token))
return err
diff --git a/web/static/push.js b/web/static/push.js
index eb5e60b..e9cd25e 100644
--- a/web/static/push.js
+++ b/web/static/push.js
@@ -153,7 +153,7 @@ export async function unsubscribePush() {
await fetch("/api/push/unsubscribe", {
method: "POST",
headers: { "Content-Type": "application/json" },
- body: JSON.stringify({ endpoint: sub.endpoint }),
+ body: JSON.stringify({ token: session.token, endpoint: sub.endpoint }),
});
await sub.unsubscribe();
} catch (e) {