Skip to content

fix(encryption): improve setKey robustness (better checks/throws) #57628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joshtrichards wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

fix(encryption): improve setKey robustness (better checks/throws) #57628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69b1f81
fix(encryption): improve setKey robustness (better checks/throws)
joshtrichards Jan 18, 2026
6d2b803
chore: fixup
joshtrichards Jan 18, 2026
8ec57e8
refactor(encryption): simplify getKey (drop dead code; clarify migrat…
joshtrichards Jan 18, 2026
619b86c
test: drop unused testSetFileOld from StorageTest
joshtrichards Jan 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 71 additions & 77 deletions lib/private/Encryption/Keys/Storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,104 +235,98 @@
}

/**
* read key from hard disk
* Read the key from disk.
*
* @param string $path to key
* @return array containing key as base64encoded key, and possible the uid
* @param string $path Path to key file.
* @return array Containing key as base64encoded key, and possibly the uid.
* @throws ServerNotAvailableException
*/
private function getKey($path): array {
$key = [
'key' => '',
];
if (!$this->view->file_exists($path)) {
return = [

Check failure on line 246 in lib/private/Encryption/Keys/Storage.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

ParseError 

lib/private/Encryption/Keys/Storage.php:246:11: ParseError: Syntax error, unexpected '=', expecting ';' on line 246 (see https://psalm.dev/173)

Check failure on line 246 in lib/private/Encryption/Keys/Storage.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidReturnStatement 

lib/private/Encryption/Keys/Storage.php:246:4: InvalidReturnStatement: Empty return statement is not expected in OC\Encryption\Keys\Storage::getKey (see https://psalm.dev/128)
'key' => '',
];
}

if ($this->view->file_exists($path)) {
if (isset($this->keyCache[$path])) {
$key = $this->keyCache[$path];
} else {
$data = $this->view->file_get_contents($path);

// Version <20.0.0.1 doesn't have this
$versionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0.0');
if (version_compare($versionFromBeforeUpdate, '20.0.0.1', '<=')) {
$key = [
'key' => base64_encode($data),
];
} else {
if ($this->config->getSystemValueBool('encryption.key_storage_migrated', true)) {
try {
$clearData = $this->crypto->decrypt($data);
} catch (\Exception $e) {
throw new ServerNotAvailableException('Could not decrypt key', 0, $e);
}

$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
throw new ServerNotAvailableException('Invalid encryption key');
}

$key = $dataArray;
} else {
/*
* Even if not all keys are migrated we should still try to decrypt it (in case some have moved).
* However it is only a failure now if it is an array and decryption fails
*/
$fallback = false;
try {
$clearData = $this->crypto->decrypt($data);
} catch (\Throwable $e) {
$fallback = true;
}

if (!$fallback) {
$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
throw new ServerNotAvailableException('Invalid encryption key');
}
$key = $dataArray;
} else {
$key = [
'key' => base64_encode($data),
];
}
}
if (isset($this->keyCache[$path])) {
return $this->keyCache[$path];
}

$data = $this->view->file_get_contents($path);
$migrated = $this->config->getSystemValueBool('encryption.key_storage_migrated', true);

if ($migrated) {
try {
$clearData = $this->crypto->decrypt($data);
$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
throw new ServerNotAvailableException('Invalid encryption key');
}

$this->keyCache[$path] = $key;
$key = $dataArray;
} catch (\Exception $e) {
// Config indicates migration completed, but decrypted data is invalid.
throw new ServerNotAvailableException('Could not decrypt key', 0, $e);
}
} else {
// If key storage migration isn't indicated as being complete,
// attempt to decrypt the key as some may have already migrated.
// Otherwise, fall back to returning the (non-migrated) base64-encoded key.
try {
$clearData = $this->crypto->decrypt($data);
$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
// used only to trigger fallback
throw new ServerNotAvailableException('Invalid encryption key');
}
$key = $dataArray;
} catch (\Throwable $e) {
// Fallback: base64-encoded blob if decryption fails
$key = [
'key' => base64_encode($data)
];
}
}

$this->keyCache[$path] = $key;
return $key;
}

/**
* write key to disk
* Write the given key to disk.
*
*
* @param string $path path to key directory
* @param array $key key
* @return bool
* @param string $path Destination file path for the key
* @param array $key Associative array with encryption key data (must have 'key')
* @return bool True if key is persisted; throws on error.
* @throws \UnexpectedValueException if $key structure is invalid (rare)
* @throws \RuntimeException if encode, encrypt, or write fails or is incomplete
*/
private function setKey($path, $key) {
private function setKey(string $path, array $key): bool {
$this->keySetPreparation(dirname($path));

$versionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0.0');
if (version_compare($versionFromBeforeUpdate, '20.0.0.1', '<=')) {
// Only store old format if this happens during the migration.
// TODO: Remove for 21
$data = base64_decode($key['key']);
} else {
// Wrap the data
$data = $this->crypto->encrypt(json_encode($key));
if (!isset($key['key'])) {
throw new \UnexpectedValueException('Provided $key missing required "key" entry');
}

$result = $this->view->file_put_contents($path, $data);
try {
$json = json_encode($key, JSON_THROW_ON_ERROR);
$data = $this->crypto->encrypt($json);
} catch (\JsonException $e) {
throw new \RuntimeException('Failed to JSON encode key for storage: ' . $e->getMessage(), 0, $e);
} catch (\Throwable $e) {
throw new \RuntimeException('Failed to encrypt key for storage: ' . $e->getMessage(), 0, $e);
}

if (is_int($result) && $result > 0) {
$this->keyCache[$path] = $key;
return true;
$result = $this->view->file_put_contents($path, $data);
$expected = \strlen($data);
if ($result === false || $result !== $expected) {
throw new \RuntimeException(
"Failed to write encryption key to {$path}: "
. ($result === false ? 'file_put_contents returned false' : "wrote {$result} bytes, expected ".\strlen($data))
);
}

return false;
$this->keyCache[$path] = $key;
return true;
}

/**
Expand Down
26 changes: 0 additions & 26 deletions tests/lib/Encryption/Keys/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,32 +89,6 @@ public function testSetFileKey(): void {
);
}

public function testSetFileOld(): void {
$this->config->method('getSystemValueString')
->with('version')
->willReturn('20.0.0.0');
$this->util->expects($this->any())
->method('getUidAndFilename')
->willReturn(['user1', '/files/foo.txt']);
$this->util->expects($this->any())
->method('stripPartialFileExtension')
->willReturnArgument(0);
$this->util->expects($this->any())
->method('isSystemWideMountPoint')
->willReturn(false);
$this->crypto->expects($this->never())
->method('encrypt');
$this->view->expects($this->once())
->method('file_put_contents')
->with($this->equalTo('/user1/files_encryption/keys/files/foo.txt/encModule/fileKey'),
$this->equalTo('key'))
->willReturn(strlen('key'));

$this->assertTrue(
$this->storage->setFileKey('user1/files/foo.txt', 'fileKey', 'key', 'encModule')
);
}

public static function dataTestGetFileKey() {
return [
['/files/foo.txt', '/files/foo.txt', true, 'key'],
Expand Down
Loading