[PATCH v1 net 0/2] dccp/tcp: Relocate security_inet_conn_request().

[PATCH v1 net 0/2] dccp/tcp: Relocate security_inet_conn_request().

[Kuniyuki Iwashima]

security_inet_conn_request() reads reqsk's remote address, but it's not
initialised in some places.

Let's make sure the address is set before security_inet_conn_request().


Kuniyuki Iwashima (2):
  dccp: Call security_inet_conn_request() after setting IPv4 addresses.
  dccp/tcp: Call security_inet_conn_request() after setting IPv6
    addresses.

 net/dccp/ipv4.c       | 6 +++---
 net/dccp/ipv6.c       | 6 +++---
 net/ipv6/syncookies.c | 7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.30.2

[PATCH v1 net 1/2] dccp: Call security_inet_conn_request() after setting IPv4 addresses.

[Kuniyuki Iwashima]

Initially, commit 4237c75c0a35 ("[MLSXFRM]: Auto-labeling of child
sockets") introduced security_inet_conn_request() in some functions
where reqsk is allocated.  The hook is added just after the allocation,
so reqsk's IPv4 remote address was not initialised then.

However, SELinux/Smack started to read it in netlbl_req_setattr()
after the cited commits.

This bug was partially fixed by commit 284904aa7946 ("lsm: Relocate
the IPv4 security_inet_conn_request() hooks").

This patch fixes the last bug in DCCPv4.

Fixes: 389fb800ac8b ("netlabel: Label incoming TCP connections correctly in SELinux")
Fixes: 07feee8f812f ("netlabel: Cleanup the Smack/NetLabel code to fix incoming TCP connections")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: James Morris <jmorris@namei.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Paul Moore <paul.moore@hp.com>
---
 net/dccp/ipv4.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 1b8cbfda6e5d..44b033fe1ef6 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -629,9 +629,6 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	if (dccp_parse_options(sk, dreq, skb))
 		goto drop_and_free;
 
-	if (security_inet_conn_request(sk, skb, req))
-		goto drop_and_free;
-
 	ireq = inet_rsk(req);
 	sk_rcv_saddr_set(req_to_sk(req), ip_hdr(skb)->daddr);
 	sk_daddr_set(req_to_sk(req), ip_hdr(skb)->saddr);
@@ -639,6 +636,9 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	ireq->ireq_family = AF_INET;
 	ireq->ir_iif = READ_ONCE(sk->sk_bound_dev_if);
 
+	if (security_inet_conn_request(sk, skb, req))
+		goto drop_and_free;
+
 	/*
 	 * Step 3: Process LISTEN state
 	 *
-- 
2.30.2

[PATCH v1 net 2/2] dccp/tcp: Call security_inet_conn_request() after setting IPv6 addresses.

[Kuniyuki Iwashima]

Initially, commit 4237c75c0a35 ("[MLSXFRM]: Auto-labeling of child
sockets") introduced security_inet_conn_request() in some functions
where reqsk is allocated.  The hook is added just after the allocation,
so reqsk's IPv6 remote address was not initialised then.

However, SELinux/Smack started to read it in netlbl_req_setattr()
after commit e1adea927080 ("calipso: Allow request sockets to be
relabelled by the lsm.").

Commit 284904aa7946 ("lsm: Relocate the IPv4 security_inet_conn_request()
hooks") fixed that kind of issue only in TCPv4 because IPv6 labeling was
not supported at that time.  Finally, the same issue was introduced again
in IPv6.

Let's apply the same fix on DCCPv6 and TCPv6.

Fixes: e1adea927080 ("calipso: Allow request sockets to be relabelled by the lsm.")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Huw Davies <huw@codeweavers.com>
Cc: Paul Moore <paul@paul-moore.com>
---
 net/dccp/ipv6.c       | 6 +++---
 net/ipv6/syncookies.c | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 8d344b219f84..4550b680665a 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -360,15 +360,15 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	if (dccp_parse_options(sk, dreq, skb))
 		goto drop_and_free;
 
-	if (security_inet_conn_request(sk, skb, req))
-		goto drop_and_free;
-
 	ireq = inet_rsk(req);
 	ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
 	ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;
 	ireq->ireq_family = AF_INET6;
 	ireq->ir_mark = inet_request_mark(sk, skb);
 
+	if (security_inet_conn_request(sk, skb, req))
+		goto drop_and_free;
+
 	if (ipv6_opt_accepted(sk, skb, IP6CB(skb)) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
 	    np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 500f6ed3b8cf..12eedc6ca2cc 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -181,14 +181,15 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	treq = tcp_rsk(req);
 	treq->tfo_listener = false;
 
-	if (security_inet_conn_request(sk, skb, req))
-		goto out_free;
-
 	req->mss = mss;
 	ireq->ir_rmt_port = th->source;
 	ireq->ir_num = ntohs(th->dest);
 	ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
 	ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;
+
+	if (security_inet_conn_request(sk, skb, req))
+		goto out_free;
+
 	if (ipv6_opt_accepted(sk, skb, &TCP_SKB_CB(skb)->header.h6) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
 	    np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
-- 
2.30.2

Re: [PATCH v1 net 2/2] dccp/tcp: Call security_inet_conn_request() after setting IPv6 addresses.

[Paul Moore]

On Mon, Oct 30, 2023 at 4:12 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Initially, commit 4237c75c0a35 ("[MLSXFRM]: Auto-labeling of child
> sockets") introduced security_inet_conn_request() in some functions
> where reqsk is allocated.  The hook is added just after the allocation,
> so reqsk's IPv6 remote address was not initialised then.
>
> However, SELinux/Smack started to read it in netlbl_req_setattr()
> after commit e1adea927080 ("calipso: Allow request sockets to be
> relabelled by the lsm.").
>
> Commit 284904aa7946 ("lsm: Relocate the IPv4 security_inet_conn_request()
> hooks") fixed that kind of issue only in TCPv4 because IPv6 labeling was
> not supported at that time.  Finally, the same issue was introduced again
> in IPv6.
>
> Let's apply the same fix on DCCPv6 and TCPv6.
>
> Fixes: e1adea927080 ("calipso: Allow request sockets to be relabelled by the lsm.")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Cc: Huw Davies <huw@codeweavers.com>
> Cc: Paul Moore <paul@paul-moore.com>
> ---
>  net/dccp/ipv6.c       | 6 +++---
>  net/ipv6/syncookies.c | 7 ++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)

Thanks for catching this and submitting a patch!

It seems like we should also update dccp_v4_conn_request(), what do you think?

> diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> index 8d344b219f84..4550b680665a 100644
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -360,15 +360,15 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
>         if (dccp_parse_options(sk, dreq, skb))
>                 goto drop_and_free;
>
> -       if (security_inet_conn_request(sk, skb, req))
> -               goto drop_and_free;
> -
>         ireq = inet_rsk(req);
>         ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
>         ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;
>         ireq->ireq_family = AF_INET6;
>         ireq->ir_mark = inet_request_mark(sk, skb);
>
> +       if (security_inet_conn_request(sk, skb, req))
> +               goto drop_and_free;
> +
>         if (ipv6_opt_accepted(sk, skb, IP6CB(skb)) ||
>             np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
>             np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index 500f6ed3b8cf..12eedc6ca2cc 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -181,14 +181,15 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>         treq = tcp_rsk(req);
>         treq->tfo_listener = false;
>
> -       if (security_inet_conn_request(sk, skb, req))
> -               goto out_free;
> -
>         req->mss = mss;
>         ireq->ir_rmt_port = th->source;
>         ireq->ir_num = ntohs(th->dest);
>         ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
>         ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;
> +
> +       if (security_inet_conn_request(sk, skb, req))
> +               goto out_free;
> +
>         if (ipv6_opt_accepted(sk, skb, &TCP_SKB_CB(skb)->header.h6) ||
>             np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
>             np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
> --
> 2.30.2

-- 
paul-moore.com

Re: [PATCH v1 net 2/2] dccp/tcp: Call security_inet_conn_request() after setting IPv6 addresses.

[Kuniyuki Iwashima]

From: Paul Moore <paul@paul-moore.com>
Date: Mon, 30 Oct 2023 17:12:33 -0400
> On Mon, Oct 30, 2023 at 4:12 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > Initially, commit 4237c75c0a35 ("[MLSXFRM]: Auto-labeling of child
> > sockets") introduced security_inet_conn_request() in some functions
> > where reqsk is allocated.  The hook is added just after the allocation,
> > so reqsk's IPv6 remote address was not initialised then.
> >
> > However, SELinux/Smack started to read it in netlbl_req_setattr()
> > after commit e1adea927080 ("calipso: Allow request sockets to be
> > relabelled by the lsm.").
> >
> > Commit 284904aa7946 ("lsm: Relocate the IPv4 security_inet_conn_request()
> > hooks") fixed that kind of issue only in TCPv4 because IPv6 labeling was
> > not supported at that time.  Finally, the same issue was introduced again
> > in IPv6.
> >
> > Let's apply the same fix on DCCPv6 and TCPv6.
> >
> > Fixes: e1adea927080 ("calipso: Allow request sockets to be relabelled by the lsm.")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> > Cc: Huw Davies <huw@codeweavers.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > ---
> >  net/dccp/ipv6.c       | 6 +++---
> >  net/ipv6/syncookies.c | 7 ++++---
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> Thanks for catching this and submitting a patch!
> 
> It seems like we should also update dccp_v4_conn_request(), what do you think?

Yes, and it's done in patch 1 as it had a separate Fixes tag.
https://lore.kernel.org/netdev/20231030201042.32885-2-kuniyu@amazon.com/

It seems get_maintainers.pl suggested another email address of
yours for patch 1.  It would be good to update .mailmap ;)

Thanks!

Re: [PATCH v1 net 2/2] dccp/tcp: Call security_inet_conn_request() after setting IPv6 addresses.

[Paul Moore]

On Mon, Oct 30, 2023 at 5:20 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Mon, 30 Oct 2023 17:12:33 -0400
> > On Mon, Oct 30, 2023 at 4:12 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > >
> > > Initially, commit 4237c75c0a35 ("[MLSXFRM]: Auto-labeling of child
> > > sockets") introduced security_inet_conn_request() in some functions
> > > where reqsk is allocated.  The hook is added just after the allocation,
> > > so reqsk's IPv6 remote address was not initialised then.
> > >
> > > However, SELinux/Smack started to read it in netlbl_req_setattr()
> > > after commit e1adea927080 ("calipso: Allow request sockets to be
> > > relabelled by the lsm.").
> > >
> > > Commit 284904aa7946 ("lsm: Relocate the IPv4 security_inet_conn_request()
> > > hooks") fixed that kind of issue only in TCPv4 because IPv6 labeling was
> > > not supported at that time.  Finally, the same issue was introduced again
> > > in IPv6.
> > >
> > > Let's apply the same fix on DCCPv6 and TCPv6.
> > >
> > > Fixes: e1adea927080 ("calipso: Allow request sockets to be relabelled by the lsm.")
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > > ---
> > > Cc: Huw Davies <huw@codeweavers.com>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > ---
> > >  net/dccp/ipv6.c       | 6 +++---
> > >  net/ipv6/syncookies.c | 7 ++++---
> > >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > Thanks for catching this and submitting a patch!
> >
> > It seems like we should also update dccp_v4_conn_request(), what do you think?
>
> Yes, and it's done in patch 1 as it had a separate Fixes tag.
> https://lore.kernel.org/netdev/20231030201042.32885-2-kuniyu@amazon.com/

Great, thanks for doing that.  netdev folks, please feel free to add
my ACK to both patches in the patchset.

Acked-by: Paul Moore <paul@paul-moore.com>

> It seems get_maintainers.pl suggested another email address of
> yours for patch 1.  It would be good to update .mailmap ;)

Yes, I really should, thanks for the reminder.  I'll send an update to
Linus once I get the merge window PRs sorted out.

-- 
paul-moore.com

Re: [PATCH v1 net 0/2] dccp/tcp: Relocate security_inet_conn_request().

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 30 Oct 2023 13:10:40 -0700 you wrote:
> security_inet_conn_request() reads reqsk's remote address, but it's not
> initialised in some places.
> 
> Let's make sure the address is set before security_inet_conn_request().
> 
> 
> Kuniyuki Iwashima (2):
>   dccp: Call security_inet_conn_request() after setting IPv4 addresses.
>   dccp/tcp: Call security_inet_conn_request() after setting IPv6
>     addresses.
> 
> [...]

Here is the summary with links:
  - [v1,net,1/2] dccp: Call security_inet_conn_request() after setting IPv4 addresses.
    https://git.kernel.org/netdev/net/c/fa2df45af130
  - [v1,net,2/2] dccp/tcp: Call security_inet_conn_request() after setting IPv6 addresses.
    https://git.kernel.org/netdev/net/c/23be1e0e2a83

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html