netdev - Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 11 Aug 2023 09:29:58 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
	netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
	johannes@...solutions.net,
	Vladimir Oltean <vladimir.oltean@....com>, gal@...dia.com,
	tariqt@...dia.com, lucien.xin@...il.com, f.fainelli@...il.com,
	andrew@...n.ch, simon.horman@...igine.com, linux@...pel-privat.de
Subject: Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass
 genl_info to .prepare_data

Fri, Aug 11, 2023 at 09:13:24AM CEST, mkubecek@...e.cz wrote:
>On Fri, Aug 11, 2023 at 08:42:45AM +0200, Jiri Pirko wrote:
>> Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@...nel.org wrote:
>> >@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
>> > 
>> > static int wol_prepare_data(const struct ethnl_req_info *req_base,
>> > 			    struct ethnl_reply_data *reply_base,
>> >-			    struct genl_info *info)
>> >+			    const struct genl_info *info)
>> > {
>> > 	struct wol_reply_data *data = WOL_REPDATA(reply_base);
>> > 	struct net_device *dev = reply_base->dev;
>> >@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
>> > 	dev->ethtool_ops->get_wol(dev, &data->wol);
>> > 	ethnl_ops_complete(dev);
>> > 	/* do not include password in notifications */
>> >-	data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
>> >+	data->show_sopass = genl_info_is_ntf(info) &&
>> >+		(data->wol.supported & WAKE_MAGICSECURE);
>> 
>> I believe that you are missing "!" here:
>> 	data->show_sopass = !genl_info_is_ntf(info) &&
>> 		(data->wol.supported & WAKE_MAGICSECURE);
>
>Agreed.
>
>> But, you are changing the output for dumpit if I'm not mistaken.
>> ethnl_default_dump_one() currently calls this with info==NULL too, not
>> only ethnl_default_notify().
>
>I would rather see this as a fix. Not showing the password in dumps made
>little sense as it meant the dump output was different from single
>device queries. It was the price to pay for inability to distinguish
>between a dump and a notification.
>
>IIRC the early versions submitted went even further and did not set
>GENL_UNS_ADMIN_PERM for ETHTOOL_MSG_WOL_GET and only omitted the
>password when the request came from an unprivileged process (so that
>unprivileged processes could still query the rest of WoL information)
>but this was dropped during the review as an unnecessary complication.
>
>> Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
>> user is here and I doubt there ever going to be any other. This
>> conditional per-op attr fill seems a bit odd.
>> 
>> Can't you handle this in side ethtool somehow? IDK :/
>
>I don't think so. The point here is that notification can be seen by any
>unprivileged process so as long as we agree that those should not see
>the wake up passwords, we must not include the password in them. While
>ethtool could certanly drop the password from its output, any other
>utility parsing the notifications (or even patched ethtool) could still
>show it to anyone.

Yeah, the question is, if it is a good design to have one CMD type
to conditionally send sensitive data. I would argue that sensitive data
could be sent over separate CMD with no notifier for it.