Harden decryption error handling and mask secrets in logs#476
Conversation
Return a single generic error for all decryption failures so they are indistinguishable, closing an AES-CBC + PKCS7 padding-oracle side channel. Check CCCryptorFinal status so bad-padding failures are no longer silently treated as success. Mask cipher key and auth key in dictionaryRepresentation instead of logging their values.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
jguz-pubnub
changed the title
Restore the original descriptive errors on the length/emptiness guards in PNAESCBCCryptor: they fire before any cipher operation on attacker-known input lengths, so they're not a padding-oracle bit and aid integrator debugging. The padding-oracle mitigation stays in PNCCCryptorWrapper.
jguz-pubnub
changed the title
… it in userInfo The generic decryption error no longer carries the raw CCCryptorStatus under a userInfo key, so callers can't recover the padding-oracle bit the generic error is meant to hide. Removes the now-unused public PNCryptorUnderlyingStatusKey. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
parfeon
left a comment
parfeon
left a comment
There was a problem hiding this comment.
Did we had any final agreement that on user's debug session (when debug log level has been set on purpose) we still need to hide this information (at least auth can stay as is)?
Also I've hidden cipher that way, so it will be possible for user to figure out which cipher key he used, so he will be able to assist later during debug.
| // Decryption failures must not be distinguishable from one another: surfacing whether a | ||
| // failure was caused by bad padding vs. wrong length vs. other reasons gives an attacker a | ||
| // padding-oracle bit (AES-CBC + PKCS7). All decrypt failures return one identical generic | ||
| // error with the underlying CommonCrypto status discarded. | ||
| if (operation != kCCEncrypt) return [self genericDecryptionError]; | ||
|
|
There was a problem hiding this comment.
who is attacker? Application developer?
Oracle attack makes sense if attacker has a ciphertext and then tries to craft secret which will let him access encrypted data. They only one who will be able to do it inside of compiled application - app developer.
| code:PNCryptorErrorDecryption | ||
| userInfo:@{ NSLocalizedDescriptionKey: @"Unable to encrypt empty data." }]; | ||
| return [PNResult resultWithData:nil error:error]; | ||
| return [PNResult resultWithData:nil error:[PNCCCryptorWrapper genericDecryptionError]]; |
There was a problem hiding this comment.
Well, there were copy & paste issue and description key should contain message that empty data can't be decrypted - why we want to make it generic without giving any info (in this case there is nothing for the attacker at all)?
| code:PNCryptorErrorDecryption | ||
| userInfo:@{ NSLocalizedDescriptionKey: @"Unable to encrypt empty data." }]; | ||
| return [PNResult resultWithData:nil error:error]; | ||
| return [PNResult resultWithData:nil error:[PNCCCryptorWrapper genericDecryptionError]]; |
There was a problem hiding this comment.
Well, there were copy & paste issue and description key should contain message that empty data can't be decrypted - why we want to make it generic without giving any info (in this case there is nothing for the attacker at all)?
Co-authored-by: Serhii Mamontov <parfeon@me.com>
jguz-pubnub
force-pushed
the
objective-c-sec-fixes
branch
from
c18d47c to
5083c28
Compare
|
@pubnub-release-bot release |
pubnub-release-bot
requested review from
kazydek and
techwritermat
as code owners
|
🚀 Release successfully completed 🚀 |
4 participants
fix: return one generic decryption error from cryptors instead of a specific message
The cryptor wrapper now returns a single generic error on any decryption failure instead of surfacing whether it was caused by bad padding, wrong length or other reasons.
fix: check CCCryptorFinal status so bad-padding failures aren't treated as success
Validate the CCCryptorFinal status, where PKCS7 padding is checked, to avoid bad-padding failures being silently treated as successful decryptions.
fix: mask cipher key and auth key in debug descriptions instead of logging their values
PNAESCBCCryptorandPNConfigurationno longer expose the cipher key or auth key indictionaryRepresentation; both are masked.