[Bug]: AppConfig::setTypedValue() decrypts the existing value when comparing, so a sensitive key is unwritable after a `secret` change
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 36.9k
- Forks
- 5.2k
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 713
Description
Summary
OC\AppConfig::setTypedValue() guards against a no-op write by comparing the new value against the
current one. For a VALUE_SENSITIVE key that comparison decrypts the stored value, so once a
sensitive config value becomes undecryptable (an instance secret that no longer matches the one
the value was written under), the key can no longer be read or overwritten — every attempt
throws RuntimeException: HMAC does not match.
This makes the failure permanent and unrecoverable through the public API. It also silently defeats
apps that explicitly try to recover from it — notifications is one, see below.
https://github.com/nextcloud/server/blob/master/lib/private/AppConfig.php#L860-L867
if ($this->hasKey($app, $key, $lazy)) {
/**
* no update if key is already known with set lazy status and value is
* not different, unless sensitivity is switched from false to true.
*/
if ($origValue === $this->getTypedValue($app, $key, $value, $lazy ?? true, $type) // ← decrypts, throws
&& (!$sensitive || $this->isSensitive($app, $key, $lazy))) {
return false;
}
}
getTypedValue() reaches Crypto::decrypt() for a sensitive key, which throws when the HMAC does
not verify. The exception escapes setTypedValue(), so the write never happens.
Steps to reproduce
Minimal:
- Store a sensitive app config value:
$appConfig->setValueString('myapp', 'mykey', 'value-1', sensitive: true); - Change
secretinconfig.php(or restore a database into an instance whosesecretdiffers —
the realistic path). - Try to overwrite it with a fresh value:
$appConfig->setValueString('myapp', 'mykey', 'value-2', sensitive: true);
Expected: the write succeeds — the caller supplied a complete new value and never asked to read
the old one. (Or at worst, a typed exception the caller can handle.)
Actual: RuntimeException: HMAC does not match. from lib/private/Security/Crypto.php#171.
The only way out is to delete the row directly so hasKey() returns false and the insert branch is
taken.
Real-world manifestation
notifications already anticipates a mismatched instance secret and tries to self-heal —
WebPushClient::getVapid():
try {
$publicKey = $this->appConfig->getAppValueString('webpush_vapid_pubkey');
$privateKey = $this->appConfig->getAppValueString('webpush_vapid_privkey');
} catch (\Throwable) {
// Decryption failed (e.g. mismatched instance secret), regenerate keys
$publicKey = '';
$privateKey = '';
}
if ($publicKey === '' || $privateKey === '') {
$vapid = VAPID::createVapidKeys();
$this->appConfig->setAppValueString('webpush_vapid_pubkey', $vapid['publicKey']);
$this->appConfig->setAppValueString('webpush_vapid_privkey', $vapid['privateKey'], sensitive: true);
}
The catch handles the failed read exactly as intended — but the recovery write then hits
the compare above and throws again, this time outside the try. So the intended self-heal can never
complete, and the constructor throws on every instantiation.
Why this is worse than a log line: nothing catches it further up, so
OC\Log\ErrorHandler::onException logs it and the PHP process dies. On our instance this killed the
nightly OCA\UpdateNotification\BackgroundJob\UpdateAvailableNotifications job for six and a half
weeks — it did not complete a single run in that window, and because each crash left the job row
reserved, it eventually stopped being scheduled at all. occ user:delete also exited non-zero
after having already removed the account, leaving the home directory behind. Nothing was visible
in the UI, and the log line names only Crypto.php#171 — never the app, key, or caller — so the
cause is very hard to find. (occ background-job:history --status=crashed was what located it.)
Suggested fix
Make the no-op comparison tolerant of an unreadable current value — the caller of setTypedValue()
supplied a complete new value and did not ask for the old one. Roughly:
try {
$sameValue = ($origValue === $this->getTypedValue($app, $key, $value, $lazy ?? true, $type));
} catch (\Exception) {
$sameValue = false; // cannot read the stored value -> it is not equal -> fall through and write
}
if ($sameValue && (!$sensitive || $this->isSensitive($app, $key, $lazy))) {
return false;
}
That turns an unrecoverable state into a self-healing one for every app that overwrites a sensitive
key, and costs only the skip-identical-write optimisation in the rare undecryptable case.
Happy to open a PR if the approach looks right.
Server configuration
Nextcloud version: 34.0.3 (bug confirmed present on master at the time of writing)
Notifications app: 7.0.0-dev.1
Database: PostgreSQL 18 (CloudNativePG)
PHP version: as shipped in the official nextcloud:34-apache image
Trigger in our case: the instance secret was replaced during a disaster-recovery rebuild, so a
restored database carried values encrypted under the previous key.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in lib/private/AppConfig.php at setTypedValue(), then trace getTypedValue() into lib/private/Security/Crypto.php#171. Reproduce an overwrite of a sensitive value after the instance secret changes and inspect the relevant AppConfig tests. Done means an unreadable existing value no longer prevents a complete replacement, while unchanged readable values retain no-op behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100