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: <ZNXjdj3edS1Up3Mt@nanopsycho> 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.
Powered by blists - more mailing lists