Skip to content

fix(encryption): apply the file header before reading the unencrypted block size - #64556

Open
glazperle wants to merge 1 commit into
nextcloud:masterfrom
glazperle:fix/encryption-legacy-block-size
Open

glazperle wants to merge 1 commit into
nextcloud:masterfrom
glazperle:fix/encryption-legacy-block-size

Conversation

@glazperle

Copy link
Copy Markdown

Summary

fixUnencryptedSize() in lib/private/Files/Storage/Wrapper/Encryption.php asked the encryption module for the unencrypted block size before calling begin(). The module only learns a file's encoding (binary or legacy base64) from the header inside begin(), so getUnencryptedBlockSize() always answered from its default $useLegacyBase64Encoding = false (apps/encryption/lib/Crypto/Encryption.php:53) and returned the binary block size for every file.

For legacy base64 files that is 8096 instead of 6072 bytes per block, and the recalculated unencrypted_size ends up exactly 4/3 too large.

This PR moves the begin() call in front of the block size lookup. begin() only resolves file key, cipher, version and encoding; it does not touch the stream, so the reordering has no other effect.

Fixes #63662

Why this matters

  • Downloads announce a Content-Length larger than the data and break; the desktop client reports RemoteHostClosedError, curl (18) end of response with X bytes missing.
  • With per-user keys the recalculation cannot run from cron; those files stay at unencrypted_size = 0 and show as 0 B. The first access from a logged-in session then writes the inflated value.
  • The wrong value is self-preserving: a base64 file's on-disk size is about 1.349·P, the inflated value 1.333·P sits below it and so passes every plausibility check in verifyUnencryptedSize(). It is never corrected again.
  • readCache() in lib/private/Files/Stream/Encryption.php derives the end suffix of the last block's HMAC position from unencrypted_size. With an inflated value the real last block is verified as N instead of Nend and fails with "Bad Signature" — see [Bug]: Encryption: AES-256-CTR legacy files fail signature verification on final block - Bad Signature #64078.

The wrong calculation is old, but it only started being persisted with eca9503 (#62780 for 34.0.3, #62779 for 33.0.8), which added recalculation for rows at zero. Before that the value was computed wrongly but never written.

Test

testFixUnencryptedSizeUsesBlockSizeFromHeader builds a module mock that answers 8096 before begin() and 6072 after it — the way the real module behaves — feeds fixUnencryptedSize() a stream with a header, three full blocks and a partial last block, and expects 3 · 6072 + strlen(plaintext of the last block).

There was no test exercising fixUnencryptedSize() itself so far; testVerifyUnencryptedSize mocks it away.

  • On master the new test fails with 24388 (= 3 · 8096 + 100) against expected 18316.
  • With the fix it passes. The rest of EncryptionTest.php is unchanged (the DB-access failures in that file when run outside --group DB are pre-existing and identical with and without the patch).

Reproducing on a fresh instance

No legacy data needed:

  1. Enable server-side encryption, set 'encryption.use_legacy_base64_encoding' => true in config.php, upload a file of a few hundred KB.
  2. UPDATE oc_filecache SET unencrypted_size = 0 WHERE fileid = <id>;
  3. Download the file while logged in. unencrypted_size is rewritten to 4/3 of the plaintext size and the download is cut short.

Repairing existing rows

Not part of this PR. The plaintext size can be recomputed from the file's block structure without decryption or keys; scripts and details are in #63662. A follow-up as an occ command is possible if wanted.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests are added or updated
  • Documentation (not needed: no user-facing change)

🤖 Generated with Claude Code

… block size

fixUnencryptedSize() asked the encryption module for the unencrypted block
size before calling begin(). The module only learns a file's encoding
(binary or legacy base64) from the header in begin(), so it answered from
its default and returned the binary block size for every file. For legacy
base64 files that is 8096 instead of 6072 bytes per block, and the
recalculated unencrypted_size ends up exactly 4/3 too large.

The wrong value breaks downloads (Content-Length larger than the data),
shows files as 0 B when the recalculation cannot run, and is never
corrected again because it stays below the on-disk size and so passes
every plausibility check in verifyUnencryptedSize(). Since the position
suffix "end" for the last block is derived from unencrypted_size, the
inflated value also makes the last block fail its signature check.

begin() only resolves file key, cipher, version and encoding and does not
touch the stream, so it can be called before the block size is read.

Fixes nextcloud#63662

Signed-off-by: Maximilian von Heyden <maximilian@vheyden.de>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@glazperle

Copy link
Copy Markdown
Author

/backport to stable34

@glazperle

Copy link
Copy Markdown
Author

/backport to stable33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong unencrypted_size for legacy base64 files: block size is read before the file header is applied

1 participant