[PATCH] Don't bind to registered ports in bindresvport

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Don't bind to registered ports in bindresvport

Dan Nicholson-2
When bindresvport binds to a random port, there's a good chance it will
pick one already registered in services. That's bad since the point of
services is to define well known ports so random programs know which
ones to avoid. The current behavior causes lots of downstream bugs and
requires hacky solutions like running programs early in boot to bind to
desired ports and handing them off when the actual services start.

https://bugzilla.redhat.com/show_bug.cgi?id=103401

Let's just fix the problem at the source. On my fedora system, 295 of
the 541 ports between 512 and 1023 are unregistered. There's plenty of
space to pick a smarter port. If there are systems that require more
random ports than that, bindresvport is probably not the right API to
use.

2012-05-31  Dan Nicholson  <[hidden email]>

        * sunrpc/bindrsvprt.c (bindresvport): Before binding the port,
        make sure it's not registered in services.
---
 sunrpc/bindrsvprt.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..8594b33 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -35,6 +35,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <netdb.h>
 
 /*
  * Bind a socket to a privileged IP port
@@ -74,15 +75,27 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  struct servent serv, *s;
+  char buf[1024];
  again:
   for (i = 0; i < nports; ++i)
     {
       sin->sin_port = htons (port++);
       if (port > endport)
  port = startport;
-      res = __bind (sd, sin, sizeof (struct sockaddr_in));
-      if (res >= 0 || errno != EADDRINUSE)
- break;
+      __getservbyport_r (sin->sin_port, NULL, &serv, buf, sizeof (buf), &s);
+      if (s != NULL)
+ {
+  /* This port is registered. Fake a return and try the next. */
+  res = -1;
+          __set_errno (EADDRINUSE);
+ }
+      else
+ {
+  res = __bind (sd, sin, sizeof (struct sockaddr_in));
+  if (res >= 0 || errno != EADDRINUSE)
+    break;
+ }
     }
 
   if (i == nports && startport != LOWPORT)
--
1.7.7.6
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-2
On Thu, May 31, 2012 at 11:32 AM, Dan Nicholson <[hidden email]> wrote:

> When bindresvport binds to a random port, there's a good chance it will
> pick one already registered in services. That's bad since the point of
> services is to define well known ports so random programs know which
> ones to avoid. The current behavior causes lots of downstream bugs and
> requires hacky solutions like running programs early in boot to bind to
> desired ports and handing them off when the actual services start.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=103401
>
> Let's just fix the problem at the source. On my fedora system, 295 of
> the 541 ports between 512 and 1023 are unregistered. There's plenty of
> space to pick a smarter port. If there are systems that require more
> random ports than that, bindresvport is probably not the right API to
> use.
>
> 2012-05-31  Dan Nicholson  <[hidden email]>
>
>        * sunrpc/bindrsvprt.c (bindresvport): Before binding the port,
>        make sure it's not registered in services.

This is an application management issue that needs to be handled by
the distributions.

If a service depends on a port between 600-1023 then it must be
started before *any* services that randomly use ports in that range
e.g. via bindrsvprt.

Your patch attempts to encode a loose dependency via a modification to
the behaviour of the implementation and that is unacceptable.

The dependency must be expressed at a higher level e.g. while managing
the services.

A more acceptable patch might:

* Attempt to find a port for which a service is *not* reserved.
* If no such port is found then fallback to the old behaviour.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Dan Nicholson-2
On Thu, May 31, 2012 at 01:16:01PM -0400, Carlos O'Donell wrote:

> On Thu, May 31, 2012 at 11:32 AM, Dan Nicholson <[hidden email]> wrote:
> > When bindresvport binds to a random port, there's a good chance it will
> > pick one already registered in services. That's bad since the point of
> > services is to define well known ports so random programs know which
> > ones to avoid. The current behavior causes lots of downstream bugs and
> > requires hacky solutions like running programs early in boot to bind to
> > desired ports and handing them off when the actual services start.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=103401
> >
> > Let's just fix the problem at the source. On my fedora system, 295 of
> > the 541 ports between 512 and 1023 are unregistered. There's plenty of
> > space to pick a smarter port. If there are systems that require more
> > random ports than that, bindresvport is probably not the right API to
> > use.
> >
> > 2012-05-31  Dan Nicholson  <[hidden email]>
> >
> >        * sunrpc/bindrsvprt.c (bindresvport): Before binding the port,
> >        make sure it's not registered in services.
>
> This is an application management issue that needs to be handled by
> the distributions.
>
> If a service depends on a port between 600-1023 then it must be
> started before *any* services that randomly use ports in that range
> e.g. via bindrsvprt.
>
> Your patch attempts to encode a loose dependency via a modification to
> the behaviour of the implementation and that is unacceptable.
>
> The dependency must be expressed at a higher level e.g. while managing
> the services.
>
> A more acceptable patch might:
>
> * Attempt to find a port for which a service is *not* reserved.
> * If no such port is found then fallback to the old behaviour.

OK, I don't entirely agree with your feelings on managing the issue at
the distro level. However, I see your point that it should fallback to
the old behavior. See below. I ran this through some tests by narrowing
the range down and it seems to work correctly. What do you think?

2012-06-01  Dan Nicholson  <[hidden email]>

        * sunrpc/bindrsvprt.c (bindresvport): Try to choose a port that
        is unregistered in services before falling back to the first
        open port.
---
 sunrpc/bindrsvprt.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..a12ee3e 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -35,6 +35,8 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <rpc/types.h>
