For the record, I'm sending an updated patch with static functions.
Signed-off-by: Otto Hollmann <otto.hollmann@xxxxxxxx>
---
src/binddynport.c | 107 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 99 insertions(+), 8 deletions(-)
diff --git a/src/binddynport.c b/src/binddynport.c
index 062629a..90e2748 100644
--- a/src/binddynport.c
+++ b/src/binddynport.c
@@ -37,6 +37,7 @@
#include <unistd.h>
#include <errno.h>
#include <string.h>
+#include <syslog.h>
#include <rpc/rpc.h>
@@ -56,6 +57,84 @@ enum {
NPORTS = ENDPORT - LOWPORT + 1,
};
+/*
+ * This function decodes information about given port from provided array and
+ * return if port is reserved or not.
+ *
+ * @reserved_ports an array of size at least "NPORTS / (8*sizeof(char)) + 1".
+ * @port port number within range LOWPORT and ENDPORT
+ *
+ * Returns 0 if port is not reserved, non-negative if port is reserved.
+ */
+static int is_reserved(char *reserved_ports, int port) {
+ port -= LOWPORT;
+ if (port < 0 || port >= NPORTS)
+ return 0;
+ return reserved_ports[port/(8*sizeof(char))] & 1<<(port%(8*sizeof(char)));
+}
+
+/*
+ * This function encodes information about given *reserved* port into provided
+ * array. Don't call this function for ports which are not reserved.
+ *
+ * @reserved_ports an array of size at least "NPORTS / (8*sizeof(char)) + 1".
+ * @port port number within range LOWPORT and ENDPORT
+ *
+ */
+static void set_reserved(char *reserved_ports, int port) {
+ port -= LOWPORT;
+ if (port < 0 || port >= NPORTS)
+ return;
+ reserved_ports[port/(8*sizeof(char))] |= 1<<(port%(8*sizeof(char)));
+}
+
+/*
+ * Parse local reserved ports obtained from
+ * /proc/sys/net/ipv4/ip_local_reserved_ports into bit array.
+ *
+ * @reserved_ports a zeroed array of size at least
+ * "NPORTS / (8*sizeof(char)) + 1". Will be used for bit-wise encoding of
+ * reserved ports.
+ *
+ * On each call, reserved ports are read from /proc and bit-wise stored into
+ * provided array
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+
+static int parse_reserved_ports(char *reserved_ports) {
+ int from, to;
+ char delimiter = ',';
+ int res;
+ FILE * file_ptr = fopen("/proc/sys/net/ipv4/ip_local_reserved_ports","r");
+ if (file_ptr == NULL) {
+ (void) syslog(LOG_ERR,
+ "Unable to open open /proc/sys/net/ipv4/ip_local_reserved_ports.");
+ return -1;
+ }
+ do {
+ if ((res = fscanf(file_ptr, "%d", &to)) != 1) {
+ if (res == EOF) break;
+ goto err;
+ }
+ if (delimiter != '-') {
+ from = to;
+ }
+ for (int i = from; i <= to; ++i) {
+ set_reserved(reserved_ports, i);
+ }
+ } while ((res = fscanf(file_ptr, "%c", &delimiter)) == 1);
+ if (res != EOF)
+ goto err;
+ fclose(file_ptr);
+ return 0;
+err:
+ (void) syslog(LOG_ERR,
+ "An error occurred while parsing ip_local_reserved_ports.");
+ fclose(file_ptr);
+ return -1;
+}
+
/*
* Bind a socket to a dynamically-assigned IP port.
*
@@ -81,7 +160,8 @@ int __binddynport(int fd)
in_port_t port, *portp;
struct sockaddr *sap;
socklen_t salen;
- int i, res;
+ int i, res, array_size;
+ char *reserved_ports;
if (__rpc_sockisbound(fd))
return 0;
@@ -119,21 +199,32 @@ int __binddynport(int fd)
gettimeofday(&tv, NULL);
seed = tv.tv_usec * getpid();
}
+ array_size = NPORTS / (8*sizeof(char)) + 1;
+ reserved_ports = malloc(array_size);
+ if (!reserved_ports) {
+ goto out;
+ }
+ memset(reserved_ports, 0, array_size);
+ parse_reserved_ports(reserved_ports);
+
port = (rand_r(&seed) % NPORTS) + LOWPORT;
for (i = 0; i < NPORTS; ++i) {
- *portp = htons(port++);
- res = bind(fd, sap, salen);
- if (res >= 0) {
- res = 0;
- break;
+ *portp = htons(port);
+ if (!is_reserved(reserved_ports, port++)) {
+ res = bind(fd, sap, salen);
+ if (res >= 0) {
+ res = 0;
+ break;
+ }
+ if (errno != EADDRINUSE)
+ break;
}
- if (errno != EADDRINUSE)
- break;
if (port > ENDPORT)
port = LOWPORT;
}
out:
+ free(reserved_ports);
mutex_unlock(&port_lock);
return res;
}
--
2.26.2
On Wed, 2022-10-05 at 10:13 +0200, Otto Hollmann wrote:
> Hi Steve
>
> 1) I tested various combinations of ip_local_port_range and
> ip_local_reserved_ports, including edge cases like
> ip_local_port_range
> equal to ip_local_reserved_ports. In all such tests library behaved
> as
> expected. Let me know if I should test anything else.
>
> No, not yet. We wanted to firstly open discussion, but now we can add
> this patch into our distribution.
>
> 2) You are right, there is no reason why it shouldn't be declared as
> static. Should I send updated patch or you will do this minor
> modification yourself?
>
>
> Otto
>
> On Thu, 2022-07-14 at 13:01 -0500, Steve Dickson wrote:
> > Hey,
> >
> > My apologies for taking so long to get to this...
> >
> > A couple questions:
> >
> > 1) How well was tested... Is in your distro already?
> > 2) Those new functions the patch introduces...
> > Don't effect the API? Meaning shouldn't they
> > declared as static?
> >
> > steved.
> >
> > On 6/9/22 4:26 AM, Otto Hollmann wrote:
> > > Read reserved ports from
> > > /proc/sys/net/ipv4/ip_local_reserved_ports,
> > > store them into bit-wise array and before binding to random port
> > > check
> > > if port is not reserved.
> > >
> > >
> > > Currently, there is no way how to reserve ports so then will not
> > > be
> > > used by rpcbind.
> > >
> > > Random ports are opened by rpcbind because of rmtcalls. There is
> > > compile-time flag for disabling them, but in some cases we can
> > > not
> > > simply disable them.
> > >
> > > One solution would be run time option --enable-rmtcalls as
> > > already
> > > discussed, but it was rejected. So if we want to keep rmtcalls
> > > enabled
> > > and also be able to reserve some ports, there is no other way
> > > than
> > > filtering available ports. The easiest and clearest way seems to
> > > be
> > > just respect kernel list of ip_reserved_ports.
> > >
> > > Unfortunately there is one known disadvantage/side effect - it
> > > affects
> > > probability of ports which are right after reserved ones. The
> > > bigger
> > > reserved block is, the higher is probability of selecting
> > > following
> > > unreserved port. But if there is no reserved port, impact of this
> > > patch
> > > is minimal/none.
> > >
> > > Signed-off-by: Otto Hollmann <otto.hollmann@xxxxxxxx>
> > > ---
> > > src/binddynport.c | 107
> > > ++++++++++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 99 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/binddynport.c b/src/binddynport.c
> > > index 062629a..6f78ebe 100644
> > > --- a/src/binddynport.c
> > > +++ b/src/binddynport.c
> > > @@ -37,6 +37,7 @@
> > > #include <unistd.h>
> > > #include <errno.h>
> > > #include <string.h>
> > > +#include <syslog.h>
> > >
> > > #include <rpc/rpc.h>
> > >
> > > @@ -56,6 +57,84 @@ enum {
> > > NPORTS = ENDPORT - LOWPORT + 1,
> > > };
> > >
> > > +/*
> > > + * This function decodes information about given port from
> > > provided array and
> > > + * return if port is reserved or not.
> > > + *
> > > + * @reserved_ports an array of size at least "NPORTS /
> > > (8*sizeof(char)) + 1".
> > > + * @port port number within range LOWPORT and ENDPORT
> > > + *
> > > + * Returns 0 if port is not reserved, non-negative if port is
> > > reserved.
> > > + */
> > > +int is_reserved(char *reserved_ports, int port) {
> > > + port -= LOWPORT;
> > > + if (port < 0 || port >= NPORTS)
> > > + return 0;
> > > + return reserved_ports[port/(8*sizeof(char))] &
> > > 1<<(port%(8*sizeof(char)));
> > > +}
> > > +
> > > +/*
> > > + * This function encodes information about given *reserved* port
> > > into provided
> > > + * array. Don't call this function for ports which are not
> > > reserved.
> > > + *
> > > + * @reserved_ports array TODO .
> > > + * @port port number within range LOWPORT and ENDPORT
> > > + *
> > > + */
> > > +void set_reserved(char *reserved_ports, int port) {
> > > + port -= LOWPORT;
> > > + if (port < 0 || port >= NPORTS)
> > > + return;
> > > + reserved_ports[port/(8*sizeof(char))] |=
> > > 1<<(port%(8*sizeof(char)));
> > > +}
> > > +
> > > +/*
> > > + * Parse local reserved ports obtained from
> > > + * /proc/sys/net/ipv4/ip_local_reserved_ports into bit array.
> > > + *
> > > + * @reserved_ports a zeroed array of size at least
> > > + * "NPORTS / (8*sizeof(char)) + 1". Will be used for bit-wise
> > > encoding of
> > > + * reserved ports.
> > > + *
> > > + * On each call, reserved ports are read from /proc and bit-wise
> > > stored into
> > > + * provided array
> > > + *
> > > + * Returns 0 on success, -1 on failure.
> > > + */
> > > +
> > > +int parse_reserved_ports(char *reserved_ports) {
> > > + int from, to;
> > > + char delimiter = ',';
> > > + int res;
> > > + FILE * file_ptr =
> > > fopen("/proc/sys/net/ipv4/ip_local_reserved_ports","r");
> > > + if (file_ptr == NULL) {
> > > + (void) syslog(LOG_ERR,
> > > + "Unable to open open
> > > /proc/sys/net/ipv4/ip_local_reserved_ports.");
> > > + return -1;
> > > + }
> > > + do {
> > > + if ((res = fscanf(file_ptr, "%d", &to)) != 1) {
> > > + if (res == EOF) break;
> > > + goto err;
> > > + }
> > > + if (delimiter != '-') {
> > > + from = to;
> > > + }
> > > + for (int i = from; i <= to; ++i) {
> > > + set_reserved(reserved_ports, i);
> > > + }
> > > + } while ((res = fscanf(file_ptr, "%c", &delimiter)) ==
> > > 1);
> > > + if (res != EOF)
> > > + goto err;
> > > + fclose(file_ptr);
> > > + return 0;
> > > +err:
> > > + (void) syslog(LOG_ERR,
> > > + "An error occurred while parsing
> > > ip_local_reserved_ports.");
> > > + fclose(file_ptr);
> > > + return -1;
> > > +}
> > > +
> > > /*
> > > * Bind a socket to a dynamically-assigned IP port.
> > > *
> > > @@ -81,7 +160,8 @@ int __binddynport(int fd)
> > > in_port_t port, *portp;
> > > struct sockaddr *sap;
> > > socklen_t salen;
> > > - int i, res;
> > > + int i, res, array_size;
> > > + char *reserved_ports;
> > >
> > > if (__rpc_sockisbound(fd))
> > > return 0;
> > > @@ -119,21 +199,32 @@ int __binddynport(int fd)
> > > gettimeofday(&tv, NULL);
> > > seed = tv.tv_usec * getpid();
> > > }
> > > + array_size = NPORTS / (8*sizeof(char)) + 1;
> > > + reserved_ports = malloc(array_size);
> > > + if (!reserved_ports) {
> > > + goto out;
> > > + }
> > > + memset(reserved_ports, 0, array_size);
> > > + parse_reserved_ports(reserved_ports);
> > > +
> > > port = (rand_r(&seed) % NPORTS) + LOWPORT;
> > > for (i = 0; i < NPORTS; ++i) {
> > > - *portp = htons(port++);
> > > - res = bind(fd, sap, salen);
> > > - if (res >= 0) {
> > > - res = 0;
> > > - break;
> > > + *portp = htons(port);
> > > + if (!is_reserved(reserved_ports, port++)) {
> > > + res = bind(fd, sap, salen);
> > > + if (res >= 0) {
> > > + res = 0;
> > > + break;
> > > + }
> > > + if (errno != EADDRINUSE)
> > > + break;
> > > }
> > > - if (errno != EADDRINUSE)
> > > - break;
> > > if (port > ENDPORT)
> > > port = LOWPORT;
> > > }
> > >
> > > out:
> > > + free(reserved_ports);
> > > mutex_unlock(&port_lock);
> > > return res;
> > > }
> >
>
