netdev - Re: [PATCH net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid

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]

Message-ID: <20201012104030.GA888@nautica>
Date:   Mon, 12 Oct 2020 12:40:30 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Anant Thazhemadam <anant.thazhemadam@...il.com>
Cc:     ericvh@...il.com, lucho@...kov.net, davem@...emloft.net,
        kuba@...nel.org, v9fs-developer@...ts.sourceforge.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+75d51fe5bf4ebe988518@...kaller.appspotmail.com
Subject: Re: [PATCH net] net: 9p: initialize sun_server.sun_path to have
 addr's value only when addr is valid

Anant Thazhemadam wrote on Mon, Oct 12, 2020:
> You mentioned how a fully zeroed address isn't exactly faulty. By extension, wouldn't that
> mean that an address that simply begins with a 0 isn't faulty as well?

That is correct.
If you have a look at the unix(7) man page that describes AF_UNIX, it
describes what 'abstract' addresses are and unix_mkname() in linux's
net/unix/af_unix.c shows how it's handled.

> This is an interesting point, because if the condition is modified to checking for addr[0] directly,
> addresses that simply begin with 0 (but have more non-zero content following) wouldn't be
> copied over either, right?

Yes, we would reject any address that starts with a nul byte -- but that
is already exactly what your patch does with strlen() already: a '\0' at
the start of the string is equivalent to strlen(addr) == 0.
The only difference is that checking for addr[0] won't run through all
the string if it doesn't start with a nul byte; but this is a one-time
thing at mount so it really doesn't matter.

> In the end, it comes down to what you define as a "valid" value that sun_path can have.
> We've already agreed that a fully zeroed address wouldn't qualify as a valid value for sun_path.
> Are addresses that aren't fully zeroed, but only begin with a 0 also to be considered as an
> unacceptable value for sun_path?

Yes, because the strcpy() a few lines below would copy nothing, leaving
sun_server.sun_path uninitialized like your example.

At that point you could ask why not "fix" that strcpy to properly copy
the address passed instead but that doesn't really make sense given
where 'addr' comes from: it's passed from userspace as a nul-terminated
string, so nothing after the first '\0' is valid.

There probably are ways to work around that (e.g. iproute's ss will
display abstract addresses with a leading '@' instead) but given nobody
ever seemed to care I think it's safe to just return EINVAL there like
you did ; there's nothing wrong with your patch as far as I'm concerned.

-- 
Dominique