netdev - Re: [PATCH net-next v1 4/7] tools/net/ynl: accept IP string inputs

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]

Message-ID: <CAD4GDZyXDpbEoVFBzU-ExYcd_Gf-hHocPUFWAXBXJOcmMRrnSg@mail.gmail.com>
Date: Wed, 4 Dec 2024 13:37:35 +0000
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Johannes Berg <johannes@...solutions.net>, linux-wireless@...r.kernel.org, 
	donald.hunter@...hat.com
Subject: Re: [PATCH net-next v1 4/7] tools/net/ynl: accept IP string inputs

On Wed, 4 Dec 2024 at 02:07, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  3 Dec 2024 13:06:52 +0000 Donald Hunter wrote:
> > +    def _from_string(self, string, display_hint, type):
>
> Any reason not to pass attr_spec instead of the members one by one?

I thought it would be better to keep the low-level helpers decoupled
from attr_spec and mostly just following existing helpers that only
have the display_hint parameter.

> > +        if display_hint in ['ipv4', 'ipv6']:
> > +            ip = ipaddress.ip_address(string)
> > +            if type == 'binary':
> > +                raw = ip.packed
> > +            else:
> > +                raw = int(ip)
> > +        else:
>
> I wonder if we should raise in this case?
> Especially if type is binary passing the string back will just blow up
> later, right? We could instead rise with a nice clear error message
> here.

It's actually a bit misleading that the attr is called 'string'
because it could be a binary input if it was supplied from python
code, i.e. not parsed from JSON on the command-line.

But you highlight the bigger point that our input validation is quite
weak and will need to be improved. In this case it would be desirable
to defensively check if the input is already binary before checking
for a display-hint. Then for input that can't be handled, we can raise
an error message.