[PATCH net 0/2] Generic netlink multicast fixes

[PATCH net 0/2] Generic netlink multicast fixes

[Ido Schimmel]

Restrict two generic netlink multicast groups - in the "psample" and
"NET_DM" families - to be root-only with the appropriate capabilities.
See individual patches for more details.

Ido Schimmel (2):
  psample: Require 'CAP_NET_ADMIN' when joining "packets" group
  drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group

 include/net/genetlink.h | 2 ++
 net/core/drop_monitor.c | 4 +++-
 net/netlink/genetlink.c | 3 +++
 net/psample/psample.c   | 3 ++-
 4 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH net 1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group

[Ido Schimmel]

The "psample" generic netlink family notifies sampled packets over the
"packets" multicast group. This is problematic since by default generic
netlink allows non-root users to listen to these notifications.

Fix by marking the group with the 'GENL_UNS_ADMIN_PERM' flag. This will
prevent non-root users or root without the 'CAP_NET_ADMIN' capability
(in the user namespace owning the network namespace) from joining the
group.

Tested using [1].

Before:

 # capsh -- -c ./psample_repo
 # capsh --drop=cap_net_admin -- -c ./psample_repo

After:

 # capsh -- -c ./psample_repo
 # capsh --drop=cap_net_admin -- -c ./psample_repo
 Failed to join "packets" multicast group

