Aias00 commented on PR #6531:
URL: https://github.com/apache/shenyu/pull/6531#issuecomment-5193353944
Strong test coverage (parameterized round-trip incl. CJK/JSON, wrong
key-length, non-base64, SPI loadability). The SPI registration and the
BouncyCastle provider guard are correct. One security design issue I think
should be addressed before merge:
**CBC with a fixed IV — IV reused across all messages.**
`AbstractCbcCryptorStrategy.buildCipher` parses the IV from the rule's `key`
string (`base64(secret):base64(iv)`) and uses the same `(key, IV)` pair for
every message encrypted under that rule. A gateway encrypts many
requests/responses under one configured rule, so the same IV is reused
per-message. CBC IV reuse leaks first-block plaintext equality and opens
chosen-plaintext attack paths (CWE-329/CWE-1204) — and gateway request bodies
are often partially attacker-controlled, which makes the CPA path reachable.
The Javadoc acknowledges this ("Security note on IV reuse") and mentions a
random-IV-per-message variant as a future enhancement, but I think it should be
the default design rather than a TODO. Suggested fix: generate a fresh IV via
`SecureRandom` per `encrypt`, prepend it to the ciphertext (`base64(iv ||
ciphertext)`), and parse it on `decrypt` — or switch to `AES/GCM/NoPadding`,
which also gives authenticated
encryption and resolves the malleability concern below.
**CBC without authentication (malleable).** `AES/CBC/PKCS7Padding` with no
MAC means an attacker who can modify ciphertext in transit can flip bits to
affect the decrypted plaintext (CBC bit-flipping). AES-GCM (above) would solve
this; alternatively CBC+HMAC. The existing RSA strategy is also
encryption-only, so this isn't a regression, but for new strategies it's worth
flagging.
Minor: key material (`base64(secret):base64(iv)`) lives in the rule config
and flows through admin/data-sync — consistent with RSA, so not a regression,
but there's no KMS/vault integration for the new symmetric secrets. Also a
small nit: `encrypt` uses `Base64.getEncoder()` (standard) while key/ciphertext
parsing uses `getMimeDecoder()` (lenient, ignores line separators) —
`getDecoder()` would be stricter and symmetric.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
