| Message ID | 20220124151146.376446-3-maximmi@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Bugfixes for syncookie BPF helpers | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 27 this patch: 27 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 22 this patch: 22 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 32 this patch: 32 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 37 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Maxim Mikityanskiy wrote: > bpf_tcp_gen_syncookie looks at the IP version in the IP header and > validates the address family of the socket. It supports IPv4 packets in > AF_INET6 dual-stack sockets. > > On the other hand, bpf_tcp_check_syncookie looks only at the address > family of the socket, ignoring the real IP version in headers, and > validates only the packet size. This implementation has some drawbacks: > > 1. Packets are not validated properly, allowing a BPF program to trick > bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4 > socket. These programs are all CAP_NET_ADMIN I believe so not so sure this is critical from a BPF program might trick the helper, but consistency is nice. > > 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end > up receiving a SYNACK with the cookie, but the following ACK gets > dropped. Agree we need to fix this. Also would be nice to add a test to capture this case so we don't break it again later. Its a bit subtle so might not be caught right away without a selftest. > > This patch fixes these issues by changing the checks in > bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP > version from the header is taken into account, and it is validated > properly with address family. Code looks good, would be nice to have a test. Acked-by: John Fastabend <john.fastabend@gmail.com> > > Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie") > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > --- > net/core/filter.c | 17 +++++++++++++----
On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > > bpf_tcp_gen_syncookie looks at the IP version in the IP header and > validates the address family of the socket. It supports IPv4 packets in > AF_INET6 dual-stack sockets. > > On the other hand, bpf_tcp_check_syncookie looks only at the address > family of the socket, ignoring the real IP version in headers, and > validates only the packet size. This implementation has some drawbacks: > > 1. Packets are not validated properly, allowing a BPF program to trick > bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4 > socket. > > 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end > up receiving a SYNACK with the cookie, but the following ACK gets > dropped. > > This patch fixes these issues by changing the checks in > bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP > version from the header is taken into account, and it is validated > properly with address family. > > Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie") > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > --- > net/core/filter.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 05efa691b796..780e635fb52a 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len > if (!th->ack || th->rst || th->syn) > return -ENOENT; > > + if (unlikely(iph_len < sizeof(struct iphdr))) > + return -EINVAL; > + > if (tcp_synq_no_recent_overflow(sk)) > return -ENOENT; > > cookie = ntohl(th->ack_seq) - 1; > > - switch (sk->sk_family) { > - case AF_INET: > - if (unlikely(iph_len < sizeof(struct iphdr))) > + /* Both struct iphdr and struct ipv6hdr have the version field at the > + * same offset so we can cast to the shorter header (struct iphdr). > + */ > + switch (((struct iphdr *)iph)->version) { > + case 4: > + if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk)) > return -EINVAL; Wouldn't this allow an arbitrary value for sk->sk_family, since there is no further check that sk_family is AF_INET?
On 2022-01-26 11:49, Lorenz Bauer wrote: > On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote: >> >> bpf_tcp_gen_syncookie looks at the IP version in the IP header and >> validates the address family of the socket. It supports IPv4 packets in >> AF_INET6 dual-stack sockets. >> >> On the other hand, bpf_tcp_check_syncookie looks only at the address >> family of the socket, ignoring the real IP version in headers, and >> validates only the packet size. This implementation has some drawbacks: >> >> 1. Packets are not validated properly, allowing a BPF program to trick >> bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4 >> socket. >> >> 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end >> up receiving a SYNACK with the cookie, but the following ACK gets >> dropped. >> >> This patch fixes these issues by changing the checks in >> bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP >> version from the header is taken into account, and it is validated >> properly with address family. >> >> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie") >> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> >> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> >> --- >> net/core/filter.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 05efa691b796..780e635fb52a 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len >> if (!th->ack || th->rst || th->syn) >> return -ENOENT; >> >> + if (unlikely(iph_len < sizeof(struct iphdr))) >> + return -EINVAL; >> + >> if (tcp_synq_no_recent_overflow(sk)) >> return -ENOENT; >> >> cookie = ntohl(th->ack_seq) - 1; >> >> - switch (sk->sk_family) { >> - case AF_INET: >> - if (unlikely(iph_len < sizeof(struct iphdr))) >> + /* Both struct iphdr and struct ipv6hdr have the version field at the >> + * same offset so we can cast to the shorter header (struct iphdr). >> + */ >> + switch (((struct iphdr *)iph)->version) { >> + case 4: >> + if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk)) >> return -EINVAL; > > Wouldn't this allow an arbitrary value for sk->sk_family, since there > is no further check that sk_family is AF_INET? It relies on the assumption that sk_family is either AF_INET or AF_INET6, when sk_protocol is IPPROTO_TCP (checked above). The same assumption is used in bpf_tcp_gen_syncookie. Do you think there are cases when it doesn't hold, and we must verify sk_family? If yes, then bpf_tcp_gen_syncookie should also be fixed.
diff --git a/net/core/filter.c b/net/core/filter.c index 05efa691b796..780e635fb52a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len if (!th->ack || th->rst || th->syn) return -ENOENT; + if (unlikely(iph_len < sizeof(struct iphdr))) + return -EINVAL; + if (tcp_synq_no_recent_overflow(sk)) return -ENOENT; cookie = ntohl(th->ack_seq) - 1; - switch (sk->sk_family) { - case AF_INET: - if (unlikely(iph_len < sizeof(struct iphdr))) + /* Both struct iphdr and struct ipv6hdr have the version field at the + * same offset so we can cast to the shorter header (struct iphdr). + */ + switch (((struct iphdr *)iph)->version) { + case 4: + if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk)) return -EINVAL; ret = __cookie_v4_check((struct iphdr *)iph, th, cookie); break; #if IS_BUILTIN(CONFIG_IPV6) - case AF_INET6: + case 6: if (unlikely(iph_len < sizeof(struct ipv6hdr))) return -EINVAL; + if (sk->sk_family != AF_INET6) + return -EINVAL; + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie); break; #endif /* CONFIG_IPV6 */
bpf_tcp_gen_syncookie looks at the IP version in the IP header and validates the address family of the socket. It supports IPv4 packets in AF_INET6 dual-stack sockets. On the other hand, bpf_tcp_check_syncookie looks only at the address family of the socket, ignoring the real IP version in headers, and validates only the packet size. This implementation has some drawbacks: 1. Packets are not validated properly, allowing a BPF program to trick bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4 socket. 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end up receiving a SYNACK with the cookie, but the following ACK gets dropped. This patch fixes these issues by changing the checks in bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP version from the header is taken into account, and it is validated properly with address family. Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie") Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> --- net/core/filter.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)