| Message ID | 20190724100204.2009-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes() | expand |
[+Cc linux-crypto] On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote: > In derive_key_aes(), tfm is assigned to NULL on line 46, and then > crypto_free_skcipher(tfm) is executed. > > crypto_free_skcipher(tfm) > crypto_skcipher_tfm(tfm) > return &tfm->base; > > Thus, a possible null-pointer dereference may occur. This analysis is incorrect because only the address &tfm->base is taken. There's no pointer dereference. In fact all the crypto_free_*() functions are no-ops on NULL pointers, and many other callers rely on it. So there's no bug here. It appears you've sent the same patch for some of these other callers (https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22), but none are Cc'ed to linux-crypto or another mailing list I'm subscribed to, so I can't respond to them. But this feedback applies equally to them too. Note also that if there actually were a bug here (which again, there doesn't appear to be), we'd need to fix it in crypto_free_*(), not in the callers. - Eric
On 2019/7/25 0:07, Eric Biggers wrote: > [+Cc linux-crypto] > > On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote: >> In derive_key_aes(), tfm is assigned to NULL on line 46, and then >> crypto_free_skcipher(tfm) is executed. >> >> crypto_free_skcipher(tfm) >> crypto_skcipher_tfm(tfm) >> return &tfm->base; >> >> Thus, a possible null-pointer dereference may occur. > This analysis is incorrect because only the address &tfm->base is taken. > There's no pointer dereference. > > In fact all the crypto_free_*() functions are no-ops on NULL pointers, and many > other callers rely on it. So there's no bug here. Thanks for the reply :) I admit that "&tfm->base" is not a null-pointer dereference when tfm is NULL. But I still think crypto_free_skcipher(tfm) can cause security problems when tfm is NULL. Looking at the code: static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) { crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); } static inline struct crypto_tfm *crypto_skcipher_tfm( struct crypto_skcipher *tfm) { return &tfm->base; } void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) { struct crypto_alg *alg; if (unlikely(!mem)) return; alg = tfm->__crt_alg; if (!tfm->exit && alg->cra_exit) alg->cra_exit(tfm); crypto_exit_ops(tfm); crypto_mod_put(alg); kzfree(mem); } The function crypto_skcipher_tfm() may return an uninitialized address (&tfm->base) when tfm is NULL. Then crypto_destroy_tfm() uses this problematic address (tfm), which may cause security problems. Besides, I also find that some kernel modules check tfm before calling crypto_free_*(), such as: drivers/crypto/vmx/aes_xts.c: if (ctx->fallback) { crypto_free_skcipher(ctx->fallback); ctx->fallback = NULL; } net/rxrpc/rxkad.c: if (conn->cipher) crypto_free_skcipher(conn->cipher); drivers/crypto/chelsio/chcr_algo.c: if (ablkctx->aes_generic) crypto_free_cipher(ablkctx->aes_generic); net/mac80211/wep.c: if (!IS_ERR(local->wep_tx_tfm)) crypto_free_cipher(local->wep_tx_tfm); Thus, I think it is better to check tfm before calling crypto_free_*(). > > It appears you've sent the same patch for some of these other callers > (https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22), but none > are Cc'ed to linux-crypto or another mailing list I'm subscribed to, so I can't > respond to them. But this feedback applies equally to them too. Ah, sorry. I just ran "get_maintainer.pl" for the kernel modules used crypto_free_*(), and forgot to cc to linux-crypto... > > Note also that if there actually were a bug here (which again, there doesn't > appear to be), we'd need to fix it in crypto_free_*(), not in the callers. > I think a possible way is to add a check of tfm in crypto_free_*(), such as: static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) { if (tfm) crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); } If you think it is okay, I can send a patch for this. Best wishes, Jia-Ju Bai
Sorry, I forgot to send to Eric, so send it again. On 2019/7/25 11:30, Jia-Ju Bai wrote: > > > On 2019/7/25 0:07, Eric Biggers wrote: >> [+Cc linux-crypto] >> >> On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote: >>> In derive_key_aes(), tfm is assigned to NULL on line 46, and then >>> crypto_free_skcipher(tfm) is executed. >>> >>> crypto_free_skcipher(tfm) >>> crypto_skcipher_tfm(tfm) >>> return &tfm->base; >>> >>> Thus, a possible null-pointer dereference may occur. >> This analysis is incorrect because only the address &tfm->base is taken. >> There's no pointer dereference. >> >> In fact all the crypto_free_*() functions are no-ops on NULL >> pointers, and many >> other callers rely on it. So there's no bug here. > > Thanks for the reply :) > I admit that "&tfm->base" is not a null-pointer dereference when tfm > is NULL. > But I still think crypto_free_skcipher(tfm) can cause security > problems when tfm is NULL. > > Looking at the code: > > static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) > { > crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); > } > > static inline struct crypto_tfm *crypto_skcipher_tfm( > struct crypto_skcipher *tfm) > { > return &tfm->base; > } > > void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) > { > struct crypto_alg *alg; > > if (unlikely(!mem)) > return; > > alg = tfm->__crt_alg; > > if (!tfm->exit && alg->cra_exit) > alg->cra_exit(tfm); > crypto_exit_ops(tfm); > crypto_mod_put(alg); > kzfree(mem); > } > > The function crypto_skcipher_tfm() may return an uninitialized address > (&tfm->base) when tfm is NULL. > Then crypto_destroy_tfm() uses this problematic address (tfm), which > may cause security problems. > > Besides, I also find that some kernel modules check tfm before calling > crypto_free_*(), such as: > > drivers/crypto/vmx/aes_xts.c: > if (ctx->fallback) { > crypto_free_skcipher(ctx->fallback); > ctx->fallback = NULL; > } > > net/rxrpc/rxkad.c: > if (conn->cipher) > crypto_free_skcipher(conn->cipher); > > drivers/crypto/chelsio/chcr_algo.c: > if (ablkctx->aes_generic) > crypto_free_cipher(ablkctx->aes_generic); > > net/mac80211/wep.c: > if (!IS_ERR(local->wep_tx_tfm)) > crypto_free_cipher(local->wep_tx_tfm); > > Thus, I think it is better to check tfm before calling crypto_free_*(). > >> >> It appears you've sent the same patch for some of these other callers >> (https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22), >> but none >> are Cc'ed to linux-crypto or another mailing list I'm subscribed to, >> so I can't >> respond to them. But this feedback applies equally to them too. > > Ah, sorry. > I just ran "get_maintainer.pl" for the kernel modules used > crypto_free_*(), and forgot to cc to linux-crypto... > >> >> Note also that if there actually were a bug here (which again, there >> doesn't >> appear to be), we'd need to fix it in crypto_free_*(), not in the >> callers. >> > > I think a possible way is to add a check of tfm in crypto_free_*(), > such as: > static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) > { > if (tfm) > crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); > } > > If you think it is okay, I can send a patch for this. > > > Best wishes, > Jia-Ju Bai
On Thu, Jul 25, 2019 at 11:33:53AM +0800, Jia-Ju Bai wrote: > Sorry, I forgot to send to Eric, so send it again. > > On 2019/7/25 11:30, Jia-Ju Bai wrote: > > > > > > On 2019/7/25 0:07, Eric Biggers wrote: > > > [+Cc linux-crypto] > > > > > > On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote: > > > > In derive_key_aes(), tfm is assigned to NULL on line 46, and then > > > > crypto_free_skcipher(tfm) is executed. > > > > > > > > crypto_free_skcipher(tfm) > > > > crypto_skcipher_tfm(tfm) > > > > return &tfm->base; > > > > > > > > Thus, a possible null-pointer dereference may occur. > > > This analysis is incorrect because only the address &tfm->base is taken. > > > There's no pointer dereference. > > > > > > In fact all the crypto_free_*() functions are no-ops on NULL > > > pointers, and many > > > other callers rely on it. So there's no bug here. > > > > Thanks for the reply :) > > I admit that "&tfm->base" is not a null-pointer dereference when tfm is > > NULL. > > But I still think crypto_free_skcipher(tfm) can cause security problems > > when tfm is NULL. > > > > Looking at the code: > > > > static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) > > { > > crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); > > } > > > > static inline struct crypto_tfm *crypto_skcipher_tfm( > > struct crypto_skcipher *tfm) > > { > > return &tfm->base; > > } > > > > void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) > > { > > struct crypto_alg *alg; > > > > if (unlikely(!mem)) > > return; When the original pointer is NULL, mem == NULL here so crypto_destroy_tfm() is a no-op. > > Besides, I also find that some kernel modules check tfm before calling > > crypto_free_*(), such as: > > > > drivers/crypto/vmx/aes_xts.c: > > if (ctx->fallback) { > > crypto_free_skcipher(ctx->fallback); > > ctx->fallback = NULL; > > } > > > > net/rxrpc/rxkad.c: > > if (conn->cipher) > > crypto_free_skcipher(conn->cipher); > > > > drivers/crypto/chelsio/chcr_algo.c: > > if (ablkctx->aes_generic) > > crypto_free_cipher(ablkctx->aes_generic); > > > > net/mac80211/wep.c: > > if (!IS_ERR(local->wep_tx_tfm)) > > crypto_free_cipher(local->wep_tx_tfm); > > Well, people sometimes do that for kfree() too. But that doesn't mean it's needed, or that it's the preferred style (it's not). - Eric
On 2019/7/25 11:39, Eric Biggers wrote: > On Thu, Jul 25, 2019 at 11:33:53AM +0800, Jia-Ju Bai wrote: >> Sorry, I forgot to send to Eric, so send it again. >> >> On 2019/7/25 11:30, Jia-Ju Bai wrote: >>> >>> On 2019/7/25 0:07, Eric Biggers wrote: >>>> [+Cc linux-crypto] >>>> >>>> On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote: >>>>> In derive_key_aes(), tfm is assigned to NULL on line 46, and then >>>>> crypto_free_skcipher(tfm) is executed. >>>>> >>>>> crypto_free_skcipher(tfm) >>>>> crypto_skcipher_tfm(tfm) >>>>> return &tfm->base; >>>>> >>>>> Thus, a possible null-pointer dereference may occur. >>>> This analysis is incorrect because only the address &tfm->base is taken. >>>> There's no pointer dereference. >>>> >>>> In fact all the crypto_free_*() functions are no-ops on NULL >>>> pointers, and many >>>> other callers rely on it. So there's no bug here. >>> Thanks for the reply :) >>> I admit that "&tfm->base" is not a null-pointer dereference when tfm is >>> NULL. >>> But I still think crypto_free_skcipher(tfm) can cause security problems >>> when tfm is NULL. >>> >>> Looking at the code: >>> >>> static inline void crypto_free_skcipher(struct crypto_skcipher *tfm) >>> { >>> crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm)); >>> } >>> >>> static inline struct crypto_tfm *crypto_skcipher_tfm( >>> struct crypto_skcipher *tfm) >>> { >>> return &tfm->base; >>> } >>> >>> void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) >>> { >>> struct crypto_alg *alg; >>> >>> if (unlikely(!mem)) >>> return; > When the original pointer is NULL, mem == NULL here so crypto_destroy_tfm() is a > no-op. I overlooked this if statement, thanks for the pointer. > >>> Besides, I also find that some kernel modules check tfm before calling >>> crypto_free_*(), such as: >>> >>> drivers/crypto/vmx/aes_xts.c: >>> if (ctx->fallback) { >>> crypto_free_skcipher(ctx->fallback); >>> ctx->fallback = NULL; >>> } >>> >>> net/rxrpc/rxkad.c: >>> if (conn->cipher) >>> crypto_free_skcipher(conn->cipher); >>> >>> drivers/crypto/chelsio/chcr_algo.c: >>> if (ablkctx->aes_generic) >>> crypto_free_cipher(ablkctx->aes_generic); >>> >>> net/mac80211/wep.c: >>> if (!IS_ERR(local->wep_tx_tfm)) >>> crypto_free_cipher(local->wep_tx_tfm); >>> > Well, people sometimes do that for kfree() too. But that doesn't mean it's > needed, or that it's the preferred style (it's not). Okay. Best wishes, Jia-Ju Bai
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 207ebed918c1..b419720cac54 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -66,7 +66,8 @@ static int derive_key_aes(const u8 *master_key, res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait); out: skcipher_request_free(req); - crypto_free_skcipher(tfm); + if (tfm) + crypto_free_skcipher(tfm); return res; }
In derive_key_aes(), tfm is assigned to NULL on line 46, and then crypto_free_skcipher(tfm) is executed. crypto_free_skcipher(tfm) crypto_skcipher_tfm(tfm) return &tfm->base; Thus, a possible null-pointer dereference may occur. To fix this bug, tfm is checked before calling crypto_free_skcipher(). This bug is found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- fs/crypto/keyinfo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)