protondrive: verify your own address in any letter case with your own keys
The check that the drive is shared and signed by your own address ignored letter case, but keys were looked up by exact address, so a server naming your address in capitals could serve keys of its own for it. Addresses are now matched ignoring ASCII case only, everywhere, and your own address in any such case is always verified with your own keys, never ones the server supplies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
c28077174e
commit
15bc60ead9
9 files changed
+240
-38
No files matched your search
@@ -5,7 +5,6 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/url"
|
||||
"strings"
|
||||
"sync"
|
||||
|
||||
"github.com/ProtonMail/gopenpgp/v3/crypto"
|
||||
@@ -32,7 +31,7 @@ type account struct {
|
||||
addresses []*address // the primary address is first
|
||||
|
||||
mu sync.Mutex
|
||||
foreign map[string]*publicKeyLookup // public keys of other users by email
|
||||
foreign map[string]*publicKeyLookup // public keys of other users by emailKey
|
||||
}
|
||||
|
||||
// publicKeyLookup is a fetch of another user's public keys, done when
|
||||
@@ -231,10 +230,26 @@ func (a *account) byID(id string) *address {
|
||||
return nil
|
||||
}
|
||||
|
||||
// byEmail returns the address email, or nil.
|
||||
// emailKey returns email with ASCII letters lowercased, which is how
|
||||
// emails are compared: Proton treats addresses case-insensitively, but
|
||||
// Unicode case folding would let a non-ASCII letter, such as the Kelvin
|
||||
// sign, stand for an ASCII one.
|
||||
func emailKey(email string) string {
|
||||
b := []byte(email)
|
||||
for i, c := range b {
|
||||
if 'A' <= c && c <= 'Z' {
|
||||
b[i] = c + 'a' - 'A'
|
||||
}
|
||||
}
|
||||
return string(b)
|
||||
}
|
||||
|
||||
// byEmail returns the address email, compared as emailKey does, or
|
||||
// nil.
|
||||
func (a *account) byEmail(email string) *address {
|
||||
key := emailKey(email)
|
||||
for _, addr := range a.addresses {
|
||||
if addr.Email == email {
|
||||
if emailKey(addr.Email) == key {
|
||||
return addr
|
||||
}
|
||||
}
|
||||
@@ -242,14 +257,9 @@ func (a *account) byEmail(email string) *address {
|
||||
}
|
||||
|
||||
// owns reports whether email is one of the account's addresses whose
|
||||
// keys it holds, ignoring case.
|
||||
// keys it holds, compared as emailKey does.
|
||||
func (a *account) owns(email string) bool {
|
||||
for _, addr := range a.addresses {
|
||||
if strings.EqualFold(addr.Email, email) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
return a.byEmail(email) != nil
|
||||
}
|
||||
|
||||
// apiCaller makes an API call with the semantics of session.call,
|
||||
@@ -260,24 +270,26 @@ type apiCaller func(ctx context.Context, opts *rest.Opts, request, response any)
|
||||
// publicKeys returns the public keys of the address email, which the
|
||||
// caller must not modify.
|
||||
//
|
||||
// The keys of the user's own addresses come from the account. Other
|
||||
// users' keys are fetched once and kept for the life of the account;
|
||||
// an email without a Proton address has an empty key ring.
|
||||
// The keys of the user's own addresses, in any ASCII case, come from
|
||||
// the account, never the server. Other users' keys are fetched once
|
||||
// per emailKey and kept for the life of the account; an email without
|
||||
// a Proton address has an empty key ring.
|
||||
func (a *account) publicKeys(ctx context.Context, call apiCaller, email string) (*crypto.KeyRing, error) {
|
||||
if addr := a.byEmail(email); addr != nil {
|
||||
return publicKeyRing(addr.keyRing)
|
||||
}
|
||||
key := emailKey(email)
|
||||
a.mu.Lock()
|
||||
l := a.foreign[email]
|
||||
l := a.foreign[key]
|
||||
if l == nil {
|
||||
l = &publicKeyLookup{done: make(chan struct{})}
|
||||
a.foreign[email] = l
|
||||
a.foreign[key] = l
|
||||
a.mu.Unlock()
|
||||
l.keyRing, l.err = fetchPublicKeys(ctx, call, email)
|
||||
if l.err != nil {
|
||||
// Let the next caller try again.
|
||||
a.mu.Lock()
|
||||
delete(a.foreign, email)
|
||||
delete(a.foreign, key)
|
||||
a.mu.Unlock()
|
||||
}
|
||||
close(l.done)
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -252,6 +253,15 @@ func TestPublicKeysCached(t *testing.T) {
|
||||
assert.Zero(t, kr.CountEntities())
|
||||
}
|
||||
assert.Equal(t, 1, srv.Requests()[route])
|
||||
|
||||
// Case variants share a lookup, and an own address in any case
|
||||
// never reaches the server.
|
||||
srv.ResetRequests()
|
||||
for _, email := range []string{"OTHER@proton.test", "Other@Proton.Test", strings.ToUpper(u.Email())} {
|
||||
_, err := a.publicKeys(context.Background(), sessionCaller(s), email)
|
||||
require.NoError(t, err)
|
||||
}
|
||||
assert.Zero(t, srv.Requests()[route])
|
||||
}
|
||||
|
||||
func TestPublicKeysErrorNotCached(t *testing.T) {
|
||||
@@ -271,3 +281,20 @@ func TestPublicKeysErrorNotCached(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, 1, kr.CountEntities())
|
||||
}
|
||||
|
||||
func TestSameEmail(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
a, b string
|
||||
same bool
|
||||
}{
|
||||
{"user@proton.me", "user@proton.me", true},
|
||||
{"User@Proton.Me", "uSER@pROTON.mE", true},
|
||||
{"user@proton.me", "user@proton.mee", false},
|
||||
// Unicode case folding would match these to ASCII letters.
|
||||
{"kim@proton.me", "\u212aim@proton.me", false},
|
||||
{"sam@proton.me", "\u017fam@proton.me", false},
|
||||
{"élan@proton.me", "Élan@proton.me", false},
|
||||
} {
|
||||
assert.Equal(t, tc.same, emailKey(tc.a) == emailKey(tc.b), "%q %q", tc.a, tc.b)
|
||||
}
|
||||
}
|
||||
@@ -309,10 +309,14 @@ func (s *Server) handlePublicKeys(w http.ResponseWriter, r *http.Request, _ *Use
|
||||
}
|
||||
s.mu.Lock()
|
||||
var addr *address
|
||||
if owner := s.keysAs[email]; owner != nil {
|
||||
addr = owner.addresses[0]
|
||||
}
|
||||
for _, u := range s.users {
|
||||
if addr = u.addressByEmailLocked(email); addr != nil {
|
||||
if addr != nil {
|
||||
break
|
||||
}
|
||||
addr = u.addressByEmailLocked(email)
|
||||
}
|
||||
var keys []*addressKey
|
||||
if addr != nil {
|
||||
|
||||
@@ -486,8 +486,9 @@ type seedSpec struct {
|
||||
mediaType string
|
||||
legacy bool // v3 PKESK and SEIPDv1 instead of v6 PKESK and SEIPDv2
|
||||
noBlocks bool
|
||||
signer *User // signs as this user's primary address if set
|
||||
anonymous bool // signed as an anonymous uploader: no signature emails
|
||||
signer *User // signs as this user's primary address if set
|
||||
signEmail string // with signer, the signature email; signer's address if empty
|
||||
anonymous bool // signed as an anonymous uploader: no signature emails
|
||||
|
||||
// contentKeySigner, if set, replaces the content key packet
|
||||
// signature: an address email of the user, or "" for none.
|
||||
@@ -522,6 +523,26 @@ func (u *User) SeedForgedFolder(parentID, name string) (id string) {
|
||||
return u.seedFolder(u.view(), parentID, name, generateKey("impostor", "impostor@proton.test"), "")
|
||||
}
|
||||
|
||||
// SeedFolderSignedAs is SeedFolder signed by signer's primary address
|
||||
// key under the signature email email, which needn't be signer's.
|
||||
func (u *User) SeedFolderSignedAs(signer *User, email, parentID, name string) (id string) {
|
||||
signKey, _ := signer.primaryAddressKey()
|
||||
return u.seedFolder(u.view(), parentID, name, signKey, email)
|
||||
}
|
||||
|
||||
// SeedFileSignedAs is SeedFileSignedBy under the signature email email,
|
||||
// which needn't be signer's.
|
||||
func (u *User) SeedFileSignedAs(signer *User, email, parentID, name string, content []byte) (id string) {
|
||||
return u.seedFile(parentID, seedSpec{
|
||||
name: name,
|
||||
content: content,
|
||||
modTime: time.Now().UTC().Format("2006-01-02T15:04:05.000Z"),
|
||||
mediaType: mediaTypeOf(name),
|
||||
signer: signer,
|
||||
signEmail: email,
|
||||
})
|
||||
}
|
||||
|
||||
// seedFolder creates folder name in parentID with its name and node
|
||||
// passphrase signed by signer as email, and returns its ID.
|
||||
func (u *User) seedFolder(v *driveView, parentID, name string, signer *crypto.Key, email string) string {
|
||||
@@ -656,6 +677,9 @@ func (u *User) seedFile(parentID string, spec seedSpec) string {
|
||||
signKey, signEmail := v.addrKey, v.email
|
||||
if spec.signer != nil {
|
||||
signKey, signEmail = spec.signer.primaryAddressKey()
|
||||
if spec.signEmail != "" {
|
||||
signEmail = spec.signEmail
|
||||
}
|
||||
}
|
||||
node := newDriveKey(!spec.legacy)
|
||||
u.srv.rememberKey(node)
|
||||
@@ -754,15 +778,15 @@ func (u *User) SeedRevision(id string, content []byte, modTime time.Time) (revis
|
||||
return rev.id
|
||||
}
|
||||
|
||||
// ForgeShareCreator makes u's main share look created by signer,
|
||||
// another user, as a hostile server could serve it: the creator email
|
||||
// is signer's primary address and the share passphrase is signed by
|
||||
// signer's key. It still decrypts with u's address key.
|
||||
func (u *User) ForgeShareCreator(signer *User) {
|
||||
// ForgeShareCreator makes u's main share look created by email, signed
|
||||
// by signer's key, as a hostile server could serve it: the creator
|
||||
// email is email and the share passphrase is signed by signer's primary
|
||||
// address key. It still decrypts with u's address key.
|
||||
func (u *User) ForgeShareCreator(signer *User, email string) {
|
||||
v := u.view()
|
||||
passphrase, err := decryptArmored(v.d.sharePass.armored, v.addrKey, nil, v.now)
|
||||
mustCrypto("decrypt share passphrase", err)
|
||||
signKey, email := signer.primaryAddressKey()
|
||||
signKey, _ := signer.primaryAddressKey()
|
||||
sig := signArmored(passphrase, signKey)
|
||||
u.srv.mu.Lock()
|
||||
defer u.srv.mu.Unlock()
|
||||
@@ -771,11 +795,11 @@ func (u *User) ForgeShareCreator(signer *User) {
|
||||
d.sharePass.signature = sig
|
||||
}
|
||||
|
||||
// ForgeRootSigner makes u's root folder look made by signer, another
|
||||
// user, as a hostile server could serve it: its signature emails are
|
||||
// signer's primary address and its name and node passphrase are signed
|
||||
// by signer's key.
|
||||
func (u *User) ForgeRootSigner(signer *User) {
|
||||
// ForgeRootSigner makes u's root folder look made by email, signed by
|
||||
// signer's key, as a hostile server could serve it: its signature
|
||||
// emails are email and its name and node passphrase are signed by
|
||||
// signer's primary address key.
|
||||
func (u *User) ForgeRootSigner(signer *User, email string) {
|
||||
v := u.view()
|
||||
root := v.d.links[v.d.rootID]
|
||||
shareKey, err := v.unlockShare()
|
||||
@@ -786,7 +810,7 @@ func (u *User) ForgeRootSigner(signer *User) {
|
||||
mustCrypto("decrypt root passphrase", err)
|
||||
name, err := decryptArmored(root.name, shareKey, nil, v.now)
|
||||
mustCrypto("decrypt root name", err)
|
||||
signKey, email := signer.primaryAddressKey()
|
||||
signKey, _ := signer.primaryAddressKey()
|
||||
sig := signArmored(passphrase, signKey)
|
||||
encName := encryptArmored(name, shareKey, signKey, false)
|
||||
u.srv.mu.Lock()
|
||||
|
||||
@@ -615,8 +615,8 @@ func TestForgeShareAndRoot(t *testing.T) {
|
||||
s := New(t)
|
||||
u := s.AddUser("user", "pass")
|
||||
other := s.AddUser("other", "pass")
|
||||
u.ForgeShareCreator(other)
|
||||
u.ForgeRootSigner(other)
|
||||
u.ForgeShareCreator(other, other.Email())
|
||||
u.ForgeRootSigner(other, other.Email())
|
||||
v := u.view()
|
||||
assert.Equal(t, other.Email(), v.d.creatorEmail)
|
||||
_, err := v.unlockShare()
|
||||
@@ -633,6 +633,21 @@ func TestForgeShareAndRoot(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestServeKeysAs(t *testing.T) {
|
||||
s := New(t)
|
||||
u := s.AddUser("user", "pass")
|
||||
other := s.AddUser("other", "pass")
|
||||
sess := s.NewSession(u)
|
||||
s.ServeKeysAs("USER@proton.test", other)
|
||||
var resp api.PublicKeysResponse
|
||||
status, body := getJSON(t, s, sess, "/core/v4/keys/all?Email=USER@proton.test", &resp)
|
||||
require.Equal(t, http.StatusOK, status, "body: %s", body)
|
||||
require.NotEmpty(t, resp.Address.Keys)
|
||||
want, err := other.addresses[0].keys[0].key.GetArmoredPublicKey()
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, want, resp.Address.Keys[0].PublicKey)
|
||||
}
|
||||
|
||||
func TestNodeSecrets(t *testing.T) {
|
||||
s := New(t)
|
||||
u := s.AddUser("user", "pass")
|
||||
|
||||
@@ -114,6 +114,7 @@ type Server struct {
|
||||
storageUploads int // block uploads in progress
|
||||
storagePeak int // most block uploads in progress at once
|
||||
secrets []issuedSecret // every secret issued, in order
|
||||
keysAs map[string]*User // whose keys GET /core/v4/keys/all serves for an email, overriding the owner's
|
||||
}
|
||||
|
||||
// New starts a fake server which is closed when the test ends.
|
||||
@@ -203,6 +204,18 @@ func (s *Server) ResetRequests() {
|
||||
s.storagePeak = s.storageUploads
|
||||
}
|
||||
|
||||
// ServeKeysAs makes GET /core/v4/keys/all serve owner's primary
|
||||
// address keys for email, as a hostile server could, whoever owns
|
||||
// email.
|
||||
func (s *Server) ServeKeysAs(email string, owner *User) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
if s.keysAs == nil {
|
||||
s.keysAs = make(map[string]*User)
|
||||
}
|
||||
s.keysAs[email] = owner
|
||||
}
|
||||
|
||||
// SecretKind is a kind of secret the server issues.
|
||||
type SecretKind int
|
||||
|
||||
|
||||
@@ -309,8 +309,8 @@ func TestHostileServer(t *testing.T) {
|
||||
name string
|
||||
apply func(u, other *protontest.User)
|
||||
}{
|
||||
{"drive shared by another user", (*protontest.User).ForgeShareCreator},
|
||||
{"root folder signed by another user", (*protontest.User).ForgeRootSigner},
|
||||
{"drive shared by another user", func(u, other *protontest.User) { u.ForgeShareCreator(other, other.Email()) }},
|
||||
{"root folder signed by another user", func(u, other *protontest.User) { u.ForgeRootSigner(other, other.Email()) }},
|
||||
} {
|
||||
t.Run(forge.name, func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
@@ -362,6 +362,91 @@ func TestHostileServer(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
// Forgeries signed by another user's key under an email, which
|
||||
// return the path of a file beneath them.
|
||||
ownAddressForgeries := []struct {
|
||||
name string
|
||||
forge func(u, other *protontest.User, email string) string
|
||||
root bool // whether the forgery is of the drive's share or root
|
||||
}{
|
||||
{"share creator", func(u, other *protontest.User, email string) string {
|
||||
u.SeedFile(u.RootID(), "file.txt", []byte("content"), time.Now())
|
||||
u.ForgeShareCreator(other, email)
|
||||
return "file.txt"
|
||||
}, true},
|
||||
{"root folder signer", func(u, other *protontest.User, email string) string {
|
||||
u.SeedFile(u.RootID(), "file.txt", []byte("content"), time.Now())
|
||||
u.ForgeRootSigner(other, email)
|
||||
return "file.txt"
|
||||
}, true},
|
||||
{"folder signer", func(u, other *protontest.User, email string) string {
|
||||
dir := u.SeedFolderSignedAs(other, email, u.RootID(), "dir")
|
||||
u.SeedFile(dir, "file.txt", []byte("content"), time.Now())
|
||||
return "dir/file.txt"
|
||||
}, false},
|
||||
{"file signer", func(u, other *protontest.User, email string) string {
|
||||
u.SeedFileSignedAs(other, email, u.RootID(), "file.txt", []byte("content"))
|
||||
return "file.txt"
|
||||
}, false},
|
||||
}
|
||||
for _, forgery := range ownAddressForgeries {
|
||||
for _, variant := range []struct {
|
||||
name string
|
||||
email string
|
||||
own bool // whether the email is the user's own address
|
||||
}{
|
||||
{"in capitals", "USER@PROTON.TEST", true},
|
||||
// U+017F folds to s under Unicode case folding, so isn't
|
||||
// the user's address.
|
||||
{"with a long s", "u\u017fer@proton.test", false},
|
||||
} {
|
||||
if !variant.own && !forgery.root {
|
||||
// Another user's signature below the root is checked
|
||||
// against keys the server serves: a protocol limit.
|
||||
continue
|
||||
}
|
||||
t.Run(forgery.name+" as the user's address "+variant.name, func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
remote, srv, u := setupFakeRemote(t)
|
||||
other := srv.AddUser("other", "pass")
|
||||
path := forgery.forge(u, other, variant.email)
|
||||
srv.ServeKeysAs(variant.email, other)
|
||||
srv.ResetRequests()
|
||||
|
||||
f := newFakeFs(t, remote, "")
|
||||
o, err := f.NewObject(ctx, path)
|
||||
require.NoError(t, err, "listing works")
|
||||
got, err := readAll(ctx, o)
|
||||
require.Error(t, err, "read")
|
||||
assert.Empty(t, got)
|
||||
assert.Contains(t, err.Error(), "ignore_signature_errors")
|
||||
if variant.own {
|
||||
assert.Zero(t, srv.Requests()["GET /core/v4/keys/all"], "the server was asked for the user's own keys")
|
||||
} else {
|
||||
assert.Contains(t, err.Error(), "not one of your addresses")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
t.Run("own address stored in mixed case", func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
srv := protontest.New(t)
|
||||
u := srv.AddUser("Mixed@Proton.Test", "pass")
|
||||
u.SeedFileSignedAs(u, "mixed@PROTON.test", u.RootID(), "file.txt", []byte("content"))
|
||||
u.ForgeShareCreator(u, "mixed@proton.test")
|
||||
u.ForgeRootSigner(u, "MIXED@proton.test")
|
||||
srv.ResetRequests()
|
||||
f, err := protondrive.NewFs(ctx, "mixed", "", sessionMap(srv, u))
|
||||
require.NoError(t, err)
|
||||
o, err := f.NewObject(ctx, "file.txt")
|
||||
require.NoError(t, err)
|
||||
got, err := readAll(ctx, o)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, "content", string(got))
|
||||
assert.Zero(t, srv.Requests()["GET /core/v4/keys/all"], "the server was asked for the user's own keys")
|
||||
})
|
||||
|
||||
t.Run("updating a file with an attacker-signed passphrase", func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
remote, _, u := setupFakeRemote(t)
|
||||
|
||||
@@ -5,6 +5,24 @@ it with a new one that links back. One entry per decision a future maintainer
|
||||
would otherwise have to reverse-engineer.
|
||||
No index: `grep -n '^## ' docs/DECISIONS.md` lists every entry.
|
||||
|
||||
## 2026-11-11 - Proton Drive matches email addresses by ASCII case only
|
||||
|
||||
**Status:** accepted
|
||||
**Context:** The own-address check of "Proton Drive roots trust in the
|
||||
user's own addresses" ignored case, but the key lookup didn't, so a
|
||||
server could name the user's address in capitals and serve its own keys
|
||||
for it, at the root and below (Task 16 re-review 2).
|
||||
**Decision:** One rule everywhere: emails are compared after lowercasing
|
||||
ASCII letters only. An email matching one of the account's addresses
|
||||
under it is verified with the account's own keys and never looked up on
|
||||
the server, and other users' keys are cached under the same key. Unicode
|
||||
case folding is not used, as it maps the Kelvin sign to k and the long s
|
||||
to s, letting a non-ASCII address pass for an ASCII one.
|
||||
**Alternatives:** Exact matching (an address the server stores in other
|
||||
case would be looked up remotely); `strings.EqualFold` (Unicode folding).
|
||||
**Consequences:** Two addresses differing only in non-ASCII case are
|
||||
different addresses to rclone.
|
||||
|
||||
## 2026-11-11 - Proton Drive roots trust in the user's own addresses
|
||||
|
||||
**Status:** accepted
|
||||
|
||||
@@ -591,8 +591,12 @@ library what must stay readable.
|
||||
the server's) and
|
||||
`func (u *User) SeedAnonymousFolder(parentID, name string) (id string)`.
|
||||
- Hostile roots (Task 16 fix round 2):
|
||||
`func (u *User) ForgeShareCreator(signer *User)` and
|
||||
`func (u *User) ForgeRootSigner(signer *User)`.
|
||||
`func (u *User) ForgeShareCreator(signer *User, email string)`,
|
||||
`func (u *User) ForgeRootSigner(signer *User, email string)`,
|
||||
`func (s *Server) ServeKeysAs(email string, owner *User)`,
|
||||
`func (u *User) SeedFolderSignedAs(signer *User, email, parentID, name string) (id string)` and
|
||||
`func (u *User) SeedFileSignedAs(signer *User, email, parentID, name string, content []byte) (id string)`
|
||||
(Task 16 fix round 3).
|
||||
- `func (u *User) Dump() []DumpedNode` - decrypted tree for assertions
|
||||
(`ID`, `Path`, `IsFolder`, `Content`, `ModTime`, `SHA1`, `State`, and
|
||||
`Err` joining every decryption, signature, block-hash, manifest and
|
||||
|
||||
Reference in new issue
Block a user