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] [day] [month] [year] [list]

Date: Fri, 11 Aug 2023 15:24:40 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Michal Kubecek <mkubecek@...e.cz>, 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

On Fri, 11 Aug 2023 09:29:58 +0200 Jiri Pirko wrote:
> >> 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.

Good catch!

Hopefully we can address that separately (I mean someone who cares can
send a patch? :)). We had multiple people get surprised by info being
NULL I think the value of the other changes outweighs resolving this
little oddity. I'm going to send a v3 with the bug fixed later.

On the existence of genl_info_is_ntf(), I would rather keep it.
I'm a bit worried someone else will need to know at some point and
will do it based on contents of info directly, which will make
future refactoring risky.