+#include <netdb.h>
 
 /*
  * Bind a socket to a privileged IP port
@@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  bool_t chkport = TRUE;
+  struct servent serv;
+  char buf[1024];
  again:
   for (i = 0; i < nports; ++i)
     {
+      struct servent *s = NULL;
+
       sin->sin_port = htons (port++);
       if (port > endport)
  port = startport;
-      res = __bind (sd, sin, sizeof (struct sockaddr_in));
-      if (res >= 0 || errno != EADDRINUSE)
- break;
+      if (chkport)
+ /* Check to see if the port is registered in services. */
+ __getservbyport_r (sin->sin_port, NULL, &serv, buf, sizeof (buf), &s);
+      if (s == NULL)
+ {
+  res = __bind (sd, sin, sizeof (struct sockaddr_in));
+  if (res >= 0 || errno != EADDRINUSE)
+    break;
+ }
     }
 
-  if (i == nports && startport != LOWPORT)
+  if (i == nports)
     {
-      startport = LOWPORT;
-      endport = STARTPORT - 1;
-      nports = STARTPORT - LOWPORT;
-      port = LOWPORT + port % (STARTPORT - LOWPORT);
-      goto again;
+      if (chkport)
+ {
+  /* Try again, but don't bother checking services. */
+  chkport = FALSE;
+  if (startport != LOWPORT)
+    port = (__getpid () % NPORTS) + STARTPORT;
+  else
+    port = LOWPORT + port % (STARTPORT - LOWPORT);
+  goto again;
+ }
+
+      if (startport != LOWPORT)
+ {
+  chkport = TRUE;
+  startport = LOWPORT;
+  endport = STARTPORT - 1;
+  nports = STARTPORT - LOWPORT;
+  port = LOWPORT + port % (STARTPORT - LOWPORT);
+  goto again;
+ }
     }
 
   return res;
--
1.7.7.6
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-2
On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <[hidden email]> wrote:
>> A more acceptable patch might:
>>
>> * Attempt to find a port for which a service is *not* reserved.
>> * If no such port is found then fallback to the old behaviour.
>
> OK, I don't entirely agree with your feelings on managing the issue at
> the distro level. However, I see your point that it should fallback to
> the old behavior. See below. I ran this through some tests by narrowing
> the range down and it seems to work correctly. What do you think?

Thanks, this looks better.

I'd like to see one of the distro maintainers review this also.

Could you please ask Adam Conrad from Canonical if he could review this?

http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers

> 2012-06-01  Dan Nicholson  <[hidden email]>
>
>        * sunrpc/bindrsvprt.c (bindresvport): Try to choose a port that
>        is unregistered in services before falling back to the first
>        open port.

Do you have FSF copyright assignment?

>  sunrpc/bindrsvprt.c |   46 +++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 37 insertions(+), 9 deletions(-)
> diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
> index d493c9f..a12ee3e 100644
> --- a/sunrpc/bindrsvprt.c
> +++ b/sunrpc/bindrsvprt.c
> @@ -35,6 +35,8 @@
>  #include <sys/types.h>
>  #include <sys/socket.h>
>  #include <netinet/in.h>
> +#include <rpc/types.h>
> +#include <netdb.h>
>
>  /*
>  * Bind a socket to a privileged IP port
> @@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
>
>   int nports = ENDPORT - startport + 1;
>   int endport = ENDPORT;
> +  bool_t chkport = TRUE;
> +  struct servent serv;
> +  char buf[1024];

A global static buffer of this size should come from malloc (and get
allocated once).

There is precedence for using alloca, but I'd like to avoid that.

>  again:
>   for (i = 0; i < nports; ++i)
>     {
> +      struct servent *s = NULL;
> +
>       sin->sin_port = htons (port++);
>       if (port > endport)
>        port = startport;
> -      res = __bind (sd, sin, sizeof (struct sockaddr_in));
> -      if (res >= 0 || errno != EADDRINUSE)
> -       break;
> +      if (chkport)
> +       /* Check to see if the port is registered in services. */

Two spaces after period.

> +       __getservbyport_r (sin->sin_port, NULL, &serv, buf, sizeof (buf), &s);
> +      if (s == NULL)
> +       {
> +         res = __bind (sd, sin, sizeof (struct sockaddr_in));
> +         if (res >= 0 || errno != EADDRINUSE)
> +           break;
> +       }
>     }
>
> -  if (i == nports && startport != LOWPORT)
> +  if (i == nports)
>     {
> -      startport = LOWPORT;
> -      endport = STARTPORT - 1;
> -      nports = STARTPORT - LOWPORT;
> -      port = LOWPORT + port % (STARTPORT - LOWPORT);
> -      goto again;
> +      if (chkport)
> +       {
> +         /* Try again, but don't bother checking services. */

Two spaces after period.

> +         chkport = FALSE;
> +         if (startport != LOWPORT)
> +           port = (__getpid () % NPORTS) + STARTPORT;
> +         else
> +           port = LOWPORT + port % (STARTPORT - LOWPORT);
> +         goto again;
> +       }
> +
> +      if (startport != LOWPORT)
> +       {
> +         chkport = TRUE;
> +         startport = LOWPORT;
> +         endport = STARTPORT - 1;
> +         nports = STARTPORT - LOWPORT;
> +         port = LOWPORT + port % (STARTPORT - LOWPORT);
> +         goto again;
> +       }
>     }
>
>   return res;
> --
> 1.7.7.6

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Dan Nicholson-2
On Fri, Jun 01, 2012 at 11:03:53PM -0400, Carlos O'Donell wrote:

> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <[hidden email]> wrote:
> >> A more acceptable patch might:
> >>
> >> * Attempt to find a port for which a service is *not* reserved.
> >> * If no such port is found then fallback to the old behaviour.
> >
> > OK, I don't entirely agree with your feelings on managing the issue at
> > the distro level. However, I see your point that it should fallback to
> > the old behavior. See below. I ran this through some tests by narrowing
> > the range down and it seems to work correctly. What do you think?
>
> Thanks, this looks better.
>
> I'd like to see one of the distro maintainers review this also.
>
> Could you please ask Adam Conrad from Canonical if he could review this?
>
> http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers

I've added him and Jeff Law since I know this has been an issue for both
distros. Here's the fedora bug again:

https://bugzilla.redhat.com/show_bug.cgi?id=103401

And here's the patch their using on Debian/Ubuntu currently to work
around the issue (header says it originates from OpenSUSE):

http://anonscm.debian.org/viewvc/pkg-glibc/glibc-package/trunk/debian/patches/any/local-bindresvport_blacklist.diff?view=markup

> > 2012-06-01  Dan Nicholson  <[hidden email]>
> >
> >        * sunrpc/bindrsvprt.c (bindresvport): Try to choose a port that
> >        is unregistered in services before falling back to the first
> >        open port.
>
> Do you have FSF copyright assignment?

No. I don't mind starting the process, though.

> >  sunrpc/bindrsvprt.c |   46 +++++++++++++++++++++++++++++++++++++---------
> >  1 files changed, 37 insertions(+), 9 deletions(-)
> > diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
> > index d493c9f..a12ee3e 100644
> > --- a/sunrpc/bindrsvprt.c
> > +++ b/sunrpc/bindrsvprt.c
> > @@ -35,6 +35,8 @@
> >  #include <sys/types.h>
> >  #include <sys/socket.h>
> >  #include <netinet/in.h>
> > +#include <rpc/types.h>
> > +#include <netdb.h>
> >
> >  /*
> >  * Bind a socket to a privileged IP port
> > @@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
> >
> >   int nports = ENDPORT - startport + 1;
> >   int endport = ENDPORT;
> > +  bool_t chkport = TRUE;
> > +  struct servent serv;
> > +  char buf[1024];
>
> A global static buffer of this size should come from malloc (and get
> allocated once).
>
> There is precedence for using alloca, but I'd like to avoid that.

I reworked the patch to use malloc with a static buffer and check for
ERANGE from getservbyport_r.

> >  again:
> >   for (i = 0; i < nports; ++i)
> >     {
> > +      struct servent *s = NULL;
> > +
> >       sin->sin_port = htons (port++);
> >       if (port > endport)
> >        port = startport;
> > -      res = __bind (sd, sin, sizeof (struct sockaddr_in));
> > -      if (res >= 0 || errno != EADDRINUSE)
> > -       break;
> > +      if (chkport)
> > +       /* Check to see if the port is registered in services. */
>
> Two spaces after period.

OK.

> > +       __getservbyport_r (sin->sin_port, NULL, &serv, buf, sizeof (buf), &s);
> > +      if (s == NULL)
> > +       {
> > +         res = __bind (sd, sin, sizeof (struct sockaddr_in));
> > +         if (res >= 0 || errno != EADDRINUSE)
> > +           break;
> > +       }
> >     }
> >
> > -  if (i == nports && startport != LOWPORT)
> > +  if (i == nports)
> >     {
> > -      startport = LOWPORT;
> > -      endport = STARTPORT - 1;
> > -      nports = STARTPORT - LOWPORT;
> > -      port = LOWPORT + port % (STARTPORT - LOWPORT);
> > -      goto again;
> > +      if (chkport)
> > +       {
> > +         /* Try again, but don't bother checking services. */
>
> Two spaces after period.

OK. See updated patch below.

2012-06-04  Dan Nicholson  <[hidden email]>

        * sunrpc/bindrsvprt.c (bindresvport): Try to choose a port that
        is unregistered in services before falling back to the first
        open port.
---
 sunrpc/bindrsvprt.c |   63 +++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..1b7a535 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -35,6 +35,8 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <rpc/types.h>
+#include <netdb.h>
 
 /*
  * Bind a socket to a privileged IP port
@@ -74,24 +76,67 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  bool_t chkport = TRUE;
+  struct servent serv;
+  static char *buf;
+  static size_t buflen = 1024;
  again:
   for (i = 0; i < nports; ++i)
     {
+      struct servent *s = NULL;
+
       sin->sin_port = htons (port++);
       if (port > endport)
  port = startport;
-      res = __bind (sd, sin, sizeof (struct sockaddr_in));
-      if (res >= 0 || errno != EADDRINUSE)
- break;
+      if (chkport)
+ {
+  /* Check to see if the port is registered in services.  */
+  if (buf == NULL)
+    buf = malloc (buflen);
+  if (buf != NULL)
+    while (__getservbyport_r (sin->sin_port, NULL, &serv, buf,
+      buflen, &s) == ERANGE)
+      {
+ char *tmpbuf = realloc (buf, 2 * buflen);
+ if (tmpbuf)
+  {
+    buflen = 2 * buflen;
+    buf = tmpbuf;
+  }
+ else
+  break;
+      }
+ }
+      if (s == NULL)
+ {
+  res = __bind (sd, sin, sizeof (struct sockaddr_in));
+  if (res >= 0 || errno != EADDRINUSE)
+    break;
+ }
     }
 
