2018-01-10 01:27:38

by Guillem Jover

[permalink] [raw]
Subject: [PATCH] Do not bind to reserved ports registered in /etc/services

When using the bindresvport() function a privileged port will be looked
for and bound to a socket. The problem is that any service using a static
privileged port registered in the /etc/services file, can get its port
taken over by libtirpc users, making the other service fail to start.

Starting the other service before libtircp users is not an option as
this does not cover restart situations, for example during package
upgrades or HA switchovers and similar.

In addition honoring the /etc/services registry is already done for
example by rpc.statd, so it seems obvious to make libtirpc follow this
same pattern too.

Signed-off-by: Guillem Jover <[email protected]>
---
src/bindresvport.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/bindresvport.c b/src/bindresvport.c
index 2d8f2bc..98e5f40 100644
--- a/src/bindresvport.c
+++ b/src/bindresvport.c
@@ -40,6 +40,7 @@
#include <netinet/in.h>

#include <errno.h>
+#include <netdb.h>
#include <string.h>
#include <unistd.h>

@@ -73,12 +74,15 @@ bindresvport_sa(sd, sa)
int sd;
struct sockaddr *sa;
{
- int res, af;
+ int res, af, so_proto;
+ socklen_t so_proto_len;
struct sockaddr_storage myaddr;
struct sockaddr_in *sin;
#ifdef INET6
struct sockaddr_in6 *sin6;
#endif
+ struct servent *se;
+ const char *se_proto;
u_int16_t *portp;
static u_int16_t port;
static short startport = STARTPORT;
@@ -125,6 +129,25 @@ bindresvport_sa(sd, sa)
}
sa->sa_family = af;

+ so_proto_len = sizeof(so_proto);
+ if (getsockopt(sd, SOL_SOCKET, SO_PROTOCOL, &so_proto, &so_proto_len) == -1) {
+ mutex_unlock(&port_lock);
+ return -1; /* errno is correctly set */
+ }
+ switch (so_proto) {
+ case IPPROTO_UDP:
+ case IPPROTO_UDPLITE:
+ se_proto = "udp";
+ break;
+ case IPPROTO_TCP:
+ se_proto = "tcp";
+ break;
+ default:
+ errno = ENOPROTOOPT;
+ mutex_unlock(&port_lock);
+ return -1;
+ }
+
if (port == 0) {
port = (getpid() % NPORTS) + STARTPORT;
}
@@ -135,6 +158,9 @@ bindresvport_sa(sd, sa)
*portp = htons(port++);
if (port > endport)
port = startport;
+ se = getservbyport(*portp, se_proto);
+ if (se != NULL)
+ continue;
res = bind(sd, sa, salen);
if (res >= 0 || errno != EADDRINUSE)
break;
--
2.15.1



2018-01-11 15:18:53

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services



On 01/09/2018 07:49 PM, Guillem Jover wrote:
> When using the bindresvport() function a privileged port will be looked
> for and bound to a socket. The problem is that any service using a static
> privileged port registered in the /etc/services file, can get its port
> taken over by libtirpc users, making the other service fail to start.
>
> Starting the other service before libtircp users is not an option as
> this does not cover restart situations, for example during package
> upgrades or HA switchovers and similar.
>
> In addition honoring the /etc/services registry is already done for
> example by rpc.statd, so it seems obvious to make libtirpc follow this
> same pattern too.
Overall I think this makes sense, but this eliminates 240 privilege
ports and worried we would run out of port (due to them in TIME_WAIT)
during a v3 mount storms. A port goes into TIME_WAIT after a v3 mount
is done... But on the other hand v3 is no longer the default and
there are 784 available ports.... Hopefully that is enough.

Does anybody else have concerns about limiting the ports space?

steved.

>
> Signed-off-by: Guillem Jover <[email protected]>
> ---
> src/bindresvport.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/src/bindresvport.c b/src/bindresvport.c
> index 2d8f2bc..98e5f40 100644
> --- a/src/bindresvport.c
> +++ b/src/bindresvport.c
> @@ -40,6 +40,7 @@
> #include <netinet/in.h>
>
> #include <errno.h>
> +#include <netdb.h>
> #include <string.h>
> #include <unistd.h>
>
> @@ -73,12 +74,15 @@ bindresvport_sa(sd, sa)
> int sd;
> struct sockaddr *sa;
> {
> - int res, af;
> + int res, af, so_proto;
> + socklen_t so_proto_len;
> struct sockaddr_storage myaddr;
> struct sockaddr_in *sin;
> #ifdef INET6
> struct sockaddr_in6 *sin6;
> #endif
> + struct servent *se;
> + const char *se_proto;
> u_int16_t *portp;
> static u_int16_t port;
> static short startport = STARTPORT;
> @@ -125,6 +129,25 @@ bindresvport_sa(sd, sa)
> }
> sa->sa_family = af;
>
> + so_proto_len = sizeof(so_proto);
> + if (getsockopt(sd, SOL_SOCKET, SO_PROTOCOL, &so_proto, &so_proto_len) == -1) {
> + mutex_unlock(&port_lock);
> + return -1; /* errno is correctly set */
> + }
> + switch (so_proto) {
> + case IPPROTO_UDP:
> + case IPPROTO_UDPLITE:
> + se_proto = "udp";
> + break;
> + case IPPROTO_TCP:
> + se_proto = "tcp";
> + break;
> + default:
> + errno = ENOPROTOOPT;
> + mutex_unlock(&port_lock);
> + return -1;
> + }
> +
> if (port == 0) {
> port = (getpid() % NPORTS) + STARTPORT;
> }
> @@ -135,6 +158,9 @@ bindresvport_sa(sd, sa)
> *portp = htons(port++);
> if (port > endport)
> port = startport;
> + se = getservbyport(*portp, se_proto);
> + if (se != NULL)
> + continue;
> res = bind(sd, sa, salen);
> if (res >= 0 || errno != EADDRINUSE)
> break;
>

2018-01-11 15:50:36

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services



> On Jan 9, 2018, at 7:49 PM, Guillem Jover <[email protected]> wrote:
>=20
> When using the bindresvport() function a privileged port will be =
looked
> for and bound to a socket. The problem is that any service using a =
static
> privileged port registered in the /etc/services file, can get its port
> taken over by libtirpc users, making the other service fail to start.
>=20
> Starting the other service before libtircp users is not an option as
> this does not cover restart situations, for example during package
> upgrades or HA switchovers and similar.
>=20
> In addition honoring the /etc/services registry is already done for
> example by rpc.statd, so it seems obvious to make libtirpc follow this
> same pattern too.

