| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89iJQvO11pRqrsHpB5Y3NuurX1UcEFu_SYq7kxJsvW7y7BQ@mail.gmail.com>
Date: Mon, 30 Oct 2023 09:16:08 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Dmitry Safonov <dima@...sta.com>
Cc: David Ahern <dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, linux-kernel@...r.kernel.org,
Andy Lutomirski <luto@...capital.net>, Ard Biesheuvel <ardb@...nel.org>,
Bob Gilligan <gilligan@...sta.com>, Dan Carpenter <error27@...il.com>,
David Laight <David.Laight@...lab.com>, Dmitry Safonov <0x7f454c46@...il.com>,
Donald Cassidy <dcassidy@...hat.com>, Eric Biggers <ebiggers@...nel.org>,
"Eric W. Biederman" <ebiederm@...ssion.com>, Francesco Ruggeri <fruggeri05@...il.com>,
"Gaillardetz, Dominik" <dgaillar@...na.com>, Herbert Xu <herbert@...dor.apana.org.au>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, Ivan Delalande <colona@...sta.com>,
Leonard Crestez <cdleonard@...il.com>, "Nassiri, Mohammad" <mnassiri@...na.com>,
Salam Noureddine <noureddine@...sta.com>, Simon Horman <horms@...nel.org>,
"Tetreault, Francois" <ftetreau@...na.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v16 net-next 15/23] net/tcp: Add tcp_hash_fail()
ratelimited logs
On Mon, Oct 23, 2023 at 9:22 PM Dmitry Safonov <dima@...sta.com> wrote:
>
> Add a helper for logging connection-detailed messages for failed TCP
> hash verification (both MD5 and AO).
>
> Co-developed-by: Francesco Ruggeri <fruggeri@...sta.com>
> Signed-off-by: Francesco Ruggeri <fruggeri@...sta.com>
> Co-developed-by: Salam Noureddine <noureddine@...sta.com>
> Signed-off-by: Salam Noureddine <noureddine@...sta.com>
> Signed-off-by: Dmitry Safonov <dima@...sta.com>
> Acked-by: David Ahern <dsahern@...nel.org>
> ---
> include/net/tcp.h | 14 ++++++++++++--
> include/net/tcp_ao.h | 29 +++++++++++++++++++++++++++++
> net/ipv4/tcp.c | 23 +++++++++++++----------
> net/ipv4/tcp_ao.c | 7 +++++++
> 4 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index d29c8a867f0e..c93ac6cc12c4 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2746,12 +2746,18 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
> int l3index;
>
> /* Invalid option or two times meet any of auth options */
> - if (tcp_parse_auth_options(th, &md5_location, &aoh))
> + if (tcp_parse_auth_options(th, &md5_location, &aoh)) {
> + tcp_hash_fail("TCP segment has incorrect auth options set",
> + family, skb, "");
> return SKB_DROP_REASON_TCP_AUTH_HDR;
> + }
>
> if (req) {
> if (tcp_rsk_used_ao(req) != !!aoh) {
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
> + tcp_hash_fail("TCP connection can't start/end using TCP-AO",
> + family, skb, "%s",
> + !aoh ? "missing AO" : "AO signed");
> return SKB_DROP_REASON_TCP_AOFAILURE;
> }
> }
> @@ -2768,10 +2774,14 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
> * the last key is impossible to remove, so there's
> * always at least one current_key.
> */
> - if (tcp_ao_required(sk, saddr, family, true))
> + if (tcp_ao_required(sk, saddr, family, true)) {
> + tcp_hash_fail("AO hash is required, but not found",
> + family, skb, "L3 index %d", l3index);
> return SKB_DROP_REASON_TCP_AONOTFOUND;
> + }
> if (unlikely(tcp_md5_do_lookup(sk, l3index, saddr, family))) {
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5NOTFOUND);
> + tcp_hash_fail("MD5 Hash not found", family, skb, "");
> return SKB_DROP_REASON_TCP_MD5NOTFOUND;
> }
> return SKB_NOT_DROPPED_YET;
> diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
> index 0c3516d1b968..4da6e3657913 100644
> --- a/include/net/tcp_ao.h
> +++ b/include/net/tcp_ao.h
> @@ -118,6 +118,35 @@ struct tcp_ao_info {
> struct rcu_head rcu;
> };
>
> +#define tcp_hash_fail(msg, family, skb, fmt, ...) \
> +do { \
> + const struct tcphdr *th = tcp_hdr(skb); \
> + char hdr_flags[5] = {}; \
> + char *f = hdr_flags; \
> + \
> + if (th->fin) \
> + *f++ = 'F'; \
> + if (th->syn) \
> + *f++ = 'S'; \
> + if (th->rst) \
> + *f++ = 'R'; \
> + if (th->ack) \
> + *f++ = 'A'; \
> + if (f != hdr_flags) \
> + *f = ' '; \
I see no null termination of hdr_flags[] if FIN+SYN+ACK+RST are all set.
I will send something like:
diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index a375a171ef3cb37ab1d8246c72c6a3e83f5c9184..5daf96a3dbee14bd3786e19ea4972e351058e6e7
100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -124,7 +124,7 @@ struct tcp_ao_info {
#define tcp_hash_fail(msg, family, skb, fmt, ...) \
do { \
const struct tcphdr *th = tcp_hdr(skb); \
- char hdr_flags[5] = {}; \
+ char hdr_flags[5]; \
char *f = hdr_flags; \
\
if (th->fin) \
@@ -135,8 +135,7 @@ do {
\
*f++ = 'R'; \
if (th->ack) \
*f++ = 'A'; \
- if (f != hdr_flags) \
- *f = ' '; \
+ *f = 0; \
if ((family) == AF_INET) { \
net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d)
%s" fmt "\n", \
msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
Powered by blists - more mailing lists