-  if (i == nports && startport != LOWPORT)
+  if (i == nports)
     {
-      startport = LOWPORT;
-      endport = STARTPORT - 1;
-      nports = STARTPORT - LOWPORT;
-      port = LOWPORT + port % (STARTPORT - LOWPORT);
-      goto again;
+      if (chkport)
+ {
+  /* Try again, but don't bother checking services.  */
+  chkport = FALSE;
+  if (startport != LOWPORT)
+    port = (__getpid () % NPORTS) + STARTPORT;
+  else
+    port = LOWPORT + port % (STARTPORT - LOWPORT);
+  goto again;
+ }
+
+      if (startport != LOWPORT)
+ {
+  chkport = TRUE;
+  startport = LOWPORT;
+  endport = STARTPORT - 1;
+  nports = STARTPORT - LOWPORT;
+  port = LOWPORT + port % (STARTPORT - LOWPORT);
+  goto again;
+ }
     }
 
   return res;
--
1.7.7.6
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Jeff Law
On 06/04/2012 09:25 AM, Dan Nicholson wrote:

>> http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers
>
> I've added him and Jeff Law since I know this has been an issue for both
> distros. Here's the fedora bug again:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=103401
>
> And here's the patch their using on Debian/Ubuntu currently to work
> around the issue (header says it originates from OpenSUSE):
It's definitely a problem.  103401 has enough state to get a sense of
the issue.

I haven't looked at this for a while as the current plan didn't require
further action from me.

Specifically for RHEL6 and older we were in the process of auditing the
major daemons to ensure they use portreserve.  While that's far from an
ideal solution (it's race prone and doesn't work well for daemon
restarts), it was deemed the best of a set of bad solutions.

For RHEL7 and beyond we're planning to use capabilities within systemd
to reserve ports.  It's a step forward, but still far from ideal.

Checking /etc/rpc every time something wants to open a rpc port might
get rather expensive and many ports are going to be avoided even though
those services aren't running.  If someone installed an /etc/rpc with
reservations for every port in the IANA database where wouldn't be too
many ports left for bindresvport to use.

We were looking at white/black lists of ports when we were pondering
fixing this in glibc itself.

However, I'll welcome this approach as just about anything would be
better than the current status quo.

And for those who think everything needing a port between 600 & 1023
should start before that rpc services, that's just not feasible in the
real world.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-2
On Tue, Jun 5, 2012 at 2:57 PM, Jeff Law <[hidden email]> wrote:

> On 06/04/2012 09:25 AM, Dan Nicholson wrote:
>
>>> http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers
>>
>>
>> I've added him and Jeff Law since I know this has been an issue for both
>> distros. Here's the fedora bug again:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=103401
>>
>> And here's the patch their using on Debian/Ubuntu currently to work
>> around the issue (header says it originates from OpenSUSE):
>
> It's definitely a problem.  103401 has enough state to get a sense of the
> issue.
>
> I haven't looked at this for a while as the current plan didn't require
> further action from me.
>
> Specifically for RHEL6 and older we were in the process of auditing the
> major daemons to ensure they use portreserve.  While that's far from an
> ideal solution (it's race prone and doesn't work well for daemon restarts),
> it was deemed the best of a set of bad solutions.
>
> For RHEL7 and beyond we're planning to use capabilities within systemd to
> reserve ports.  It's a step forward, but still far from ideal.
>
> Checking /etc/rpc every time something wants to open a rpc port might get
> rather expensive and many ports are going to be avoided even though those
> services aren't running.  If someone installed an /etc/rpc with reservations
> for every port in the IANA database where wouldn't be too many ports left
> for bindresvport to use.
>
> We were looking at white/black lists of ports when we were pondering fixing
> this in glibc itself.
>
> However, I'll welcome this approach as just about anything would be better
> than the current status quo.
>
> And for those who think everything needing a port between 600 & 1023 should
> start before that rpc services, that's just not feasible in the real world.

The currently proposed fix for glibc is an additional heuristic that
slows down bindresvport but provides some protection against the
behaviour of stealing ports for described services. Thus allowing the
distribution or maintainer to manage /etc/rpc in order to provide
protection against service port conflicts when using the bindresvport
API. If managed properly it should solve a lot of headache.
Unfortunately on a system with *lots* of dynamically enabled and
disabled services it doesn't solve the problem.

You *need* a higher level understanding of the problem. You need to
detect that a daemon is using the port you need and have the services
manager use the dependencies to start and stop services in such a way
that it results in a working system.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Dan Nicholson-2
In reply to this post by Jeff Law
On 6/5/12, Jeff Law <[hidden email]> wrote:

> On 06/04/2012 09:25 AM, Dan Nicholson wrote:
>
>>> http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers
>>
>> I've added him and Jeff Law since I know this has been an issue for both
>> distros. Here's the fedora bug again:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=103401
>>
>> And here's the patch their using on Debian/Ubuntu currently to work
>> around the issue (header says it originates from OpenSUSE):
> It's definitely a problem.  103401 has enough state to get a sense of
> the issue.
>
> I haven't looked at this for a while as the current plan didn't require
> further action from me.
>
> Specifically for RHEL6 and older we were in the process of auditing the
> major daemons to ensure they use portreserve.  While that's far from an
> ideal solution (it's race prone and doesn't work well for daemon
> restarts), it was deemed the best of a set of bad solutions.

Right, this is specifically the hack I think should go away.