Does the glibc version of bindresvport(3) skip ports? I ask because
this hasn't been a noted problem before. Which service exactly is
causing the difficulty?

Usually applications that grab a privileged port are short-lived.
statd is careful to avoid ports in /etc/services because it allocates
one socket with a privileged port for contacting the kernel, and keeps
it in place.


> Signed-off-by: Guillem Jover <[email protected]>
> ---
> src/bindresvport.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>=20
> diff --git a/src/bindresvport.c b/src/bindresvport.c
> index 2d8f2bc..98e5f40 100644
> --- a/src/bindresvport.c
> +++ b/src/bindresvport.c
> @@ -40,6 +40,7 @@
> #include <netinet/in.h>
>=20
> #include <errno.h>
> +#include <netdb.h>
> #include <string.h>
> #include <unistd.h>
>=20
> @@ -73,12 +74,15 @@ bindresvport_sa(sd, sa)
> int sd;
> struct sockaddr *sa;
> {
> - int res, af;
> + int res, af, so_proto;
> + socklen_t so_proto_len;
> struct sockaddr_storage myaddr;
> struct sockaddr_in *sin;
> #ifdef INET6
> struct sockaddr_in6 *sin6;
> #endif
> + struct servent *se;
> + const char *se_proto;
> u_int16_t *portp;
> static u_int16_t port;
> static short startport =3D STARTPORT;
> @@ -125,6 +129,25 @@ bindresvport_sa(sd, sa)
> }
> sa->sa_family =3D af;
>=20
> + so_proto_len =3D sizeof(so_proto);
> + if (getsockopt(sd, SOL_SOCKET, SO_PROTOCOL, &so_proto, =
&so_proto_len) =3D=3D -1) {
> + mutex_unlock(&port_lock);
> + return -1; /* errno is correctly set */
> + }
> + switch (so_proto) {
> + case IPPROTO_UDP:
> + case IPPROTO_UDPLITE:

What is UDPLITE? Would it require a separate netid
or other infrastructure? IMO it's not correct to add
this transport protocol in this patch. If you resend,
please remove this line.


> + se_proto =3D "udp";
> + break;
> + case IPPROTO_TCP:
> + se_proto =3D "tcp";
> + break;
> + default:
> + errno =3D ENOPROTOOPT;
> + mutex_unlock(&port_lock);
> + return -1;
> + }
> +
> if (port =3D=3D 0) {
> port =3D (getpid() % NPORTS) + STARTPORT;
> }
> @@ -135,6 +158,9 @@ bindresvport_sa(sd, sa)
> *portp =3D htons(port++);
> if (port > endport)=20
> port =3D startport;
> + se =3D getservbyport(*portp, se_proto);
> + if (se !=3D NULL)
> + continue;
> res =3D bind(sd, sa, salen);
> if (res >=3D 0 || errno !=3D EADDRINUSE)
> break;
> --=20
> 2.15.1
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever




2018-01-12 18:14:47

by Guillem Jover

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

Hi!

On Thu, 2018-01-11 at 10:50:14 -0500, Chuck Lever wrote:
> On Jan 9, 2018, at 7:49 PM, Guillem Jover <[email protected]> wrote:
> > When using the bindresvport() function a privileged port will be looked
> > for and bound to a socket. The problem is that any service using a static
> > privileged port registered in the /etc/services file, can get its port
> > taken over by libtirpc users, making the other service fail to start.
> >
> > Starting the other service before libtircp users is not an option as
> > this does not cover restart situations, for example during package
> > upgrades or HA switchovers and similar.
> >
> > In addition honoring the /etc/services registry is already done for
> > example by rpc.statd, so it seems obvious to make libtirpc follow this
> > same pattern too.
>
> Does the glibc version of bindresvport(3) skip ports? I ask because
> this hasn't been a noted problem before. Which service exactly is
> causing the difficulty?

