From a security review of the OAuth code (see also #866). The on-disk token cache (cache_disk()) currently stores tokens as serialized R objects encrypted with AES-CTR using the obfuscation key that ships inside the package. Since that key is public by design, the format has two weaknesses beyond the (documented) lack of confidentiality:
- No integrity. AES-CTR is unauthenticated and malleable, so anyone who can write to the cache directory can tamper with or forge a cache entry that decrypts "successfully", e.g. to inject an attacker-controlled access token.
- Untrusted deserialization.
secret_read_rds() calls unserialize() on the decrypted bytes, so a forged cache file yields an arbitrary R object — an unnecessary attack surface for method-dispatch surprises.
Proposal
- Encrypt with
openssl::aes_gcm_encrypt() instead of aes_ctr_encrypt(). GCM is authenticated, so a tampered or forged file fails loudly at decryption instead of decrypting to attacker-controlled bytes. openssl is already in Imports.
- Store the token as JSON rather than a serialized R object. Tokens arrive as JSON and
oauth_token() objects are simple lists, so this is a natural fit and eliminates the unserialize() surface entirely.
To be clear about the threat model: the obfuscation key remains public, so this is tamper-evidence, not confidentiality. Real confidentiality against local attackers is a separate discussion (OS keyring backend); file permissions were addressed in #866.
Migration
This changes the cache file format, invalidating existing cached tokens (users re-authenticate once). Precedent: 1.3.0 already invalidated all cached tokens via the rlang hash change, so a format break is survivable, and doing it soon after is the cheap moment. Old-format files are cleaned up by the existing pruning rules; we could also proactively delete anything that fails GCM authentication.
Open question
Should the exported secret_write_rds()/secret_read_rds() (and secret_encrypt_file() etc.) also move to GCM? They're used with real user-supplied keys, where tampering is less of a concern, but authenticated encryption would still be an improvement. That requires reading the old format for back-compat, so it could be a separate follow-up.
From a security review of the OAuth code (see also #866). The on-disk token cache (
cache_disk()) currently stores tokens as serialized R objects encrypted with AES-CTR using the obfuscation key that ships inside the package. Since that key is public by design, the format has two weaknesses beyond the (documented) lack of confidentiality:secret_read_rds()callsunserialize()on the decrypted bytes, so a forged cache file yields an arbitrary R object — an unnecessary attack surface for method-dispatch surprises.Proposal
openssl::aes_gcm_encrypt()instead ofaes_ctr_encrypt(). GCM is authenticated, so a tampered or forged file fails loudly at decryption instead of decrypting to attacker-controlled bytes. openssl is already in Imports.oauth_token()objects are simple lists, so this is a natural fit and eliminates theunserialize()surface entirely.To be clear about the threat model: the obfuscation key remains public, so this is tamper-evidence, not confidentiality. Real confidentiality against local attackers is a separate discussion (OS keyring backend); file permissions were addressed in #866.
Migration
This changes the cache file format, invalidating existing cached tokens (users re-authenticate once). Precedent: 1.3.0 already invalidated all cached tokens via the rlang hash change, so a format break is survivable, and doing it soon after is the cheap moment. Old-format files are cleaned up by the existing pruning rules; we could also proactively delete anything that fails GCM authentication.
Open question
Should the exported
secret_write_rds()/secret_read_rds()(andsecret_encrypt_file()etc.) also move to GCM? They're used with real user-supplied keys, where tampering is less of a concern, but authenticated encryption would still be an improvement. That requires reading the old format for back-compat, so it could be a separate follow-up.