> For RHEL7 and beyond we're planning to use capabilities within systemd
> to reserve ports.  It's a step forward, but still far from ideal.

Definitely an improvement since systemd can reserve the ports more
cleanly than an out of band process like portreserve, but still you're
just plugging holes, IMO.

> Checking /etc/rpc every time something wants to open a rpc port might
> get rather expensive and many ports are going to be avoided even though
> those services aren't running.

It's actually /etc/services, but I don't think this is really
expensive in the context. Since this is part of nss, it should
essentially be grabbed from a static buffer in libc. In a call to ask
libc to bind a reserved port for you, I don't think it's unexpected
that it will query some part of nss.

> If someone installed an /etc/rpc with
> reservations for every port in the IANA database where wouldn't be too
> many ports left for bindresvport to use.

Originally I did write it that way, but Carlos suggested to change so
that if all the ports are registered that it fall back to just
grabbing the first open port. That's what the current patch does.
There are actually 4 phases:

1. Check between 600 & 1023, skipping open ports and ports registered
in services
2. Check between 512 & 599, skipping open ports and ports registered in services
3. Check between 600 & 1023, skipping open ports
4. Check between 512 & 599, skipping open ports

If you get all the way through that, you'll get EADDRINUSE.

> We were looking at white/black lists of ports when we were pondering
> fixing this in glibc itself.

Here I'm considering /etc/services as the black list. Maintaining a
separate black list seems like it would eventually just become
services given enough time. It's just a matter of time until someone
files a bug asking to reserve 674 for acap or whatever. :) IMO, if you
want a reserved port in the lower range, you get it in services.
That's what it seems to be there for.

> However, I'll welcome this approach as just about anything would be
> better than the current status quo.
>
> And for those who think everything needing a port between 600 & 1023
> should start before that rpc services, that's just not feasible in the
> real world.

Definitely agree with that.

--
Dan
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Petr Baudis
In reply to this post by Carlos O'Donell-2
On Fri, Jun 01, 2012 at 11:03:53PM -0400, Carlos O'Donell wrote:

> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <[hidden email]> wrote:
> > @@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
> >
> >   int nports = ENDPORT - startport + 1;
> >   int endport = ENDPORT;
> > +  bool_t chkport = TRUE;
> > +  struct servent serv;
> > +  char buf[1024];
>
> A global static buffer of this size should come from malloc (and get
> allocated once).

  Is there a reason for explicitly making this non-thread-safe? Won't
that break existing programs?

  (The current code also uses two statics, but running the function
multiple times in parallel shouldn't result in anything worse than
slightly scattered port allocations, if I read the code right.)

> There is precedence for using alloca, but I'd like to avoid that.

  Why? __MAX_ALLOCA_CUTOFF is currently at 65536, so 1024 bytes on
stack should be a non-issue.

--
                                Petr "Pasky" Baudis
        Smart data structures and dumb code works a lot better
        than the other way around.  -- Eric S. Raymond
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-4
On 6/5/2012 8:46 PM, Petr Baudis wrote:

> On Fri, Jun 01, 2012 at 11:03:53PM -0400, Carlos O'Donell wrote:
>> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <[hidden email]> wrote:
>>> @@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
>>>
>>>   int nports = ENDPORT - startport + 1;
>>>   int endport = ENDPORT;
>>> +  bool_t chkport = TRUE;
>>> +  struct servent serv;
>>> +  char buf[1024];
>>
>> A global static buffer of this size should come from malloc (and get
>> allocated once).
>
>   Is there a reason for explicitly making this non-thread-safe? Won't
> that break existing programs?
>
>   (The current code also uses two statics, but running the function
> multiple times in parallel shouldn't result in anything worse than
> slightly scattered port allocations, if I read the code right.)

This function is not listed as being thread-safe or async-signal safe
in any API that I've seen documented.

I do not think the code is thread-safe, for example:

 80       sin->sin_port = htons (port++);
 81       if (port > endport)
 82         port = startport;
 83       res = __bind (sd, sin, sizeof (struct sockaddr_in));
 84       if (res >= 0 || errno != EADDRINUSE)
 85         break;

If N threads all execute port++ on line 80 the values for sin_port
may exceed endport, they are not adjusted, __bind succeeds
with the non-privileged port allocation, and you've violated the API.

>> There is precedence for using alloca, but I'd like to avoid that.
>
>   Why? __MAX_ALLOCA_CUTOFF is currently at 65536, so 1024 bytes on
> stack should be a non-issue.

Using alloca can create a security risk by putting the buffer in
in the same location every time relative to the stack pointer or
call sequence. While with malloc the allocation locations can be
random.

Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
[hidden email]
[hidden email]
+1 (613) 963 1026
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Roland McGrath-4
> Using alloca can create a security risk by putting the buffer in
> in the same location every time relative to the stack pointer or
> call sequence. While with malloc the allocation locations can be
> random.

This kind of rationale is a very generic one.  Throughout libc, we
have decided that using alloca (or VLAs) is the appropriate thing to
do because of the performance benefit despite this kind of concern.
(It's also the case that for reasonably small sizes alloca can "never"
fail--i.e. the only way it can fail is a stack-extending page fault
that cannot be serviced, crashing the program--while using malloc
requires handling the ENOMEM case explicitly.)  There is always a
trade-off between performance and "hardening" and other such concerns.
I think we should be more systematic about this than simply going with
the preference of whoever happens to be writing or reviewing the code.

