Created attachment 192613 [details] Fix proposal Hello, encrypt_multi and decrypt_multi seems to process too many data when the underlying buffer is larger than the data to process. Steps to reproduce: use encrypt_multi/decrypt_multi into an IPSEC context (for ciphering ESP). Decryption will erase the authentication data contained at the end of the mbuf. I attached a patch with the fixup I used.
Nice find. It seems like this can only happen if passed an mbuf chain or uio longer than crd_len. Does using the API like this actually make sense? We could maybe add an assertion that buffer input sizes matches crd_len (+ crd_skip). In the case of esp_input(), it seems like m_split() could be used to separate the authentication portion from the encrypted contents before passing to crypto_dispatch(). OTOH, your patch looks totally correct. I would additionally cast i to size_t to avoid ambiguity in comparison signedness in min() macro.
FWIW, clearly introduced in r331639 (my fault). If you hit this, does that mean you are trying to use IPSec with Chacha20? Nothing else implements decrypt_multi. :-)
No, I'm not trying to use IPSEC with Chacha20. We did implement the decrypt_multi for AES-CBC. In my opinion, it makes sense to use the API like this. It's simpler for the caller and not so hard to manage in OCF. Actually, it works fine when doing the encryption block by block, since the loop ends when crd_len is processed. Ok for the cast.
(In reply to valentin.vergez@stormshield.eu from comment #3) > We did implement the decrypt_multi for AES-CBC. Oh, cool. > In my opinion, it makes sense to use the API like this. It's simpler for the caller and not so hard to manage in OCF. Actually, it works fine when doing the encryption block by block, since the loop ends when crd_len is processed. Yep, only *crypt_multi case is broken. I will commit your fix. Thanks for the report and patch.
A commit references this bug: Author: cem Date: Thu Apr 19 15:24:22 UTC 2018 New revision: 332778 URL: https://svnweb.freebsd.org/changeset/base/332778 Log: cryptosoft: Do not exceed crd_len around *crypt_multi When a caller passes in a uio or mbuf chain that is longer than crd_len, in tandem with a transform that supports the multi-block interface, swcr_encdec() would process the entire mbuf or uio instead of just the portion indicated by crd_len (+ crd_skip). De/encryption are performed in-place, so this would trash subsequent uio or mbuf contents. This was introduced in r331639 (mea culpa). It only affects the {de,en}crypt_multi() family of interfaces. That interface only has one consumer transform in-tree (for now): Chacha20. PR: 227605 Submitted by: Valentin Vergez <valentin.vergez AT stormshield.eu> Changes: head/sys/opencrypto/cryptosoft.c
Thank you for the commit