protondrive: return only the bytes asked for when a range covers the whole file
A range covering the whole file was read as a plain full read, so a file whose recorded size understated its content returned more bytes than the range asked for. It is now checked as a full read but stops at the end of the range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
8666b66038
commit
0cd3af4295
6 files changed
+69
-6
No files matched your search
@@ -86,6 +86,18 @@ const (
|
||||
// signature which fails is errIntegrity from open, on the same terms.
|
||||
// The reader must be closed.
|
||||
func (c *client) open(ctx context.Context, n *node, offset, limit int64) (io.ReadCloser, error) {
|
||||
return c.openRead(ctx, n, offset, limit, offset == 0 && limit < 0)
|
||||
}
|
||||
|
||||
// openWhole is open for limit bytes from offset 0 of a range which
|
||||
// covers the whole file, checked as a full read is.
|
||||
func (c *client) openWhole(ctx context.Context, n *node, limit int64) (io.ReadCloser, error) {
|
||||
return c.openRead(ctx, n, 0, limit, true)
|
||||
}
|
||||
|
||||
// openRead is open, listing every block and checking the manifest
|
||||
// before releasing any byte if full is set, which needs offset 0.
|
||||
func (c *client) openRead(ctx context.Context, n *node, offset, limit int64, full bool) (io.ReadCloser, error) {
|
||||
if n.IsFolder || n.link == nil || n.link.File == nil || n.RevisionID == "" {
|
||||
return nil, fmt.Errorf("open %q: not a file with content", n.Name)
|
||||
}
|
||||
@@ -100,7 +112,7 @@ func (c *client) open(ctx context.Context, n *node, offset, limit int64) (io.Rea
|
||||
if n.X != nil {
|
||||
sizes = n.X.BlockSizes
|
||||
}
|
||||
full := offset == 0 && limit < 0
|
||||
full = full && offset == 0
|
||||
if len(sizes) > c.maxFileBlocks {
|
||||
return nil, fmt.Errorf("open %q: %w: %d blocks recorded, more than %d blocks", n.Name, errIntegrity, len(sizes), c.maxFileBlocks)
|
||||
}
|
||||
@@ -132,7 +144,7 @@ func (c *client) open(ctx context.Context, n *node, offset, limit int64) (io.Rea
|
||||
// bytes.
|
||||
first, skip := blockAt(sizes, offset)
|
||||
last := -1
|
||||
if limit > 0 {
|
||||
if limit > 0 && !full {
|
||||
last, _ = blockAt(sizes, offset+limit-1)
|
||||
}
|
||||
maxBlocks := len(sizes)
|
||||
|
||||
@@ -1162,11 +1162,14 @@ func (o *Object) Open(ctx context.Context, options ...fs.OpenOption) (io.ReadClo
|
||||
}
|
||||
}
|
||||
}
|
||||
var rc io.ReadCloser
|
||||
var err error
|
||||
if known && offset == 0 && limit >= size {
|
||||
// The whole file, as a full read, so the manifest is checked.
|
||||
limit = -1
|
||||
// The whole file, checked as a full read is.
|
||||
rc, err = o.fs.c.openWhole(ctx, o.n, limit)
|
||||
} else {
|
||||
rc, err = o.fs.c.open(ctx, o.n, offset, limit)
|
||||
}
|
||||
rc, err := o.fs.c.open(ctx, o.n, offset, limit)
|
||||
if err != nil {
|
||||
return nil, translateError(err, opObject)
|
||||
}
|
||||
|
||||
@@ -834,6 +834,28 @@ func (u *User) SetParent(id, parentID string) {
|
||||
l.parentID = parentID
|
||||
}
|
||||
|
||||
// ClaimSize makes the extended attributes of file id's active revision
|
||||
// claim size, signed by u as an uploader with a bug could write them.
|
||||
func (u *User) ClaimSize(id string, size int64) {
|
||||
v := u.view()
|
||||
l := v.d.links[id]
|
||||
if l == nil || l.typ != linkTypeFile {
|
||||
panic("protontest: no file " + id)
|
||||
}
|
||||
key, _ := v.contentKey(l)
|
||||
plain, err := decryptArmored(l.activeRevision().xattr, key, nil, v.now)
|
||||
mustCrypto("decrypt extended attributes", err)
|
||||
var x fileXAttr
|
||||
mustCrypto("parse extended attributes", json.Unmarshal(plain, &x))
|
||||
x.Common.Size = &size
|
||||
out, err := json.Marshal(&x)
|
||||
mustCrypto("marshal extended attributes", err)
|
||||
enc := encryptArmored(out, key, v.addrKey, true)
|
||||
u.srv.mu.Lock()
|
||||
defer u.srv.mu.Unlock()
|
||||
changeActiveRevision(func(r *revision) { r.xattr = enc })(u.mustDriveLocked().links[id])
|
||||
}
|
||||
|
||||
// insertLink adds an active link l under parentID and returns its new
|
||||
// ID, created now unless l has a creation time. It panics if parentID
|
||||
// has an active or draft child with the same name hash, which a seeding
|
||||
|
||||
@@ -499,6 +499,27 @@ func TestHostileServer(t *testing.T) {
|
||||
assert.Equal(t, 4, strings.Count(log, unverified), log)
|
||||
})
|
||||
|
||||
t.Run("a range covering the whole of a file claiming a smaller size", func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
remote, _, u := setupFakeRemote(t)
|
||||
content := patternedContent(blockSize+10, 14)
|
||||
id := u.SeedFile(u.RootID(), "short.bin", content, time.Now())
|
||||
const claimed = 100
|
||||
u.ClaimSize(id, claimed)
|
||||
f := newFakeFs(t, remote, "")
|
||||
o, err := f.NewObject(ctx, "short.bin")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, int64(claimed), o.Size())
|
||||
for _, r := range []fs.RangeOption{{Start: 0, End: -1}, {Start: 0, End: claimed - 1}} {
|
||||
rc, err := o.Open(ctx, &r)
|
||||
require.NoError(t, err)
|
||||
got, err := io.ReadAll(rc)
|
||||
require.NoError(t, rc.Close())
|
||||
require.NoError(t, err, "%v", r)
|
||||
assert.True(t, bytes.Equal(content[:claimed], got), "%v returned %d bytes, not the %d asked for", r, len(got), claimed)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("updating a file with an attacker-signed passphrase", func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
remote, _, u := setupFakeRemote(t)
|
||||
|
||||
@@ -8,6 +8,10 @@ Small, specific, independently actionable. Remove only when actually done.
|
||||
|
||||
## Next
|
||||
|
||||
- [ ] protondrive: a range from offset 0 of a file whose size isn't
|
||||
recorded (`bytes=0-N`, N at or past the end) is read as a ranged read,
|
||||
so its manifest isn't checked; only a known size identifies a range
|
||||
covering the whole file.
|
||||
- [ ] protondrive: below the root, a node signed under another user's
|
||||
email (a shared folder's members) is verified against public keys the
|
||||
server supplies (`account.publicKeys`), so a hostile server can forge
|
||||
|
||||
@@ -300,7 +300,8 @@ AES-GCM blocks; names, passphrases, hash keys and attributes without AEAD).
|
||||
Failure is an error. An advanced option `ignore_signature_errors`
|
||||
downgrades content and authorship signature failures to a logged `ERROR`
|
||||
for users with legacy or shared data (#8003, #7344). **(call)** A range
|
||||
covering the whole file from offset 0 (as `bytes=0-`) is a full read.
|
||||
covering the whole file from offset 0 (as `bytes=0-`) is checked as a
|
||||
full read, still returning only the bytes asked for.
|
||||
Other ranged reads check each block against its listed hash but cannot
|
||||
verify the manifest; this is documented, as it is in the SDK.
|
||||
- A file may list at most 2^19 blocks (2 TiB in 4 MiB blocks, more than
|
||||
|
||||
Reference in new issue
Block a user