| Message ID | 20230223090508.443157-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: tls: fix possible info leak in tls_set_device_offload() | expand |
On 2023/2/23 17:05, Hangyu Hua wrote: > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails > before kmalloc cctx->iv. This may cause info leak when we call > do_tls_getsockopt_conf(). Should we use kfree_sensitive() here if info leaking is what we want to avoid? > > Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > net/tls/tls_device.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > index 6c593788dc25..a63f6f727f58 100644 > --- a/net/tls/tls_device.c > +++ b/net/tls/tls_device.c > @@ -1241,8 +1241,10 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx) > kfree(start_marker_record); > free_rec_seq: > kfree(ctx->tx.rec_seq); > + ctx->tx.rec_seq = NULL; > free_iv: > kfree(ctx->tx.iv); > + ctx->tx.iv = NULL; > release_netdev: > dev_put(netdev); > return rc; >
2023-02-23, 17:05:08 +0800, Hangyu Hua wrote: > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails > before kmalloc cctx->iv. This may cause info leak when we call > do_tls_getsockopt_conf(). Is there really an issue here? If both tls_set_device_offload and tls_set_sw_offload fail, do_tls_setsockopt_conf will clear crypto_{send,recv} from the context. Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so we won't try to access iv or rec_seq.
On 23/2/2023 19:15, Sabrina Dubroca wrote: > 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote: >> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But >> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails >> before kmalloc cctx->iv. This may cause info leak when we call >> do_tls_getsockopt_conf(). > > Is there really an issue here? > > If both tls_set_device_offload and tls_set_sw_offload fail, > do_tls_setsockopt_conf will clear crypto_{send,recv} from the context. > Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so > we won't try to access iv or rec_seq. > My bad. I forget memzero_explicit. Then this is harmless. But I still think it is better to set them to NULL like tls_set_sw_offload's error path because we don't know there are another way to do this(I will change the commit log). What do you think?
On 24/2/2023 11:07, Hangyu Hua wrote: > On 23/2/2023 19:15, Sabrina Dubroca wrote: >> 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote: >>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But >>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it >>> fails >>> before kmalloc cctx->iv. This may cause info leak when we call >>> do_tls_getsockopt_conf(). >> >> Is there really an issue here? >> >> If both tls_set_device_offload and tls_set_sw_offload fail, >> do_tls_setsockopt_conf will clear crypto_{send,recv} from the context. >> Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so >> we won't try to access iv or rec_seq. >> > > My bad. I forget memzero_explicit. Then this is harmless. But I still > think it is better to set them to NULL like tls_set_sw_offload's error > path because we don't know there are another way to do this(I will > change the commit log). What do you think? Like a rare case, there is a race condition between do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not protected by lock_sock in do_tls_getsockopt_conf. It's just too difficult to satisfy both conditions at the same time.
2023-02-24, 11:33:29 +0800, Hangyu Hua wrote: > On 24/2/2023 11:07, Hangyu Hua wrote: > > On 23/2/2023 19:15, Sabrina Dubroca wrote: > > > 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote: > > > > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But > > > > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL > > > > if it fails > > > > before kmalloc cctx->iv. This may cause info leak when we call > > > > do_tls_getsockopt_conf(). > > > > > > Is there really an issue here? > > > > > > If both tls_set_device_offload and tls_set_sw_offload fail, > > > do_tls_setsockopt_conf will clear crypto_{send,recv} from the context. > > > Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so > > > we won't try to access iv or rec_seq. > > > > > > > My bad. I forget memzero_explicit. Then this is harmless. But I still > > think it is better to set them to NULL like tls_set_sw_offload's error > > path because we don't know there are another way to do this(I will > > change the commit log). What do you think? Yes, I guess for consistency between functions it would be ok. > Like a rare case, there is a race condition between > do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous > condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not > protected by lock_sock in do_tls_getsockopt_conf. It's just too > difficult to satisfy both conditions at the same time. Ugh, thanks for noticing this. We should move the lock_sock in getsockopt before TLS_CRYPTO_INFO_READY. Do you want to write that patch? Thanks.
On 24/2/2023 15:57, Sabrina Dubroca wrote: > 2023-02-24, 11:33:29 +0800, Hangyu Hua wrote: >> On 24/2/2023 11:07, Hangyu Hua wrote: >>> On 23/2/2023 19:15, Sabrina Dubroca wrote: >>>> 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote: >>>>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But >>>>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL >>>>> if it fails >>>>> before kmalloc cctx->iv. This may cause info leak when we call >>>>> do_tls_getsockopt_conf(). >>>> >>>> Is there really an issue here? >>>> >>>> If both tls_set_device_offload and tls_set_sw_offload fail, >>>> do_tls_setsockopt_conf will clear crypto_{send,recv} from the context. >>>> Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so >>>> we won't try to access iv or rec_seq. >>>> >>> >>> My bad. I forget memzero_explicit. Then this is harmless. But I still >>> think it is better to set them to NULL like tls_set_sw_offload's error >>> path because we don't know there are another way to do this(I will >>> change the commit log). What do you think? > > Yes, I guess for consistency between functions it would be ok. > >> Like a rare case, there is a race condition between >> do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous >> condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not >> protected by lock_sock in do_tls_getsockopt_conf. It's just too >> difficult to satisfy both conditions at the same time. > > Ugh, thanks for noticing this. We should move the lock_sock in > getsockopt before TLS_CRYPTO_INFO_READY. Do you want to write that > patch? > > Thanks. > I see. I will make a new patch to fix the race and send v2 of this. Thanks, Hangyu
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index 6c593788dc25..a63f6f727f58 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -1241,8 +1241,10 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx) kfree(start_marker_record); free_rec_seq: kfree(ctx->tx.rec_seq); + ctx->tx.rec_seq = NULL; free_iv: kfree(ctx->tx.iv); + ctx->tx.iv = NULL; release_netdev: dev_put(netdev); return rc;
After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails before kmalloc cctx->iv. This may cause info leak when we call do_tls_getsockopt_conf(). Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- net/tls/tls_device.c | 2 ++ 1 file changed, 2 insertions(+)