[1]
 $ cat psample.c
 #include <stdio.h>
 #include <netlink/genl/ctrl.h>
 #include <netlink/genl/genl.h>
 #include <netlink/socket.h>

 int join_grp(struct nl_sock *sk, const char *grp_name)
 {
 	int grp, err;

 	grp = genl_ctrl_resolve_grp(sk, "psample", grp_name);
 	if (grp < 0) {
 		fprintf(stderr, "Failed to resolve \"%s\" multicast group\n",
 			grp_name);
 		return grp;
 	}

 	err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
 	if (err) {
 		fprintf(stderr, "Failed to join \"%s\" multicast group\n",
 			grp_name);
 		return err;
 	}

 	return 0;
 }

 int main(int argc, char **argv)
 {
 	struct nl_sock *sk;
 	int err;

 	sk = nl_socket_alloc();
 	if (!sk) {
 		fprintf(stderr, "Failed to allocate socket\n");
 		return -1;
 	}

 	err = genl_connect(sk);
 	if (err) {
 		fprintf(stderr, "Failed to connect socket\n");
 		return err;
 	}

 	err = join_grp(sk, "config");
 	if (err)
 		return err;

 	err = join_grp(sk, "packets");
 	if (err)
 		return err;

 	return 0;
 }
 $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o psample_repo psample.c

Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/psample/psample.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/psample/psample.c b/net/psample/psample.c
index 81a794e36f53..c34e902855db 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -31,7 +31,8 @@ enum psample_nl_multicast_groups {
 
 static const struct genl_multicast_group psample_nl_mcgrps[] = {
 	[PSAMPLE_NL_MCGRP_CONFIG] = { .name = PSAMPLE_NL_MCGRP_CONFIG_NAME },
-	[PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME },
+	[PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME,
+				      .flags = GENL_UNS_ADMIN_PERM },
 };
 
 static struct genl_family psample_nl_family __ro_after_init;
-- 
2.40.1

Re: [PATCH net 1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group

[Jacob Keller]

On 12/6/2023 1:31 PM, Ido Schimmel wrote:
> The "psample" generic netlink family notifies sampled packets over the
> "packets" multicast group. This is problematic since by default generic
> netlink allows non-root users to listen to these notifications.
> 
> Fix by marking the group with the 'GENL_UNS_ADMIN_PERM' flag. This will
> prevent non-root users or root without the 'CAP_NET_ADMIN' capability
> (in the user namespace owning the network namespace) from joining the
> group.
> 

Ooh that could be a pretty significant leak. Glad to see it closed.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

> Tested using [1].
> 
> Before:
> 
>  # capsh -- -c ./psample_repo
>  # capsh --drop=cap_net_admin -- -c ./psample_repo
> 
> After:
> 
>  # capsh -- -c ./psample_repo
>  # capsh --drop=cap_net_admin -- -c ./psample_repo
>  Failed to join "packets" multicast group
> 
> [1]
>  $ cat psample.c
>  #include <stdio.h>
>  #include <netlink/genl/ctrl.h>
>  #include <netlink/genl/genl.h>
>  #include <netlink/socket.h>
> 
>  int join_grp(struct nl_sock *sk, const char *grp_name)
>  {
>  	int grp, err;
> 
>  	grp = genl_ctrl_resolve_grp(sk, "psample", grp_name);
>  	if (grp < 0) {
>  		fprintf(stderr, "Failed to resolve \"%s\" multicast group\n",
>  			grp_name);
>  		return grp;
>  	}
> 
>  	err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
>  	if (err) {
>  		fprintf(stderr, "Failed to join \"%s\" multicast group\n",
>  			grp_name);
>  		return err;
>  	}
> 
>  	return 0;
>  }
> 
>  int main(int argc, char **argv)
>  {
>  	struct nl_sock *sk;
>  	int err;
> 
>  	sk = nl_socket_alloc();
>  	if (!sk) {
>  		fprintf(stderr, "Failed to allocate socket\n");
>  		return -1;
>  	}
> 
>  	err = genl_connect(sk);
>  	if (err) {
>  		fprintf(stderr, "Failed to connect socket\n");
>  		return err;
>  	}
> 
>  	err = join_grp(sk, "config");
>  	if (err)
>  		return err;
> 
>  	err = join_grp(sk, "packets");
>  	if (err)
>  		return err;
> 
>  	return 0;
>  }
>  $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o psample_repo psample.c
> 
> Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
> Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/psample/psample.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 81a794e36f53..c34e902855db 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -31,7 +31,8 @@ enum psample_nl_multicast_groups {
>  
>  static const struct genl_multicast_group psample_nl_mcgrps[] = {
>  	[PSAMPLE_NL_MCGRP_CONFIG] = { .name = PSAMPLE_NL_MCGRP_CONFIG_NAME },
> -	[PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME },
> +	[PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> +				      .flags = GENL_UNS_ADMIN_PERM },
>  };
>  
>  static struct genl_family psample_nl_family __ro_after_init;

Re: [PATCH net 1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group

[Jamal Hadi Salim]

On Wed, Dec 6, 2023 at 4:33 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> The "psample" generic netlink family notifies sampled packets over the
> "packets" multicast group. This is problematic since by default generic
> netlink allows non-root users to listen to these notifications.
>
> Fix by marking the group with the 'GENL_UNS_ADMIN_PERM' flag. This will
> prevent non-root users or root without the 'CAP_NET_ADMIN' capability
> (in the user namespace owning the network namespace) from joining the
> group.
>

Out of curiosity, shouldnt reading/getting also be disallowed then?
Traditionally both listening and reading has been allowed without root
for most netlink endpoints...
IOW, if i cant listen but am able to dump, isnt whatever "security
hole" still in play even after this change?

cheers,
jamal



> Tested using [1].
>
> Before:
>
>  # capsh -- -c ./psample_repo
>  # capsh --drop=cap_net_admin -- -c ./psample_repo
>
> After:
>
>  # capsh -- -c ./psample_repo
>  # capsh --drop=cap_net_admin -- -c ./psample_repo
>  Failed to join "packets" multicast group
>
> [1]
>  $ cat psample.c
>  #include <stdio.h>
>  #include <netlink/genl/ctrl.h>
>  #include <netlink/genl/genl.h>
>  #include <netlink/socket.h>
>
>  int join_grp(struct nl_sock *sk, const char *grp_name)
>  {
>         int grp, err;
>
>         grp = genl_ctrl_resolve_grp(sk, "psample", grp_name);
>         if (grp < 0) {
>                 fprintf(stderr, "Failed to resolve \"%s\" multicast group\n",
>                         grp_name);
>                 return grp;
>         }
>
>         err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
>         if (err) {
>                 fprintf(stderr, "Failed to join \"%s\" multicast group\n",
>                         grp_name);
>                 return err;
>         }
>
>         return 0;
>  }
>
>  int main(int argc, char **argv)
>  {
>         struct nl_sock *sk;
>         int err;
>
>         sk = nl_socket_alloc();
>         if (!sk) {
>                 fprintf(stderr, "Failed to allocate socket\n");
>                 return -1;
>         }
>
>         err = genl_connect(sk);
>         if (err) {
>                 fprintf(stderr, "Failed to connect socket\n");
>                 return err;
>         }
>
>         err = join_grp(sk, "config");
>         if (err)
>                 return err;
>
>         err = join_grp(sk, "packets");
>         if (err)
>                 return err;
>
>         return 0;
>  }
>  $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o psample_repo psample.c
>
> Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
> Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/psample/psample.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 81a794e36f53..c34e902855db 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -31,7 +31,8 @@ enum psample_nl_multicast_groups {
>
>  static const struct genl_multicast_group psample_nl_mcgrps[] = {
>         [PSAMPLE_NL_MCGRP_CONFIG] = { .name = PSAMPLE_NL_MCGRP_CONFIG_NAME },
> -       [PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME },
> +       [PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> +                                     .flags = GENL_UNS_ADMIN_PERM },
>  };
>
>  static struct genl_family psample_nl_family __ro_after_init;
> --
> 2.40.1
>

Re: [PATCH net 1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group

[Jiri Pirko]

Thu, Dec 07, 2023 at 11:16:30AM CET, jhs@mojatatu.com wrote:
>On Wed, Dec 6, 2023 at 4:33 PM Ido Schimmel <idosch@nvidia.com> wrote:
>>
>> The "psample" generic netlink family notifies sampled packets over the
>> "packets" multicast group. This is problematic since by default generic
>> netlink allows non-root users to listen to these notifications.
>>
>> Fix by marking the group with the 'GENL_UNS_ADMIN_PERM' flag. This will
>> prevent non-root users or root without the 'CAP_NET_ADMIN' capability
>> (in the user namespace owning the network namespace) from joining the
>> group.
>>
>
>Out of curiosity, shouldnt reading/getting also be disallowed then?

This is about the sampled packets. You only get them by notifications.


>Traditionally both listening and reading has been allowed without root
>for most netlink endpoints...
>IOW, if i cant listen but am able to dump, isnt whatever "security
>hole" still in play even after this change?
>
>cheers,
>jamal
>
>
>
>> Tested using [1].
>>
>> Before:
>>
>>  # capsh -- -c ./psample_repo
>>  # capsh --drop=cap_net_admin -- -c ./psample_repo
>>
>> After:
>>
>>  # capsh -- -c ./psample_repo
>>  # capsh --drop=cap_net_admin -- -c ./psample_repo
>>  Failed to join "packets" multicast group
>>
>> [1]
>>  $ cat psample.c
>>  #include <stdio.h>
>>  #include <netlink/genl/ctrl.h>
>>  #include <netlink/genl/genl.h>
>>  #include <netlink/socket.h>
>>
>>  int join_grp(struct nl_sock *sk, const char *grp_name)
>>  {
>>         int grp, err;
>>
>>         grp = genl_ctrl_resolve_grp(sk, "psample", grp_name);
>>         if (grp < 0) {
>>                 fprintf(stderr, "Failed to resolve \"%s\" multicast group\n",
>>                         grp_name);
>>                 return grp;
>>         }
>>
>>         err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
>>         if (err) {
>>                 fprintf(stderr, "Failed to join \"%s\" multicast group\n",
>>                         grp_name);
>>                 return err;
>>         }
>>
>>         return 0;
>>  }
>>
>>  int main(int argc, char **argv)
>>  {
>>         struct nl_sock *sk;
>>         int err;
>>
>>         sk = nl_socket_alloc();
>>         if (!sk) {
>>                 fprintf(stderr, "Failed to allocate socket\n");
>>                 return -1;
>>         }
>>
>>         err = genl_connect(sk);
>>         if (err) {
>>                 fprintf(stderr, "Failed to connect socket\n");
>>                 return err;
>>         }
>>
>>         err = join_grp(sk, "config");
>>         if (err)
>>                 return err;
>>
>>         err = join_grp(sk, "packets");
>>         if (err)
>>                 return err;
>>
>>         return 0;
>>  }
>>  $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o psample_repo psample.c
>>
>> Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
>> Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
>> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
>> ---
>>  net/psample/psample.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/psample/psample.c b/net/psample/psample.c
>> index 81a794e36f53..c34e902855db 100644
>> --- a/net/psample/psample.c
>> +++ b/net/psample/psample.c
>> @@ -31,7 +31,8 @@ enum psample_nl_multicast_groups {
>>
>>  static const struct genl_multicast_group psample_nl_mcgrps[] = {
>>         [PSAMPLE_NL_MCGRP_CONFIG] = { .name = PSAMPLE_NL_MCGRP_CONFIG_NAME },
>> -       [PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME },
>> +       [PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME,
>> +                                     .flags = GENL_UNS_ADMIN_PERM },
>>  };
>>
>>  static struct genl_family psample_nl_family __ro_after_init;
>> --
>> 2.40.1
>>

Re: [PATCH net 1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group

[Jiri Pirko]

Wed, Dec 06, 2023 at 10:31:01PM CET, idosch@nvidia.com wrote:
>The "psample" generic netlink family notifies sampled packets over the
>"packets" multicast group. This is problematic since by default generic
>netlink allows non-root users to listen to these notifications.
>
>Fix by marking the group with the 'GENL_UNS_ADMIN_PERM' flag. This will
>prevent non-root users or root without the 'CAP_NET_ADMIN' capability
>(in the user namespace owning the network namespace) from joining the
>group.
>
>Tested using [1].
>
>Before:
>
> # capsh -- -c ./psample_repo
> # capsh --drop=cap_net_admin -- -c ./psample_repo
>
>After:
>
> # capsh -- -c ./psample_repo
> # capsh --drop=cap_net_admin -- -c ./psample_repo
> Failed to join "packets" multicast group
>
>[1]
> $ cat psample.c
> #include <stdio.h>
> #include <netlink/genl/ctrl.h>
> #include <netlink/genl/genl.h>
> #include <netlink/socket.h>
>
> int join_grp(struct nl_sock *sk, const char *grp_name)
> {
> 	int grp, err;
>
> 	grp = genl_ctrl_resolve_grp(sk, "psample", grp_name);
> 	if (grp < 0) {
> 		fprintf(stderr, "Failed to resolve \"%s\" multicast group\n",
> 			grp_name);
> 		return grp;
> 	}
>
> 	err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
> 	if (err) {
> 		fprintf(stderr, "Failed to join \"%s\" multicast group\n",
> 			grp_name);
> 		return err;
> 	}
>
> 	return 0;
> }
>
> int main(int argc, char **argv)
> {
> 	struct nl_sock *sk;
> 	int err;
>
> 	sk = nl_socket_alloc();
> 	if (!sk) {
> 		fprintf(stderr, "Failed to allocate socket\n");
> 		return -1;
> 	}
>
> 	err = genl_connect(sk);
> 	if (err) {
> 		fprintf(stderr, "Failed to connect socket\n");
> 		return err;
> 	}
>
> 	err = join_grp(sk, "config");
> 	if (err)
> 		return err;
>
> 	err = join_grp(sk, "packets");
> 	if (err)
> 		return err;
>
> 	return 0;
> }
> $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o psample_repo psample.c
>
>Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
>Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
>Signed-off-by: Ido Schimmel <idosch@nvidia.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH net 2/2] drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group

[Ido Schimmel]

The "NET_DM" generic netlink family notifies drop locations over the
"events" multicast group. This is problematic since by default generic
netlink allows non-root users to listen to these notifications.

Fix by adding a new field to the generic netlink multicast group
structure that when set prevents non-root users or root without the
'CAP_SYS_ADMIN' capability (in the user namespace owning the network
namespace) from joining the group. Set this field for the "events"
group. Use 'CAP_SYS_ADMIN' rather than 'CAP_NET_ADMIN' because of the
nature of the information that is shared over this group.

Note that the capability check in this case will always be performed
against the initial user namespace since the family is not netns aware
and only operates in the initial network namespace.

A new field is added to the structure rather than using the "flags"
field because the existing field uses uAPI flags and it is inappropriate
to add a new uAPI flag for an internal kernel check. In net-next we can
rework the "flags" field to use internal flags and fold the new field
into it. But for now, in order to reduce the amount of changes, add a
new field.

Since the information can only be consumed by root, mark the control
plane operations that start and stop the tracing as root-only using the
'GENL_ADMIN_PERM' flag.

Tested using [1].

Before:

 # capsh -- -c ./dm_repo
 # capsh --drop=cap_sys_admin -- -c ./dm_repo

After:

 # capsh -- -c ./dm_repo
 # capsh --drop=cap_sys_admin -- -c ./dm_repo
 Failed to join "events" multicast group

[1]
 $ cat dm.c
 #include <stdio.h>
 #include <netlink/genl/ctrl.h>
 #include <netlink/genl/genl.h>
 #include <netlink/socket.h>

 int main(int argc, char **argv)
 {
 	struct nl_sock *sk;
 	int grp, err;

 	sk = nl_socket_alloc();
 	if (!sk) {
 		fprintf(stderr, "Failed to allocate socket\n");
 		return -1;
 	}

 	err = genl_connect(sk);
 	if (err) {
 		fprintf(stderr, "Failed to connect socket\n");
 		return err;
 	}

 	grp = genl_ctrl_resolve_grp(sk, "NET_DM", "events");
 	if (grp < 0) {
 		fprintf(stderr,
 			"Failed to resolve \"events\" multicast group\n");
 		return grp;
 	}

 	err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
 	if (err) {
 		fprintf(stderr, "Failed to join \"events\" multicast group\n");
 		return err;
 	}

 	return 0;
 }
 $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o dm_repo dm.c

Fixes: 9a8afc8d3962 ("Network Drop Monitor: Adding drop monitor implementation & Netlink protocol")
Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/genetlink.h | 2 ++
 net/core/drop_monitor.c | 4 +++-
 net/netlink/genetlink.c | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index e18a4c0d69ee..c53244f20437 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -12,10 +12,12 @@
  * struct genl_multicast_group - generic netlink multicast group
  * @name: name of the multicast group, names are per-family
  * @flags: GENL_* flags (%GENL_ADMIN_PERM or %GENL_UNS_ADMIN_PERM)
+ * @cap_sys_admin: whether %CAP_SYS_ADMIN is required for binding
  */
 struct genl_multicast_group {
 	char			name[GENL_NAMSIZ];
 	u8			flags;
+	u8			cap_sys_admin:1;
 };
 
 struct genl_split_ops;
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index aff31cd944c2..b240d9aae4a6 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -183,7 +183,7 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data)
 }
 
 static const struct genl_multicast_group dropmon_mcgrps[] = {
-	{ .name = "events", },
+	{ .name = "events", .cap_sys_admin = 1 },
 };
 
 static void send_dm_alert(struct work_struct *work)
@@ -1619,11 +1619,13 @@ static const struct genl_small_ops dropmon_ops[] = {
 		.cmd = NET_DM_CMD_START,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = net_dm_cmd_trace,
+		.flags = GENL_ADMIN_PERM,
 	},
 	{
 		.cmd = NET_DM_CMD_STOP,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = net_dm_cmd_trace,
+		.flags = GENL_ADMIN_PERM,
 	},
 	{
 		.cmd = NET_DM_CMD_CONFIG_GET,
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 92ef5ed2e7b0..9c7ffd10df2a 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1691,6 +1691,9 @@ static int genl_bind(struct net *net, int group)
 		if ((grp->flags & GENL_UNS_ADMIN_PERM) &&
 		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
 			ret = -EPERM;
+		if (grp->cap_sys_admin &&
+		    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
+			ret = -EPERM;
 
 		break;
 	}
-- 
2.40.1

Re: [PATCH net 2/2] drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group

[Jacob Keller]

On 12/6/2023 1:31 PM, Ido Schimmel wrote:
> The "NET_DM" generic netlink family notifies drop locations over the
> "events" multicast group. This is problematic since by default generic
> netlink allows non-root users to listen to these notifications.
> 
> Fix by adding a new field to the generic netlink multicast group
> structure that when set prevents non-root users or root without the
> 'CAP_SYS_ADMIN' capability (in the user namespace owning the network
> namespace) from joining the group. Set this field for the "events"
> group. Use 'CAP_SYS_ADMIN' rather than 'CAP_NET_ADMIN' because of the
> nature of the information that is shared over this group.
> 
> Note that the capability check in this case will always be performed
> against the initial user namespace since the family is not netns aware
> and only operates in the initial network namespace.
> 
> A new field is added to the structure rather than using the "flags"
> field because the existing field uses uAPI flags and it is inappropriate
> to add a new uAPI flag for an internal kernel check. In net-next we can
> rework the "flags" field to use internal flags and fold the new field
> into it. But for now, in order to reduce the amount of changes, add a
> new field.
> 
> Since the information can only be consumed by root, mark the control
> plane operations that start and stop the tracing as root-only using the
> 'GENL_ADMIN_PERM' flag.
> 

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net 2/2] drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group

[Jiri Pirko]

Wed, Dec 06, 2023 at 10:31:02PM CET, idosch@nvidia.com wrote:
>The "NET_DM" generic netlink family notifies drop locations over the
>"events" multicast group. This is problematic since by default generic
>netlink allows non-root users to listen to these notifications.
>
>Fix by adding a new field to the generic netlink multicast group
>structure that when set prevents non-root users or root without the
>'CAP_SYS_ADMIN' capability (in the user namespace owning the network
>namespace) from joining the group. Set this field for the "events"
>group. Use 'CAP_SYS_ADMIN' rather than 'CAP_NET_ADMIN' because of the
>nature of the information that is shared over this group.
>
>Note that the capability check in this case will always be performed
>against the initial user namespace since the family is not netns aware
>and only operates in the initial network namespace.
>
>A new field is added to the structure rather than using the "flags"
>field because the existing field uses uAPI flags and it is inappropriate
>to add a new uAPI flag for an internal kernel check. In net-next we can
>rework the "flags" field to use internal flags and fold the new field
>into it. But for now, in order to reduce the amount of changes, add a
>new field.
>
>Since the information can only be consumed by root, mark the control
>plane operations that start and stop the tracing as root-only using the
>'GENL_ADMIN_PERM' flag.
>
>Tested using [1].
>
>Before:
>
> # capsh -- -c ./dm_repo
> # capsh --drop=cap_sys_admin -- -c ./dm_repo
>
>After:
>
> # capsh -- -c ./dm_repo
> # capsh --drop=cap_sys_admin -- -c ./dm_repo
> Failed to join "events" multicast group
>
>[1]
> $ cat dm.c
> #include <stdio.h>
> #include <netlink/genl/ctrl.h>
> #include <netlink/genl/genl.h>
> #include <netlink/socket.h>
>
> int main(int argc, char **argv)
> {
> 	struct nl_sock *sk;
> 	int grp, err;
>
> 	sk = nl_socket_alloc();
> 	if (!sk) {
> 		fprintf(stderr, "Failed to allocate socket\n");
> 		return -1;
> 	}
>
> 	err = genl_connect(sk);
> 	if (err) {
> 		fprintf(stderr, "Failed to connect socket\n");
> 		return err;
> 	}
>
> 	grp = genl_ctrl_resolve_grp(sk, "NET_DM", "events");
> 	if (grp < 0) {
> 		fprintf(stderr,
> 			"Failed to resolve \"events\" multicast group\n");
> 		return grp;
> 	}
>
> 	err = nl_socket_add_memberships(sk, grp, NFNLGRP_NONE);
> 	if (err) {
> 		fprintf(stderr, "Failed to join \"events\" multicast group\n");
> 		return err;
> 	}
>
> 	return 0;
> }
> $ gcc -I/usr/include/libnl3 -lnl-3 -lnl-genl-3 -o dm_repo dm.c
>
>Fixes: 9a8afc8d3962 ("Network Drop Monitor: Adding drop monitor implementation & Netlink protocol")
>Reported-by: "The UK's National Cyber Security Centre (NCSC)" <security@ncsc.gov.uk>
>Signed-off-by: Ido Schimmel <idosch@nvidia.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net 0/2] Generic netlink multicast fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 6 Dec 2023 23:31:00 +0200 you wrote:
> Restrict two generic netlink multicast groups - in the "psample" and
> "NET_DM" families - to be root-only with the appropriate capabilities.
> See individual patches for more details.
> 
> Ido Schimmel (2):
>   psample: Require 'CAP_NET_ADMIN' when joining "packets" group
>   drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group
> 
> [...]

Here is the summary with links:
  - [net,1/2] psample: Require 'CAP_NET_ADMIN' when joining "packets" group
    https://git.kernel.org/netdev/net/c/44ec98ea5ea9
  - [net,2/2] drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group
    https://git.kernel.org/netdev/net/c/e03781879a0d

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