This particular case is one where the interface is not at all
performance-sensitive (if it were, even considering having it call NSS
modules and such things would be entirely out of the question--as it
stands, I just think it's quite dubious, but others feel differently)
and the API doesn't make it particularly difficult to fail gracefully
for the ENOMEM case.  So that may be good reason to choose malloc over
alloca here.  It's probably right that the decision should be made
case-by-case considering these sorts of issues.  But I don't think the
decision should ever be ad hoc like this.

The existing standard is that using alloca in libc is always fine,
given appropriate use of the size-cutoff macros.  Before objecting to
use of alloca is considered a valid reason to reject a libc change,
we need to have a clear and systematic policy about how the decision
is made.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-4
On 6/6/2012 4:51 PM, Roland McGrath wrote:

>> Using alloca can create a security risk by putting the buffer in
>> in the same location every time relative to the stack pointer or
>> call sequence. While with malloc the allocation locations can be
>> random.
>
> This kind of rationale is a very generic one.  Throughout libc, we
> have decided that using alloca (or VLAs) is the appropriate thing to
> do because of the performance benefit despite this kind of concern.
> (It's also the case that for reasonably small sizes alloca can "never"
> fail--i.e. the only way it can fail is a stack-extending page fault
> that cannot be serviced, crashing the program--while using malloc
> requires handling the ENOMEM case explicitly.)  There is always a
> trade-off between performance and "hardening" and other such concerns.
> I think we should be more systematic about this than simply going with
> the preference of whoever happens to be writing or reviewing the code.
>
> This particular case is one where the interface is not at all
> performance-sensitive (if it were, even considering having it call NSS
> modules and such things would be entirely out of the question--as it
> stands, I just think it's quite dubious, but others feel differently)
> and the API doesn't make it particularly difficult to fail gracefully
> for the ENOMEM case.  So that may be good reason to choose malloc over
> alloca here.  It's probably right that the decision should be made
> case-by-case considering these sorts of issues.  But I don't think the
> decision should ever be ad hoc like this.
>
> The existing standard is that using alloca in libc is always fine,
> given appropriate use of the size-cutoff macros.  Before objecting to
> use of alloca is considered a valid reason to reject a libc change,
> we need to have a clear and systematic policy about how the decision
> is made.

You make a very good point.

First draft:
http://sourceware.org/glibc/wiki/Style_and_Conventions#Alloca_vs._Malloc

Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
[hidden email]
[hidden email]
+1 (613) 963 1026
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Roland McGrath-4
After this, please use an appropriate Subject: for this general discussion.

> First draft:
> http://sourceware.org/glibc/wiki/Style_and_Conventions#Alloca_vs._Malloc

I think policy details should be posted on this list rather than just on
the wiki, for easier discussion.  The wiki is a good place to record
information once we've settled on it, but it's not a good place to hash it
out from scratch.  

The draft doesn't even mention __libc_use_alloca et al.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-4
On 6/6/2012 6:05 PM, Roland McGrath wrote:

> After this, please use an appropriate Subject: for this general
> discussion.
>
>> First draft:
>> http://sourceware.org/glibc/wiki/Style_and_Conventions#Alloca_vs._Malloc
>
>>
> I think policy details should be posted on this list rather than just
> on the wiki, for easier discussion.  The wiki is a good place to
> record information once we've settled on it, but it's not a good
> place to hash it out from scratch.
>
> The draft doesn't even mention __libc_use_alloca et al.

That's because I don't have any idea what that function does :-)

Let's start with this:

~~~
== Alloca vs. Malloc ==

The use of alloca vs. malloc must be evaluated on a case-by-case basis. There are many things to consider when looking at using alloca or malloc.

 * Is this a hot path with a small allocation? If it is then it is very likely a good candidate for using alloca, using malloc would slow down the fast path too much.

 * What is stored in the alloca'd space? Is there a security issue inherent in placing this information at a known offset from the stack pointer? If there is then it might call for using malloc.

 * Is the allocated space large? The use of alloca in glibc is limited and large allocations will need to be taken from malloc.

 * Does the API allow returning ENOMEM? The use of malloc introduces the possibility that the allocation might fail. If the API doesn't allow ENOMEM to be returned then it might be better to use alloca unless there are other circumstances.

At present there is no magic bullet of special procedure for selecting alloca vs. malloc, if there was then we could encode it into this wiki or into a macro.
~~~

What else do we need?

Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
[hidden email]
[hidden email]
+1 (613) 963 1026
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Dan Nicholson-2
In reply to this post by Carlos O'Donell-4
On 6/6/12, Carlos O'Donell <[hidden email]> wrote:

> On 6/5/2012 8:46 PM, Petr Baudis wrote:
>> On Fri, Jun 01, 2012 at 11:03:53PM -0400, Carlos O'Donell wrote:
>>> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <[hidden email]>
>>> wrote:
>>>> @@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
>>>>
>>>>   int nports = ENDPORT - startport + 1;
>>>>   int endport = ENDPORT;
>>>> +  bool_t chkport = TRUE;
>>>> +  struct servent serv;
>>>> +  char buf[1024];
>>>
>>> A global static buffer of this size should come from malloc (and get
>>> allocated once).
>>
>>   Is there a reason for explicitly making this non-thread-safe? Won't
>> that break existing programs?
>>
>>   (The current code also uses two statics, but running the function
>> multiple times in parallel shouldn't result in anything worse than
>> slightly scattered port allocations, if I read the code right.)
>
> This function is not listed as being thread-safe or async-signal safe
> in any API that I've seen documented.

If this really doesn't need to be thread-safe, then you could get rid
of the buffer completely using the non-reentrant getservbyport. It
would simplify the code some.

--
Dan
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Jeff Law
In reply to this post by Carlos O'Donell-4
On 06/06/2012 06:49 PM, Carlos O'Donell wrote:

> The use of alloca vs. malloc must be evaluated on a case-by-case basis. There are many things to consider when looking at using alloca or malloc.
>
>   * Is this a hot path with a small allocation? If it is then it is very likely a good candidate for using alloca, using malloc would slow down the fast path too much.
>
>   * What is stored in the alloca'd space? Is there a security issue inherent in placing this information at a known offset from the stack pointer? If there is then it might call for using malloc.
>
>   * Is the allocated space large? The use of alloca in glibc is limited and large allocations will need to be taken from malloc.
>
>   * Does the API allow returning ENOMEM? The use of malloc introduces the possibility that the allocation might fail. If the API doesn't allow ENOMEM to be returned then it might be better to use alloca unless there are other circumstances.
>
> At present there is no magic bullet of special procedure for selecting alloca vs. malloc, if there was then we could encode it into this wiki or into a macro.
> ~~~
>
> What else do we need?
If the size of the allocation is known, then it is a good candidate for
alloca.

If there is a path by which the allocated space is grown, then it needs
alloca accounting and should conditionally switch to malloc when the
amount of alloca'd space grows too large.

If the size of the allocation is not known apriori, then serious
consideration should be given to using malloc or at least have a path
which falls back to malloc.  This is especially true if the amount of
allocated space is directly or indirectly under user control.  ie, a
format string, buffer for reading files, buffer for network data, etc.

There's been *far* too many cases where an attacker can blow out the
stack resulting in either a DoS attack or via stack shifting open up an
arbitrary write into another thread's stack or the heap.

Jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Mike Frysinger
In reply to this post by Carlos O'Donell-2
On Friday 01 June 2012 23:03:53 Carlos O'Donell wrote:
> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson wrote:
> >   int nports = ENDPORT - startport + 1;
> >   int endport = ENDPORT;
> > +  bool_t chkport = TRUE;
> > +  struct servent serv;
> > +  char buf[1024];
>
> A global static buffer of this size should come from malloc (and get
> allocated once).

eh ?  it's declared on the stack ... not sure what you mean about global or
static.  it's pretty much the same thing as calling alloca(1024).
-mike

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Paul Eggert
In reply to this post by Carlos O'Donell-4
On 06/06/2012 05:49 PM, Carlos O'Donell wrote:
>  * What is stored in the alloca'd space? Is there a security issue
>  inherent in placing this information at a known offset from the
>  stack pointer? If there is then it might call for using malloc.

There is always a security issue in accessing memory, regardless of
whether one uses alloca or simply takes the address of a local
variable.  So I'm not sure why we're picking on alloca here.

>  * Is the allocated space large? The use of alloca in glibc is
>  limited and large allocations will need to be taken from malloc.

__libc_use_alloca does that.

Here is a quick cut at a proposed revised draft.

-----

Here are some things to consider when deciding whether to use
alloca or malloc.

* Do not use alloca to create an array whose size S is such that
  ! libc_use_alloca (S), as large arrays like that may bypass
  stack-overflow checking.

* If the storage may need to outlive the current function, then
  obviously alloca cannot be used.

* If the API does not allow returning a memory-allocation failure
  indication such as ENOMEM, then alloca may be preferable, as malloc
  can fail.

* If this is a hot path with a small allocation, prefer alloca,
  as it is typically much faster.

* When growing a buffer, either on the stack or on the heap,
  watch out for integer overflow when calculating the new size.
  Such overflow should be treated as allocation failure than
  letting the integer wrap around.

* If the size of the buffer is directly or indirectly under user control,
  consider imposing a maximum to help make denial-of-service attacks
  more difficult.

* If this is a hot path and the allocation size is typically small but
  may be large, and is known in advance, you can use the following
  pattern:

    bool use_alloca = __libc_use_alloca (bufsize);
    struct foo *buf = use_alloca ? alloca (bufsize) : malloc (bufsize);
    do_work_with (buf, bufsize);
    if (! use_alloca)
      free (buf);

  This use of alloca is a memory optimization.  That is, the above
  'alloca' example is equivalent to the following:

    struct foo buffer[__MAX_ALLOCA_CUTOFF / sizeof (struct foo)];
    bool use_auto = bufsize <= sizeof buffer;
    struct foo *buf = use_auto ? buffer : malloc (bufsize);
    do_work_with (buf, bufsize);
    if (! use_auto)
      free (buf);

  except that the 'alloca' version consumes only the stack space
  needed, rather than always consuming approximately
  __MAX_ALLOCA_CUTOFF bytes.

* If the amount of storage is not known in advance but may grow
  without bound, you can start with a small buffer on the stack and
  switch to malloc if it is not large enough.  For example:

    struct foo buffer[__MAX_ALLOCA_CUTOFF / sizeof (struct foo)];
    struct foo *buf = buffer;
    size_t bufsize = sizeof buffer;
    void *allocated = NULL;
    size_t needed;
    while (bufsize < (needed = do_work_with (buf, bufsize)))
      {
        buf = realloc (allocated, needed);
        if (! buf)
          {
            needed = 0;
            break;
          }
        allocated = buf;
        bufsize = needed;
      }
    free (allocated);
    return needed; /* This is zero on allocation failure.  */

* You can also grow a single buffer on the stack, so long as it does
  not get too large, by using 'extend_alloca'.  This is a memory
  optimization of the previous example, just as 'alloca' is a memory
  optimization of a local array.  For example:

    size_t bufsize = 256;
    struct foo *buf = alloca (bufsize);
    void *allocated = NULL;
    size_t needed;
    while (bufsize < (needed = do_work_with (buf, bufsize)))
      {
        if (__libc_use_alloca (needed))
          buf = extend_alloca (buf, bufsize, needed);
        else
          {
            buf = realloc (allocated, needed);
            if (! buf)
              {
                needed = 0;
                break;
              }
            bufsize = needed;
          }
      }
    free (allocated);
    return needed; /* This is zero on allocation failure.  */

* To boost performance a bit in the typical case of the above
  examples, you can use __builtin_expect, e.g.,
  "if (__builtin_expect (use_alloca, 1))" instead of just "if (use_alloca)".
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Jakub Jelinek
On Thu, Jun 07, 2012 at 01:10:21AM -0700, Paul Eggert wrote:

>   This use of alloca is a memory optimization.  That is, the above
>   'alloca' example is equivalent to the following:
>
>     struct foo buffer[__MAX_ALLOCA_CUTOFF / sizeof (struct foo)];
>     bool use_auto = bufsize <= sizeof buffer;
>     struct foo *buf = use_auto ? buffer : malloc (bufsize);
>     do_work_with (buf, bufsize);
>     if (! use_auto)
>       free (buf);
>
>   except that the 'alloca' version consumes only the stack space
>   needed, rather than always consuming approximately
>   __MAX_ALLOCA_CUTOFF bytes.

__libc_use_alloca does actually more than that, it isn't a fixed limit
comparison, but if the size is larger than PTHREAD_STACK_MIN / 4,
it limits use of alloca to current thread stack size / 4 (and
__MAX_ALLOCA_CUTOFF as a maximum).

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't bind to registered ports in bindresvport

Carlos O'Donell-4
In reply to this post by Paul Eggert
On 6/7/2012 4:10 AM, Paul Eggert wrote:

> On 06/06/2012 05:49 PM, Carlos O'Donell wrote:
>>  * What is stored in the alloca'd space? Is there a security issue
>>  inherent in placing this information at a known offset from the
>>  stack pointer? If there is then it might call for using malloc.
>
> There is always a security issue in accessing memory, regardless of
> whether one uses alloca or simply takes the address of a local
> variable.  So I'm not sure why we're picking on alloca here.
>
>>  * Is the allocated space large? The use of alloca in glibc is
>>  limited and large allocations will need to be taken from malloc.
>
> __libc_use_alloca does that.
>
> Here is a quick cut at a proposed revised draft.

Your proposed draft is excellent, and a considerable improvement
over my initial draft.

I've moved your revised draft into the portal and after review
I consider it a good final version.

Please review the wiki to ensure correctness in copying:
http://sourceware.org/glibc/wiki/Style_and_Conventions#Alloca_vs._Malloc

I revised the wording slightly here"
~~~
  This use of alloca is a memory optimization.  That is, the above
{{{alloca}}} example is almost equivalent to the following:
{{{
    struct foo buffer[__MAX_ALLOCA_CUTOFF / sizeof (struct foo)];
    bool use_auto = bufsize <= sizeof buffer;
    struct foo *buf = use_auto ? buffer : malloc (bufsize);
    do_work_with (buf, bufsize);
    if (! use_auto)
      free (buf);
}}}
  except that the {{{alloca}}} version consumes only the stack space
needed, rather than always consuming approximately {{{__MAX_ALLOCA_CUTOFF}}}
bytes. Note that this isn't quite equivalent to the previous example
because {{{__libc_use_alloca}}} limits alloca to {{{PTHREAD_STACK_MIN/4}}}
(which may be bigger than {{{__MAX_ALLOCA_CUTOFF}}}) or
{{{<thread stack size>/4}}}, the latter having a maximum bound
of {{{__MAX_ALLOCA_CUTOFF}}}.
~~~

In truth I find the logic in glibc for __libc_use_alloca to be
broken for large PTHREAD_STACK_MIN sizes.

Say your PTHREAD_STACK_MIN is > 4*__MAX_ALLOCA_CUTOFF (no such
machine currently exists, at most IA64 is 196k or ~3*__MAX_ALLOCA_CUTOFF),
say 8*__MAX_ALLOCA_CUTOFF.

Allocations between 1 -> PTHREAD_STACK_MIN/4 or 2*__MAX_ALLOCA_CUTOFF
succeed without any problem.

However, as soon as you allocate PTHREAD_STACK_MIN/4+1 bytes or
2*__MAX_ALLOCA_CUTOFF+1 bytes then the maximum allocation is clamped
down to __MAX_ALLOCA_CUTOFF? That's seems unintuitive to say the least.
Note that it will almost surely clamp to __MAX_ALLOCA_CUTOFF
because a default stackblock for most machines is on the order of
megabytes (2MB min, so 2MB/4 = 512kb > 64kb).

Thankfully no target has PTHREAD_STACK_MIN > 4*__MAX_ALOCA_CUTOFF,
and allocations in that case proceed smoothly from:
1 -> PTHREAD_STACK_MIN/4 < __MAX_ALLOCA_CUTOFF,
up to __MAX_ALLOCA_CUTOFF, at which point you can't allocate more.

Did I read the code wrong?

Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
[hidden email]
[hidden email]
+1 (613) 963 1026
12