So, this is all a bit of a mess, and apparently it has been known in
some quarters for quite some time. :(

This was reported to glibc a long time ago [B] but at the time upstream
bogusly rejected the idea of using /etc/services (or similar). Instead
someone had to create a workaround to overcome this [P], a daemon which
binds to a port, until the other service requests the port to be released
so that it can itself bind to it, which is still obviously prone to races
during restarts and complicates things.

[B] <https://sourceware.org/bugzilla/show_bug.cgi?id=1014>
<https://bugzilla.redhat.com/show_bug.cgi?id=103401>
<https://bugzilla.novell.com/show_bug.cgi?id=262341>
<https://bugs.debian.org/638322>
[P] <http://cyberelk.net/tim/software/portreserve/>

Then some distributions instead patched their glibc to honor a new
/etc/bindresvport.blacklist file in the bindresvport() function; OpenSUSE
at least, and apparently Debian too [F] (although this is not documented
anywhere). In addition nfs-utils upstream (where an rpc.statd comes from)
already honors /etc/services. To complicate things, rpcbind and other
SunRPC clients using libtirpc do not honor neither /etc/services nor
/etc/bindresvport.blacklist, because libtirpc has its own bindrecvport()
implementation.

[F] <https://sources.debian.org/src/glibc/2.26-3/debian/patches/any/local-bindresvport_blacklist.diff/>

On the above Debian bug report, it was proposed to make libtirpc switch
to use the libc bindresvport() implementation so that at least on those
distributions where it is locally patched it would honor the
/etc/bindresvport.blacklist file. The problem with this, is of course
that it does not help any upstream code on any other non-patched system.

So I decided against using the undocumented /etc/bindresvport.blacklist,
because that requires filling it up with ports that are already present
in /etc/services, which seems pointless. It does not support a fragments
style directory such as /etc/bindresvport.blacklist.d/ anyway which
makes "registering" the ports from each package too difficult. And
because nfs-utils' rpc.statd will honor /etc/services on any system
regardless of the libc implementation, which is not the case for
/etc/bindresvport.blacklist.

But at this point, if you disagree and prefer some other solution, I'm
happy to oblige, because I'd rather have anything at all. :)

> Usually applications that grab a privileged port are short-lived.
> statd is careful to avoid ports in /etc/services because it allocates
> one socket with a privileged port for contacting the kernel, and keeps
> it in place.

The other long-lived libtirpc instance that is causing problems for
us is rpcbind. And fixing this at the root seemed better than patching
just rpcbind.

> > diff --git a/src/bindresvport.c b/src/bindresvport.c
> > index 2d8f2bc..98e5f40 100644
> > --- a/src/bindresvport.c
> > +++ b/src/bindresvport.c
> > @@ -125,6 +129,25 @@ bindresvport_sa(sd, sa)
> > }
> > sa->sa_family = af;
> >
> > + so_proto_len = sizeof(so_proto);
> > + if (getsockopt(sd, SOL_SOCKET, SO_PROTOCOL, &so_proto, &so_proto_len) == -1) {
> > + mutex_unlock(&port_lock);
> > + return -1; /* errno is correctly set */
> > + }
> > + switch (so_proto) {
> > + case IPPROTO_UDP:
> > + case IPPROTO_UDPLITE:
>
> What is UDPLITE?

<http://man7.org/linux/man-pages/man7/udplite.7.html>. I was trying to
make this as generic as possible, but checking again now, I guess this
might deserve a new protocol name in /etc/services anyway.

> Would it require a separate netid
> or other infrastructure? IMO it's not correct to add
> this transport protocol in this patch. If you resend,
> please remove this line.

Yeah, will drop it. The one not being handled because I was not sure
how, that does appear for example on /etc/services on a Debian system
is "ddp", but given that the library is for SunRPC there's not much
point in trying to handle that case here anyway.

Thanks,
Guillem

2018-01-12 18:41:56

by Guillem Jover

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On Thu, 2018-01-11 at 10:18:46 -0500, Steve Dickson wrote:
> Overall I think this makes sense, but this eliminates 240 privilege
> ports and worried we would run out of port (due to them in TIME_WAIT)
> during a v3 mount storms. A port goes into TIME_WAIT after a v3 mount
> is done... But on the other hand v3 is no longer the default and
> there are 784 available ports.... Hopefully that is enough.

Hmm, those numbers do not match my own. bindresvport() uses the port
range between 512 and 1023 inclusive. On my Debian stable (stretch)
and unstable systems these are the number of registered ports in
/etc/services:

,---
# UDP
$ awk '/^[^#]/ { print $2 }' /etc/services | \
sed -n -e 's,/udp,,p' | \
while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
then echo $port; fi; done | sort -u | wc -l
31
# TCP
$ awk '/^[^#]/ { print $2 }' /etc/services | \
sed -n -e 's,/tcp,,p' | \
while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
then echo $port; fi; done | sort -u | wc -l
48
`---

So that's pretty low. Not as low as the 9 listed in
/etc/bindresvport.blacklist, but as I've mentioned on the other reply,
given that the list is incomplete and that does not support dynamically
registering the ports being actively used on the current system with a
".d/" style fragments directory, it would eventually require to ship
all the ports already listed in /etc/services anyway, which gains us
nothing.

In addition I notice now that even the comment in that file (at least on
Debian) is bogus, as it does not account for ports between 512 and 599:

,--- /etc/bindresvport.blacklist
#
# This file contains a list of port numbers between 600 and 1024,
# which should not be used by bindresvport. bindresvport is mostly
# called by RPC services. This mostly solves the problem, that a
# RPC service uses a well known port of another service.
#
`---

:)

Thanks,
Guillem

2018-01-12 19:12:22

by Thorsten Kukuk

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services

On Fri, Jan 12, Guillem Jover wrote:

> On Thu, 2018-01-11 at 10:18:46 -0500, Steve Dickson wrote:
> > Overall I think this makes sense, but this eliminates 240 privilege
> > ports and worried we would run out of port (due to them in TIME_WAIT)
> > during a v3 mount storms. A port goes into TIME_WAIT after a v3 mount
> > is done... But on the other hand v3 is no longer the default and
> > there are 784 available ports.... Hopefully that is enough.
>
> Hmm, those numbers do not match my own. bindresvport() uses the port
> range between 512 and 1023 inclusive. On my Debian stable (stretch)
> and unstable systems these are the number of registered ports in
> /etc/services:
>
> ,---
> # UDP
> $ awk '/^[^#]/ { print $2 }' /etc/services | \
> sed -n -e 's,/udp,,p' | \
> while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
> then echo $port; fi; done | sort -u | wc -l
> 31
> # TCP
> $ awk '/^[^#]/ { print $2 }' /etc/services | \
> sed -n -e 's,/tcp,,p' | \
> while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
> then echo $port; fi; done | sort -u | wc -l
> 48
> `---

This numbers are only low, since Debian is using a hand selected
/etc/services file with most entries missing. But your change
would not be limited to libtirpc on Debian.
I have 276 for TCP and 276 for UDP, that's much, much more. So
already about 50% of the available range.

Thorsten

--
Thorsten Kukuk, Distinguished Engineer, Senior Architect SLES & CaaSP
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

2018-01-12 19:12:57

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services



> On Jan 12, 2018, at 1:05 PM, Guillem Jover <[email protected]> wrote:
>=20
> Hi!
>=20
> On Thu, 2018-01-11 at 10:50:14 -0500, Chuck Lever wrote:
>> On Jan 9, 2018, at 7:49 PM, Guillem Jover <[email protected]> wrote:
>>> When using the bindresvport() function a privileged port will be =
looked
>>> for and bound to a socket. The problem is that any service using a =
static
>>> privileged port registered in the /etc/services file, can get its =
port
>>> taken over by libtirpc users, making the other service fail to =
start.
>>>=20
>>> Starting the other service before libtircp users is not an option as
>>> this does not cover restart situations, for example during package
>>> upgrades or HA switchovers and similar.
>>>=20
>>> In addition honoring the /etc/services registry is already done for
>>> example by rpc.statd, so it seems obvious to make libtirpc follow =
this
>>> same pattern too.
>>=20
>> Does the glibc version of bindresvport(3) skip ports? I ask because
>> this hasn't been a noted problem before. Which service exactly is
>> causing the difficulty?
>=20
> So, this is all a bit of a mess, and apparently it has been known in
> some quarters for quite some time. :(
>=20
> This was reported to glibc a long time ago [B] but at the time =
upstream
> bogusly rejected the idea of using /etc/services (or similar). Instead
> someone had to create a workaround to overcome this [P], a daemon =
which
> binds to a port, until the other service requests the port to be =
released
> so that it can itself bind to it, which is still obviously prone to =
races
> during restarts and complicates things.
>=20
> [B] <https://sourceware.org/bugzilla/show_bug.cgi?id=3D1014>
> <https://bugzilla.redhat.com/show_bug.cgi?id=3D103401>
> <https://bugzilla.novell.com/show_bug.cgi?id=3D262341>
> <https://bugs.debian.org/638322>
> [P] <http://cyberelk.net/tim/software/portreserve/>
>=20
> Then some distributions instead patched their glibc to honor a new
> /etc/bindresvport.blacklist file in the bindresvport() function; =
OpenSUSE
> at least, and apparently Debian too [F] (although this is not =
documented
> anywhere). In addition nfs-utils upstream (where an rpc.statd comes =
from)
> already honors /etc/services. To complicate things, rpcbind and other
> SunRPC clients using libtirpc do not honor neither /etc/services nor
> /etc/bindresvport.blacklist, because libtirpc has its own =
bindrecvport()
> implementation.

Hi Guillem-

rpc.statd was a local fix to acquire a single port for one long
lived socket. It's not a solution intended to be a generic API
for all RPC applications on the system. Using /etc/services is
completely adequate for this purpose.

If bindresvport(3) is changed to avoid well-known ports,
rpc.statd could easily be converted to use it, for example.


> [F] =
<https://sources.debian.org/src/glibc/2.26-3/debian/patches/any/local-bind=
resvport_blacklist.diff/>
>=20
> On the above Debian bug report, it was proposed to make libtirpc =
switch
> to use the libc bindresvport() implementation so that at least on =
those
> distributions where it is locally patched it would honor the
> /etc/bindresvport.blacklist file. The problem with this, is of course
> that it does not help any upstream code on any other non-patched =
system.

The community issue here is that there have evolved, over time,
multiple RPC libraries with divergent capabilities. The only way
to truly address this confusion is to eliminate all but one of
them, which is far outside the scope of your bug fix. For now we
have to live with it.


> So I decided against using the undocumented =
/etc/bindresvport.blacklist,
> because that requires filling it up with ports that are already =
present
> in /etc/services, which seems pointless.

It's not pointless if the two sets of ports are typically not
entirely conjunct.


> It does not support a fragments
> style directory such as /etc/bindresvport.blacklist.d/ anyway which
> makes "registering" the ports from each package too difficult.

That would be simple enough to add. Again, not really a reason
not to go with a black list. And /etc/services has the same
issue.


> And
> because nfs-utils' rpc.statd will honor /etc/services on any system
> regardless of the libc implementation, which is not the case for
> /etc/bindresvport.blacklist.

As above, rpc.statd can be converted to use a bindresvport(3)
that avoids well-known ports quite easily.


> But at this point, if you disagree and prefer some other solution, I'm
> happy to oblige, because I'd rather have anything at all. :)

I agree there's an issue that needs a solution.


>> Usually applications that grab a privileged port are short-lived.
>> statd is careful to avoid ports in /etc/services because it allocates
>> one socket with a privileged port for contacting the kernel, and =
keeps
>> it in place.
>=20
> The other long-lived libtirpc instance that is causing problems for
> us is rpcbind. And fixing this at the root seemed better than patching
> just rpcbind.

Perhaps. Callers that need only a short-lived socket can work
without any restrictions. The issue is those one or two users
that create a long-lived socket with a privileged port and then
drop privileges. Perhaps a better solution for them is to retain
CAP_NET_BIND_SERVICE and use short-lived sockets instead.


>>> diff --git a/src/bindresvport.c b/src/bindresvport.c
>>> index 2d8f2bc..98e5f40 100644
>>> --- a/src/bindresvport.c
>>> +++ b/src/bindresvport.c
>>> @@ -125,6 +129,25 @@ bindresvport_sa(sd, sa)
>>> }
>>> sa->sa_family =3D af;
>>>=20
>>> + so_proto_len =3D sizeof(so_proto);
>>> + if (getsockopt(sd, SOL_SOCKET, SO_PROTOCOL, &so_proto, =
&so_proto_len) =3D=3D -1) {
>>> + mutex_unlock(&port_lock);
>>> + return -1; /* errno is correctly set */
>>> + }
>>> + switch (so_proto) {
>>> + case IPPROTO_UDP:
>>> + case IPPROTO_UDPLITE:
>>=20
>> What is UDPLITE?
>=20
> <http://man7.org/linux/man-pages/man7/udplite.7.html>. I was trying to
> make this as generic as possible, but checking again now, I guess this
> might deserve a new protocol name in /etc/services anyway.
>=20
>> Would it require a separate netid
>> or other infrastructure? IMO it's not correct to add
>> this transport protocol in this patch. If you resend,
>> please remove this line.
>=20
> Yeah, will drop it. The one not being handled because I was not sure
> how, that does appear for example on /etc/services on a Debian system
> is "ddp", but given that the library is for SunRPC there's not much
> point in trying to handle that case here anyway.
>=20
> Thanks,
> Guillem

--
Chuck Lever




2018-01-12 19:26:53

by Tom Talpey

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On 1/12/2018 1:41 PM, Guillem Jover wrote:
> On Thu, 2018-01-11 at 10:18:46 -0500, Steve Dickson wrote:
>> Overall I think this makes sense, but this eliminates 240 privilege
>> ports and worried we would run out of port (due to them in TIME_WAIT)
>> during a v3 mount storms. A port goes into TIME_WAIT after a v3 mount
>> is done... But on the other hand v3 is no longer the default and
>> there are 784 available ports.... Hopefully that is enough.
>
> Hmm, those numbers do not match my own. bindresvport() uses the port
> range between 512 and 1023 inclusive. On my Debian stable (stretch)

Properly speaking, no service should be binding to any port but the
one it is assigned to. This includes 0-1023 as well as 1024-49151.

https://tools.ietf.org/html/rfc6335

See last quoted sentence from:

https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml

>> Service names are assigned on a first-come, first-served process, as
>> documented in [RFC6335].
>>
>> Port numbers are assigned in various ways, based on three ranges: System
>> Ports (0-1023), User Ports (1024-49151), and the Dynamic and/or Private
>> Ports (49152-65535); the difference uses of these ranges is described in
>> [RFC6335]. According to Section 8.1.2 of [RFC6335], System Ports are
>> assigned by the "IETF Review" or "IESG Approval" procedures described in
>> [RFC8126]. User Ports are assigned by IANA using the "IETF Review" process,
>> the "IESG Approval" process, or the "Expert Review" process, as per
>> [RFC6335]. Dynamic Ports are not assigned.
>>
>> The registration procedures for service names and port numbers are
>> described in [RFC6335].
>>
>> Assigned ports both System and User ports SHOULD NOT be used without
>> or prior to IANA registration.

Tom.




> and unstable systems these are the number of registered ports in
> /etc/services:
>
> ,---
> # UDP
> $ awk '/^[^#]/ { print $2 }' /etc/services | \
> sed -n -e 's,/udp,,p' | \
> while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
> then echo $port; fi; done | sort -u | wc -l
> 31
> # TCP
> $ awk '/^[^#]/ { print $2 }' /etc/services | \
> sed -n -e 's,/tcp,,p' | \
> while read port; do if [ $port -ge 512 -a $port -lt 1024 ]; \
> then echo $port; fi; done | sort -u | wc -l
> 48
> `---
>
> So that's pretty low. Not as low as the 9 listed in
> /etc/bindresvport.blacklist, but as I've mentioned on the other reply,
> given that the list is incomplete and that does not support dynamically
> registering the ports being actively used on the current system with a
> ".d/" style fragments directory, it would eventually require to ship
> all the ports already listed in /etc/services anyway, which gains us
> nothing.
>
> In addition I notice now that even the comment in that file (at least on
> Debian) is bogus, as it does not account for ports between 512 and 599:
>
> ,--- /etc/bindresvport.blacklist
> #
> # This file contains a list of port numbers between 600 and 1024,
> # which should not be used by bindresvport. bindresvport is mostly
> # called by RPC services. This mostly solves the problem, that a
> # RPC service uses a well known port of another service.
> #
> `---
>
> :)
>
> Thanks,
> Guillem
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

2018-01-12 21:11:54

by Thorsten Kukuk

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services

On Fri, Jan 12, Chuck Lever wrote:

> > On Jan 12, 2018, at 1:05 PM, Guillem Jover <[email protected]> wrote:

> > [F] <https://sources.debian.org/src/glibc/2.26-3/debian/patches/any/local-bindresvport_blacklist.diff/>
> >
> > On the above Debian bug report, it was proposed to make libtirpc switch
> > to use the libc bindresvport() implementation so that at least on those
> > distributions where it is locally patched it would honor the
> > /etc/bindresvport.blacklist file. The problem with this, is of course
> > that it does not help any upstream code on any other non-patched system.
>
> The community issue here is that there have evolved, over time,
> multiple RPC libraries with divergent capabilities. The only way
> to truly address this confusion is to eliminate all but one of
> them, which is far outside the scope of your bug fix. For now we
> have to live with it.

openSUSE is removing sunrpc from glibc, Fedora seems to be removing
sunrpc from glibc, so it's only a matter of time when libtirpc is the
only RPC implementation used on Linux.

Thorsten

--
Thorsten Kukuk, Distinguished Engineer, Senior Architect SLES & CaaSP
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

2018-01-12 21:15:20

by Chuck Lever

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services



> On Jan 12, 2018, at 4:12 PM, Thorsten Kukuk <[email protected]> wrote:
>=20
> On Fri, Jan 12, Chuck Lever wrote:
>=20
>>> On Jan 12, 2018, at 1:05 PM, Guillem Jover <[email protected]> =
wrote:
>=20
>>> [F] =
<https://sources.debian.org/src/glibc/2.26-3/debian/patches/any/local-bind=
resvport_blacklist.diff/>
>>>=20
>>> On the above Debian bug report, it was proposed to make libtirpc =
switch
>>> to use the libc bindresvport() implementation so that at least on =
those
>>> distributions where it is locally patched it would honor the
>>> /etc/bindresvport.blacklist file. The problem with this, is of =
course
>>> that it does not help any upstream code on any other non-patched =
system.
>>=20
>> The community issue here is that there have evolved, over time,
>> multiple RPC libraries with divergent capabilities. The only way
>> to truly address this confusion is to eliminate all but one of
>> them, which is far outside the scope of your bug fix. For now we
>> have to live with it.
>=20
> openSUSE is removing sunrpc from glibc, Fedora seems to be removing
> sunrpc from glibc, so it's only a matter of time when libtirpc is the
> only RPC implementation used on Linux.

There are other RPC implementations, for example:

- The nonstandard TI-RPC library used internally by Ganesha

- The RPC librar(ies) buried in Kerberos implementations

So there's more to do than simply removing the one in glibc,
unfortunately.


--
Chuck Lever




2018-01-12 21:30:26

by Matt Benjamin

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services

Hi Chuck,

On Fri, Jan 12, 2018 at 4:14 PM, Chuck Lever <[email protected]> wrote:

>>
>> openSUSE is removing sunrpc from glibc, Fedora seems to be removing
>> sunrpc from glibc, so it's only a matter of time when libtirpc is the
>> only RPC implementation used on Linux.
>
> There are other RPC implementations, for example:
>
> - The nonstandard TI-RPC library used internally by Ganesha
>

The current idea behind libntirpc is that it has no intent, and
provides no trivial ability, to serve as the ONC RPC implementation
for any Linux application services. It is, in other words, a
3rd-party component from the Linux system point of view.

Matt

> - The RPC librar(ies) buried in Kerberos implementations
>
> So there's more to do than simply removing the one in glibc,
> unfortunately.

>
>
> --
> Chuck Lever


--

Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel. 734-821-5101
fax. 734-769-8938
cel. 734-216-5309

2018-01-12 22:08:55

by Steve Dickson

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services



On 01/12/2018 04:12 PM, Thorsten Kukuk wrote:
> On Fri, Jan 12, Chuck Lever wrote:
>
>>> On Jan 12, 2018, at 1:05 PM, Guillem Jover <[email protected]> wrote:
>
>>> [F] <https://sources.debian.org/src/glibc/2.26-3/debian/patches/any/local-bindresvport_blacklist.diff/>
>>>
>>> On the above Debian bug report, it was proposed to make libtirpc switch
>>> to use the libc bindresvport() implementation so that at least on those
>>> distributions where it is locally patched it would honor the
>>> /etc/bindresvport.blacklist file. The problem with this, is of course
>>> that it does not help any upstream code on any other non-patched system.
>>
>> The community issue here is that there have evolved, over time,
>> multiple RPC libraries with divergent capabilities. The only way
>> to truly address this confusion is to eliminate all but one of
>> them, which is far outside the scope of your bug fix. For now we
>> have to live with it.
>
> openSUSE is removing sunrpc from glibc, Fedora seems to be removing
> sunrpc from glibc, so it's only a matter of time when libtirpc is the
> only RPC implementation used on Linux.
We are...
https://bugzilla.redhat.com/show_bug.cgi?id=1531540

We also adding a new package that will contain rpcgen
https://bugzilla.redhat.com/show_bug.cgi?id=1532364

based off Thorsten's git tree
https://github.com/thkukuk/rpcsvc-proto

steved.

2018-02-08 18:07:50

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On Fri, Jan 12, 2018 at 2:19 PM, Tom Talpey <[email protected]> wrote:
> On 1/12/2018 1:41 PM, Guillem Jover wrote:
>>
>> On Thu, 2018-01-11 at 10:18:46 -0500, Steve Dickson wrote:
>>>
>>> Overall I think this makes sense, but this eliminates 240 privilege
>>> ports and worried we would run out of port (due to them in TIME_WAIT)
>>> during a v3 mount storms. A port goes into TIME_WAIT after a v3 mount
>>> is done... But on the other hand v3 is no longer the default and
>>> there are 784 available ports.... Hopefully that is enough.
>>
>>
>> Hmm, those numbers do not match my own. bindresvport() uses the port
>> range between 512 and 1023 inclusive. On my Debian stable (stretch)
>
>
> Properly speaking, no service should be binding to any port but the
> one it is assigned to. This includes 0-1023 as well as 1024-49151.
>
> https://tools.ietf.org/html/rfc6335
>
> See last quoted sentence from:
>
> https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml
>
>>> Service names are assigned on a first-come, first-served process, as
>>> documented in [RFC6335].
>>>
>>> Port numbers are assigned in various ways, based on three ranges: System
>>> Ports (0-1023), User Ports (1024-49151), and the Dynamic and/or Private
>>> Ports (49152-65535); the difference uses of these ranges is described in
>>> [RFC6335]. According to Section 8.1.2 of [RFC6335], System Ports are
>>> assigned by the "IETF Review" or "IESG Approval" procedures described in
>>> [RFC8126]. User Ports are assigned by IANA using the "IETF Review" process,
>>> the "IESG Approval" process, or the "Expert Review" process, as per
>>> [RFC6335]. Dynamic Ports are not assigned.
>>>
>>> The registration procedures for service names and port numbers are
>>> described in [RFC6335].
>>>
>>> Assigned ports both System and User ports SHOULD NOT be used without
>>> or prior to IANA registration.
>
>
> Tom.

At Steve's request, I've filed a bug against libtirpc:

https://bugzilla.linux-nfs.org/show_bug.cgi?id=320

to document the issue and the rationales for alternate solutions.

I'm fairly confident that bindresvport(3) is never necessary for
svc_tli_create(3). It is arguably also not appropriate for
clnt_tli_create(3). Therefore IMO it should be removed from those code
paths. That by itself would provide some relief and would eliminate
the need to alter the current bindresvport(3) implementation (say, by
introducing a blacklisting mechanism).

As Tom mentioned above, RFC 6335 describes a port range that will
never interfere with service ports allocated by IANA. This range is
known as the Dynamic or Private port range (49152 - 65535). On my
systems, bind(3) already frequently allocates from the Dynamic port
range purely by chance.

To completely prevent the accidental allocation of a well-known
service port, a special internal wrapper for the bind(3) system call
(similar to bindresvport(3)) can be constructed for libtirpc that
allocates only from the Dynamic port range whenever a caller does not
specify a port, making libtirpc adhere to the Best Common Practices
outlined in RFC 6335, and thereby closing this window for user space
RPC services completely.

A more thorough solution IMO would be to fix up the bind(3) system
call to allocate only from the Dynamic port range whenever a caller
does not specify a port. This would take care of not only user space
RPC services but of all dynamically allocated ports on the system,
including ports dynamically allocated in the kernel.

--
"We cannot take our next breath without the exhale."
-- Ellen Scott Grable

2018-02-08 18:36:22

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services



> On Feb 8, 2018, at 1:07 PM, Chuck Lever <[email protected]> wrote:
>=20
> On Fri, Jan 12, 2018 at 2:19 PM, Tom Talpey <[email protected]> wrote:
>> On 1/12/2018 1:41 PM, Guillem Jover wrote:
>>>=20
>>> On Thu, 2018-01-11 at 10:18:46 -0500, Steve Dickson wrote:
>>>>=20
>>>> Overall I think this makes sense, but this eliminates 240 privilege
>>>> ports and worried we would run out of port (due to them in =
TIME_WAIT)
>>>> during a v3 mount storms. A port goes into TIME_WAIT after a v3 =
mount
>>>> is done... But on the other hand v3 is no longer the default and
>>>> there are 784 available ports.... Hopefully that is enough.
>>>=20
>>>=20
>>> Hmm, those numbers do not match my own. bindresvport() uses the port
>>> range between 512 and 1023 inclusive. On my Debian stable (stretch)
>>=20
>>=20
>> Properly speaking, no service should be binding to any port but the
>> one it is assigned to. This includes 0-1023 as well as 1024-49151.
>>=20
>> https://tools.ietf.org/html/rfc6335
>>=20
>> See last quoted sentence from:
>>=20
>> =
https://www.iana.org/assignments/service-names-port-numbers/service-names-=
port-numbers.xhtml
>>=20
>>>> Service names are assigned on a first-come, first-served process, =
as
>>>> documented in [RFC6335].
>>>>=20
>>>> Port numbers are assigned in various ways, based on three ranges: =
System
>>>> Ports (0-1023), User Ports (1024-49151), and the Dynamic and/or =
Private
>>>> Ports (49152-65535); the difference uses of these ranges is =
described in
>>>> [RFC6335]. According to Section 8.1.2 of [RFC6335], System Ports =
are
>>>> assigned by the "IETF Review" or "IESG Approval" procedures =
described in
>>>> [RFC8126]. User Ports are assigned by IANA using the "IETF Review" =
process,
>>>> the "IESG Approval" process, or the "Expert Review" process, as per
>>>> [RFC6335]. Dynamic Ports are not assigned.
>>>>=20
>>>> The registration procedures for service names and port numbers are
>>>> described in [RFC6335].
>>>>=20
>>>> Assigned ports both System and User ports SHOULD NOT be used =
without
>>>> or prior to IANA registration.
>>=20
>>=20
>> Tom.
>=20
> At Steve's request, I've filed a bug against libtirpc:
>=20
> https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D320
>=20
> to document the issue and the rationales for alternate solutions.
>=20
> I'm fairly confident that bindresvport(3) is never necessary for
> svc_tli_create(3). It is arguably also not appropriate for
> clnt_tli_create(3). Therefore IMO it should be removed from those code
> paths. That by itself would provide some relief and would eliminate
> the need to alter the current bindresvport(3) implementation (say, by
> introducing a blacklisting mechanism).
>=20
> As Tom mentioned above, RFC 6335 describes a port range that will
> never interfere with service ports allocated by IANA. This range is
> known as the Dynamic or Private port range (49152 - 65535). On my
> systems, bind(3) already frequently allocates from the Dynamic port
> range purely by chance.
>=20
> To completely prevent the accidental allocation of a well-known
> service port, a special internal wrapper for the bind(3) system call
> (similar to bindresvport(3)) can be constructed for libtirpc that
> allocates only from the Dynamic port range whenever a caller does not
> specify a port, making libtirpc adhere to the Best Common Practices
> outlined in RFC 6335, and thereby closing this window for user space
> RPC services completely.
>=20
> A more thorough solution IMO would be to fix up the bind(3) system
> call to allocate only from the Dynamic port range whenever a caller
> does not specify a port. This would take care of not only user space
> RPC services but of all dynamically allocated ports on the system,
> including ports dynamically allocated in the kernel.

Replace "bind(3)" with "bind(2)" throughout. Oops.

--
Chuck Lever
[email protected]




2018-03-06 18:09:03

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On Thu, Feb 8, 2018 at 1:36 PM, Chuck Lever <[email protected]> wrote:
>
>
>> On Feb 8, 2018, at 1:07 PM, Chuck Lever <[email protected]> wrote:
>>
>> At Steve's request, I've filed a bug against libtirpc:
>>
>> https://bugzilla.linux-nfs.org/show_bug.cgi?id=320
>>
>> to document the issue and the rationales for alternate solutions.
>>
>> I'm fairly confident that bindresvport(3) is never necessary for
>> svc_tli_create(3). It is arguably also not appropriate for
>> clnt_tli_create(3). Therefore IMO it should be removed from those code
>> paths. That by itself would provide some relief and would eliminate
>> the need to alter the current bindresvport(3) implementation (say, by
>> introducing a blacklisting mechanism).
>>
>> As Tom mentioned above, RFC 6335 describes a port range that will
>> never interfere with service ports allocated by IANA. This range is
>> known as the Dynamic or Private port range (49152 - 65535). On my
>> systems, bind(3) already frequently allocates from the Dynamic port
>> range purely by chance.
>>
>> To completely prevent the accidental allocation of a well-known
>> service port, a special internal wrapper for the bind(3) system call
>> (similar to bindresvport(3)) can be constructed for libtirpc that
>> allocates only from the Dynamic port range whenever a caller does not
>> specify a port, making libtirpc adhere to the Best Common Practices
>> outlined in RFC 6335, and thereby closing this window for user space
>> RPC services completely.
>>
>> A more thorough solution IMO would be to fix up the bind(3) system
>> call to allocate only from the Dynamic port range whenever a caller
>> does not specify a port. This would take care of not only user space
>> RPC services but of all dynamically allocated ports on the system,
>> including ports dynamically allocated in the kernel.
>
> Replace "bind(3)" with "bind(2)" throughout. Oops.
>
> --
> Chuck Lever
> [email protected]

As a first step, Steve has committed patches that address bugzilla 320.
This makes libtirpc avoid allocating reserved ports for RPC service
listeners and long-running clients that request a dynamically assigned
port. svc_tli_create(3) and clnt_tli_create(3) were wrong to use
bindresvport(3) for this purpose.

IMO this addresses the two cases Guillem Jover reported explicitly:
rpc.statd's listener, and the rpcbind CALLIT client. mountd on RHEL
systems appears to use 20048 (though it can use any dynamically-
assigned port) and lockd's service port is assigned by the kernel, so
it is typically outside the reserved port range.

Even so, I can see that making bindresvport(3) more particular
about skipping IANA-assigned ports in the reserved port range might
provide additional benefit/relief.

I checked with a pal on the Solaris NFS team to see if their user
space bindresvport(3) implementation was careful to avoid IANA-
assigned ports. He said it does not currently, but they have
considered it and probably will consider it again because there are
occasional requests for such functionality.

The question now is whether to consult /etc/services, a separate
blacklist, or both, when trolling for an unused reserved port. It seems
easiest to start with /etc/services; then perhaps introduce a blacklist
file/directory later if that is desirable. Other opinions?

Note that neither of these solutions addresses the largest consumer
of dynamically-assigned reserved ports: the kernel NFS client. The
only way we have to address that today is the "noresvport" mount
option. (We could make that the default for Kerberos mounts).

--
"We cannot take our next breath without the exhale."
-- Ellen Scott Grable

2018-03-08 20:24:24

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On Tue, Mar 06, 2018 at 01:09:01PM -0500, Chuck Lever wrote:
> Note that neither of these solutions addresses the largest consumer
> of dynamically-assigned reserved ports: the kernel NFS client. The
> only way we have to address that today is the "noresvport" mount
> option. (We could make that the default for Kerberos mounts).

Makes sense to me.

Looks like knfsd's not helpful here, though: the export option
("secure"/"insecure") defaults to "secure", which always requires a low
port. It should be easy to modify "secure" to mean "require low ports
only for auth_sys/auth_null", and that's probably the right thing to do.

--b.

2018-03-08 21:26:01

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] Do not bind to reserved ports registered in /etc/services

On Thu, Mar 08, 2018 at 03:24:23PM -0500, bfields wrote:
> Looks like knfsd's not helpful here, though: the export option
> ("secure"/"insecure") defaults to "secure", which always requires a low
> port. It should be easy to modify "secure" to mean "require low ports
> only for auth_sys/auth_null", and that's probably the right thing to do.

Disclaimer: totally untested.

--b.

commit ddc2a5f5ce98
Author: J. Bruce Fields <[email protected]>
Date: Thu Mar 8 15:49:48 2018 -0500

nfsd: don't require low ports for gss requests

In a traditional NFS deployment using auth_unix, the clients are trusted
to correctly report the credentials of their logged-in users. The
server assumes that only root on client machines is allowed to send
requests from low-numbered ports, so it can use the originating port
number to distinguish "real" NFS clients from NFS clients run by
ordinary users, to prevent ordinary users from spoofing credentials.

The originating port number on a gss-authenticated request is less
important. The authentication ties the request to a user, and we take
it as proof that that user authorized the request. The low port number
check no longer adds much.

So, don't enforce low port numbers in the auth_gss case.

Signed-off-by: J. Bruce Fields <[email protected]>

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 8aa011820c4a..764e6cae6533 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -87,13 +87,23 @@ nfsd_mode_check(struct svc_rqst *rqstp, struct dentry *dentry,
return nfserr_inval;
}

+static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags)
+{
+ if (flags & NFSEXP_INSECURE_PORT)
+ return true;
+ /* We don't require gss requests to use low ports: */
+ if (rqstp->rq_cred.cr_flavor >= RPC_AUTH_GSS)
+ return true;
+ return test_bit(RQ_SECURE, &rqstp->rq_flags);
+}
+
static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
struct svc_export *exp)
{
int flags = nfsexp_flags(rqstp, exp);

/* Check if the request originated from a secure port. */
- if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && !(flags & NFSEXP_INSECURE_PORT)) {
+ if (!nfsd_originating_port_ok(rqstp, flags)) {
RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
dprintk("nfsd: request from insecure port %s!\n",
svc_print_addr(rqstp, buf, sizeof(buf)));

2018-03-08 21:28:56

by Chuck Lever

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services



> On Mar 8, 2018, at 4:26 PM, J. Bruce Fields <[email protected]> =
wrote:
>=20
> On Thu, Mar 08, 2018 at 03:24:23PM -0500, bfields wrote:
>> Looks like knfsd's not helpful here, though: the export option
>> ("secure"/"insecure") defaults to "secure", which always requires a =
low
>> port. It should be easy to modify "secure" to mean "require low =
ports
>> only for auth_sys/auth_null", and that's probably the right thing to =
do.
>=20
> Disclaimer: totally untested.
>=20
> --b.
>=20
> commit ddc2a5f5ce98
> Author: J. Bruce Fields <[email protected]>
> Date: Thu Mar 8 15:49:48 2018 -0500
>=20
> nfsd: don't require low ports for gss requests
>=20
> In a traditional NFS deployment using auth_unix, the clients are =
trusted
> to correctly report the credentials of their logged-in users. The
> server assumes that only root on client machines is allowed to send
> requests from low-numbered ports, so it can use the originating =
port
> number to distinguish "real" NFS clients from NFS clients run by
> ordinary users, to prevent ordinary users from spoofing =
credentials.
>=20
> The originating port number on a gss-authenticated request is less
> important. The authentication ties the request to a user, and we =
take
> it as proof that that user authorized the request. The low port =
number
> check no longer adds much.
>=20
> So, don't enforce low port numbers in the auth_gss case.
>=20
> Signed-off-by: J. Bruce Fields <[email protected]>

Looks plausible to me, and I like the approach.

Reviewed-by: Chuck Lever <[email protected]>


> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 8aa011820c4a..764e6cae6533 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -87,13 +87,23 @@ nfsd_mode_check(struct svc_rqst *rqstp, struct =
dentry *dentry,
> return nfserr_inval;
> }
>=20
> +static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int =
flags)
> +{
> + if (flags & NFSEXP_INSECURE_PORT)
> + return true;
> + /* We don't require gss requests to use low ports: */
> + if (rqstp->rq_cred.cr_flavor >=3D RPC_AUTH_GSS)
> + return true;
> + return test_bit(RQ_SECURE, &rqstp->rq_flags);
> +}
> +
> static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
> struct svc_export *exp)
> {
> int flags =3D nfsexp_flags(rqstp, exp);
>=20
> /* Check if the request originated from a secure port. */
> - if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && !(flags & =
NFSEXP_INSECURE_PORT)) {
> + if (!nfsd_originating_port_ok(rqstp, flags)) {
> RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
> dprintk("nfsd: request from insecure port %s!\n",
> svc_print_addr(rqstp, buf, sizeof(buf)));
>=20
> =
--------------------------------------------------------------------------=
----
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Libtirpc-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever
[email protected]




2018-03-08 21:35:19

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [Libtirpc-devel] [PATCH] Do not bind to reserved ports registered in /etc/services

On Thu, Mar 08, 2018 at 04:28:53PM -0500, Chuck Lever wrote:
>
>
> > On Mar 8, 2018, at 4:26 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > On Thu, Mar 08, 2018 at 03:24:23PM -0500, bfields wrote:
> >> Looks like knfsd's not helpful here, though: the export option
> >> ("secure"/"insecure") defaults to "secure", which always requires a low
> >> port. It should be easy to modify "secure" to mean "require low ports
> >> only for auth_sys/auth_null", and that's probably the right thing to do.
> >
> > Disclaimer: totally untested.
> >
> > --b.
> >
> > commit ddc2a5f5ce98
> > Author: J. Bruce Fields <[email protected]>
> > Date: Thu Mar 8 15:49:48 2018 -0500
> >
> > nfsd: don't require low ports for gss requests
> >
> > In a traditional NFS deployment using auth_unix, the clients are trusted
> > to correctly report the credentials of their logged-in users. The
> > server assumes that only root on client machines is allowed to send
> > requests from low-numbered ports, so it can use the originating port
> > number to distinguish "real" NFS clients from NFS clients run by
> > ordinary users, to prevent ordinary users from spoofing credentials.
> >
> > The originating port number on a gss-authenticated request is less
> > important. The authentication ties the request to a user, and we take
> > it as proof that that user authorized the request. The low port number
> > check no longer adds much.
> >
> > So, don't enforce low port numbers in the auth_gss case.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
>
> Looks plausible to me, and I like the approach.
>
> Reviewed-by: Chuck Lever <[email protected]>

Thanks for taking a look. Also thinking something like this for
exports(5):

--b.

commit 4e3583326c19
Author: J. Bruce Fields <[email protected]>
Date: Thu Mar 8 16:32:11 2018 -0500

exports: document change to "insecure" export option

We're changing the kernel to allow gss requests from high ports even
when "secure" is set.

If the change gets backported to distro kernels, the kernel version may
be an imperfect predictor of the behavior, but I think it's the best we
can do. I consider the change a bugfix, so hopefully this is OK.

Signed-off-by: J. Bruce Fields <[email protected]>

diff --git a/utils/exportfs/exports.man b/utils/exportfs/exports.man
index db47dfdee775..1596fd75578b 100644
--- a/utils/exportfs/exports.man
+++ b/utils/exportfs/exports.man
@@ -131,10 +131,12 @@ this way are ro, rw, no_root_squash, root_squash, and all_squash.
understands the following export options:
.TP
.IR secure
-This option requires that requests originate on an Internet port less
-than IPPORT_RESERVED (1024). This option is on by default. To turn it
-off, specify
+This option requires that requests not using gss originate on an
+Internet port less than IPPORT_RESERVED (1024). This option is on by default.
+To turn it off, specify
.IR insecure .
+(NOTE: older kernels (before upstream kernel version 4.17) enforced this
+requirement on gss requests as well.)
.TP
.IR rw
Allow both read and write requests on this NFS volume. The