The original patches fixing CVE-2023-1076 are incorrect in my opinion. This small series fixes them up; see the individual commit messages for explanation.
I have a very elaborate test procedure demonstrating the problem for both tun and tap; it involves libvirt, qemu, and "crash". I can share that procedure if necessary, but it's indeed quite long (I wrote it originally for our QE team).
The patches in this series are supposed to "re-fix" CVE-2023-1076; given that said CVE is classified as Low Impact (CVSSv3=5.5), I'm posting this publicly, and not suggesting any embargo. Red Hat Product Security may assign a new CVE number later.
I've tested the patches on top of v6.5-rc4, with "crash" built at commit c74f375e0ef7.
Cc: Eric Dumazet edumazet@google.com Cc: Lorenzo Colitti lorenzo@google.com Cc: Paolo Abeni pabeni@redhat.com Cc: Pietro Borrello borrello@diag.uniroma1.it Cc: netdev@vger.kernel.org Cc: stable@vger.kernel.org
Laszlo Ersek (2): net: tun_chr_open(): set sk_uid from current_fsuid() net: tap_open(): set sk_uid from current_fsuid()
drivers/net/tap.c | 2 +- drivers/net/tun.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
Commit a096ccca6e50 initializes the "sk_uid" field in the protocol socket (struct sock) from the "/dev/net/tun" device node's owner UID. Per original commit 86741ec25462 ("net: core: Add a UID field to struct sock.", 2016-11-04), that's wrong: the idea is to cache the UID of the userspace process that creates the socket. Commit 86741ec25462 mentions socket() and accept(); with "tun", the action that creates the socket is open("/dev/net/tun").
Therefore the device node's owner UID is irrelevant. In most cases, "/dev/net/tun" will be owned by root, so in practice, commit a096ccca6e50 has no observable effect:
- before, "sk_uid" would be zero, due to undefined behavior (CVE-2023-1076),
- after, "sk_uid" would be zero, due to "/dev/net/tun" being owned by root.
What matters is the (fs)UID of the process performing the open(), so cache that in "sk_uid".
Cc: Eric Dumazet edumazet@google.com Cc: Lorenzo Colitti lorenzo@google.com Cc: Paolo Abeni pabeni@redhat.com Cc: Pietro Borrello borrello@diag.uniroma1.it Cc: netdev@vger.kernel.org Cc: stable@vger.kernel.org Fixes: a096ccca6e50 ("tun: tun_chr_open(): correctly initialize socket uid") Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2173435 Signed-off-by: Laszlo Ersek lersek@redhat.com --- drivers/net/tun.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index d75456adc62a..25f0191df00b 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -3469,7 +3469,7 @@ static int tun_chr_open(struct inode *inode, struct file * file) tfile->socket.file = file; tfile->socket.ops = &tun_socket_ops;
- sock_init_data_uid(&tfile->socket, &tfile->sk, inode->i_uid); + sock_init_data_uid(&tfile->socket, &tfile->sk, current_fsuid());
tfile->sk.sk_write_space = tun_sock_write_space; tfile->sk.sk_sndbuf = INT_MAX;
Commit 66b2c338adce initializes the "sk_uid" field in the protocol socket (struct sock) from the "/dev/tapX" device node's owner UID. Per original commit 86741ec25462 ("net: core: Add a UID field to struct sock.", 2016-11-04), that's wrong: the idea is to cache the UID of the userspace process that creates the socket. Commit 86741ec25462 mentions socket() and accept(); with "tap", the action that creates the socket is open("/dev/tapX").
Therefore the device node's owner UID is irrelevant. In most cases, "/dev/tapX" will be owned by root, so in practice, commit 66b2c338adce has no observable effect:
- before, "sk_uid" would be zero, due to undefined behavior (CVE-2023-1076),
- after, "sk_uid" would be zero, due to "/dev/tapX" being owned by root.
What matters is the (fs)UID of the process performing the open(), so cache that in "sk_uid".
Cc: Eric Dumazet edumazet@google.com Cc: Lorenzo Colitti lorenzo@google.com Cc: Paolo Abeni pabeni@redhat.com Cc: Pietro Borrello borrello@diag.uniroma1.it Cc: netdev@vger.kernel.org Cc: stable@vger.kernel.org Fixes: 66b2c338adce ("tap: tap_open(): correctly initialize socket uid") Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2173435 Signed-off-by: Laszlo Ersek lersek@redhat.com --- drivers/net/tap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 9137fb8c1c42..49d1d6acf95e 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -534,7 +534,7 @@ static int tap_open(struct inode *inode, struct file *file) q->sock.state = SS_CONNECTED; q->sock.file = file; q->sock.ops = &tap_socket_ops; - sock_init_data_uid(&q->sock, &q->sk, inode->i_uid); + sock_init_data_uid(&q->sock, &q->sk, current_fsuid()); q->sk.sk_write_space = tap_sock_write_space; q->sk.sk_destruct = tap_sock_destruct; q->flags = IFF_VNET_HDR | IFF_NO_PI | IFF_TAP;
On Mon, 31 Jul 2023 18:42:35 +0200 Laszlo Ersek wrote:
The original patches fixing CVE-2023-1076 are incorrect in my opinion. This small series fixes them up; see the individual commit messages for explanation.
I have a very elaborate test procedure demonstrating the problem for both tun and tap; it involves libvirt, qemu, and "crash". I can share that procedure if necessary, but it's indeed quite long (I wrote it originally for our QE team).
The patches in this series are supposed to "re-fix" CVE-2023-1076; given that said CVE is classified as Low Impact (CVSSv3=5.5), I'm posting this publicly, and not suggesting any embargo. Red Hat Product Security may assign a new CVE number later.
I've tested the patches on top of v6.5-rc4, with "crash" built at commit c74f375e0ef7.
FTR this was applied yesterday to net. Thanks!
linux-stable-mirror@lists.linaro.org
-
Jakub Kicinski -
Laszlo Ersek