fix(powerdns): prevent decrypt() garbage from corrupting plaintext API keys

WHMCS's addon-module password-type fields are stored plaintext in
tbladdonmodules.value — unlike tblservers.password which IS encrypted
at rest. Config::get() was blindly calling decrypt() on the raw value
and then preferring its output over raw when the two differed.

Unfortunately, when decrypt() is fed a plaintext string, it doesn't
return empty or unchanged — it returns a short binary-garbage string
(observed: 4 bytes of \xEF\xBF\xBD unicode-replacement noise for a
32-char plaintext). That garbage then went into the X-API-Key header,
PowerDNS responded 401, and every rDNS read returned an empty zone list,
surfacing as "no zone" for every IP in the client UI.

Fix: only accept decrypt()'s output when it's printable ASCII. Real
API keys are always printable; decrypted ciphertext that looks like
binary is a mangled-plaintext signal, so we fall back to raw. Also
trim() the chosen value to defeat a second foot-gun — admin UIs can
silently append a newline on paste, which would land in the header
verbatim and produce the same 401.

Diagnosed via direct WHMCS tbladdonmodules inspection on a user's
affected install; confirmed the fix end-to-end with a live ping()
returning HTTP 200 post-deploy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Prophet731
2026-04-17 22:04:04 -04:00
parent 65f3f36569
commit daaddc7c24

View File

@@ -119,23 +119,50 @@ class Config
$config['cacheTtl'] = max(10, (int) ($rows['cacheTtl'] ?? 60)); $config['cacheTtl'] = max(10, (int) ($rows['cacheTtl'] ?? 60));
if (! empty($rows['apiKey'])) { if (! empty($rows['apiKey'])) {
$raw = (string) $rows['apiKey'];
$decrypted = '';
try { try {
// decrypt() is WHMCS's global helper — matches how the VirtFusion // decrypt() is WHMCS's global helper — matches how the VirtFusion
// bearer token is handled in Module::getCP(). // bearer token is handled in Module::getCP().
$decrypted = decrypt($rows['apiKey']); $decrypted = (string) decrypt($raw);
// Fallback to raw value if decrypt returned empty or non-string —
// defends against the rare case where decrypt silently fails
// (wrong encryption key at rest) or the value was inserted
// manually as plaintext during development.
$config['apiKey'] = is_string($decrypted) && $decrypted !== '' ? $decrypted : (string) $rows['apiKey'];
} catch (\Throwable $e) { } catch (\Throwable $e) {
// Even when decrypt throws, we try the raw value so a diagnostic // Even when decrypt throws, we try the raw value so a diagnostic
// path exists. Operator sees the decrypt error in the module log // path exists. Operator sees the decrypt error in the module log
// but isn't locked out of using the addon while they investigate. // but isn't locked out of using the addon while they investigate.
$config['apiKey'] = (string) $rows['apiKey']; Log::insert('PowerDns:Config', 'decrypt threw', $e->getMessage());
Log::insert('PowerDns:Config', 'decrypt skipped', $e->getMessage());
} }
// WHMCS addon module password-type fields are stored PLAINTEXT in
// tbladdonmodules.value (unlike tblservers.password which IS encrypted).
// When fed a plaintext input, WHMCS's decrypt() doesn't return empty
// or unchanged — it returns a short binary garbage string. If we used
// that as the API key we'd produce a baffling 401 from PowerDNS.
//
// Heuristic: an API key is printable ASCII by definition. If
// decrypt() produced non-printable output, we know it mangled a
// plaintext value and we should stick with raw. If decrypt()
// produced a different-but-printable string, it's a genuine
// decryption of an actually-encrypted value (unusual for addons,
// but some third-party setups do encrypt at rest).
//
// trim() handles another common foot-gun: admin UIs silently
// appending a newline on paste, which would land in the
// X-API-Key: header verbatim and also produce a 401.
$candidate = $raw;
if ($decrypted !== '' && $decrypted !== $raw && ctype_print($decrypted)) {
$candidate = $decrypted;
} elseif ($decrypted !== '' && $decrypted !== $raw) {
// Decrypt output wasn't printable — it's garbage from mangling
// a plaintext input. Log once so the diagnostic trail is clear
// but don't expose key material.
Log::insert(
'PowerDns:Config',
'decrypt produced non-printable output; using raw',
['raw_len' => strlen($raw), 'dec_len' => strlen($decrypted)],
);
}
$config['apiKey'] = trim($candidate);
} }
} catch (\Throwable $e) { } catch (\Throwable $e) {
// Any DB-level failure (table doesn't exist, connection dropped, etc.) // Any DB-level failure (table doesn't exist, connection dropped, etc.)