File encryption corruption when trying to encode existing files


Observed Behavior:

After initializing the keyring, I attempted to encode an existing file with aes-256-cbc. However, it always returns the same thing when cat-ing that file.
[kendfinger@spot phabricator]$ ./bin/files cat F54
Usage Exception: File data integrity check failed. Use “–salvage” to bypass integrity checks. This flag is dangerous, use it at your own risk. Underlying error: File data integrity check failed. Dark forces have corrupted or tampered with this file. The file data can not be read.

When I set the key to be the default key, fresh files are fine. Essentially, the migrateToStorageEngine() code in PhabricatorFile fails to work correctly.

Also a note, --salvage returns the correct data, with the correct hash. As I say at the bottom, it seems as though this is because of a mismatch in the hashes that are being verified (integrity hash vs encrypted content hash)

I have detailed logs here (I added HMAC logging and logging about the hashes it observed as well. The filenames in the Gist describe the stages that the logs are from). Also, all content and keys here are throw-aways.

Expected Behavior:
Phabricator should correctly encode the file, and pass integrity checks.

Phabricator Version:
phabricator 05f333dfba0ad17fecab36c5bd8dce215e9f221f (Mon, Jun 18)
arcanist 222800a86ed002c564e2760d6c5d9e93810b5b96 (Tue, Jun 19)
phutil 4206849bb05b60f536a1c78e33adee68dac67aa9 (Thu, Jun 7)

Reproduction Steps:

  1. Upload a file to Phabricator
  2. Configure the keyring
  3. ./bin/files encode --key files-key --as aes-256-cbc F1
  4. ./bin/files cat F1

After some investigation, I determined that it seems as though the integrity hash is checking against the original content, and not of the formatted content.
The failure comes from the following pseudo-code:
$expected_from_integrity_hash_in_db != $actual_storage_format_encrypted_content_hash

I’ve reproduced this bug on 3 systems, and all give the same result.

I’d also like to take the time to thank the wonderful developers at Phacility. Phabricator is a one of a kind product.