netdev - Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

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, 25 Oct 2019 11:45:48 -0300
From:   'Marcelo Ricardo Leitner' <marcelo.leitner@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock
 and asoc

On Fri, Oct 25, 2019 at 02:26:57PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 25 October 2019 14:22
> > On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 25 October 2019 04:24
> > > ...
> > > > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > > > >
> > > > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > > > >  					   pinfo.spinfo_assoc_id);
> > > > > -	if (!transport)
> > > > > -		return -EINVAL;
> > > > > +	if (!transport) {
> > > > > +		retval = -EINVAL;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	if (transport->state == SCTP_PF &&
> > > > > +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > > > > +		retval = -EACCES;
> > > > > +		goto out;
> > > > > +	}
> > > >
> > > > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > > > set the system or the socket into the disabled state in order to
> > > > trigger this new check.
> > >
> > > Only because the default isn't to be backwards compatible with the
> >                            ^^^^^
> > 
> > You meant "is", right? Then we're agreeing.
> 
> No, I meant isn't.

Then you missed this detail in the patch. The default here IS to be
backwards compatible.

> The application must see a backwards compatible interface unless
> the application itself requests something different.
> The sysadmin can't be allowed to change the API seen by old applications.

Disagree. Sysadmins should be able to harden their systems as much as
they want/need. Yet, if that causes issues with old applications,
that's on them.

> 
> AFAICT if the protocol part of PF is enabled (which handles primary path
> failure better than the older version) and ' transport->state == SCTP_PF'
> is true then an old application binary will  get a completely unexpected -EACCESS
> rather than a valid state (out of the old valid states) if it requests 'peer addr_info'.
> 
> You cannot assume that just because some sysctl is set (because someone
> building a distribution suddenly decided it was a 'good idea') that an
> application binary will not fall in a big heap due to an error condition
> that couldn't ever happen before.

Agree, but that assumption doesn't have a room here. If the
distribution decided to harden the system, that's on them. Ditto for
many many other decisions, like having SELinux policies to block sshd
to bind only on port 22 and so, or for building the kernel without
SCTP_COOKIE_HMAC_MD5 because they think it's weak, etc.

  Marcelo