nfs-utils in openSUSE was recently updated to a version with fsidd and a
routine security audit was performed. No real security issues were
found but a couple of oddities were highlighted. The follow two patches
propose ways to clean up these oddities.
NeilBrown
---
NeilBrown (2):
fsidd: don't use assert() on expr with side-effect.
fsidd: provide better default socket name.
support/reexport/fsidd.c | 38 +++++++++++++++++++++----------------
support/reexport/reexport.c | 3 +++
support/reexport/reexport.h | 2 +-
3 files changed, 26 insertions(+), 17 deletions(-)
--
Signature
Having the default socket name be in the current directory is a poor
choice for a daemon that is expected to run as root.
It is also likely better to use an "abstract" socket name. abstract
names do not exist in the filesystem namespace and are local to a
network namespace. Using an abstract name ensures that the nfsd,
mountd, and fsidd are all in the same network namespace.
This patch:
- uses a single #define for the default socket name, rather than 2;
- allows the socket name to start with '@' which is interpreted to
be a request to use the abstract name space (systemd uses the same
convention).
- changes the default to "@/run/fsid.sock". I don't know of a formal
standard for choosing names in the abstract name space, the defacto
standard (seen in "ss -xa|grep @") is to use a name similar to what
might be used in the filesystem.
Signed-off-by: NeilBrown <[email protected]>
---
support/reexport/fsidd.c | 10 ++++++----
support/reexport/reexport.c | 3 +++
support/reexport/reexport.h | 2 +-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c
index 3fef1ef3512b..37649d065ce6 100644
--- a/support/reexport/fsidd.c
+++ b/support/reexport/fsidd.c
@@ -18,11 +18,10 @@
#include "conffile.h"
#include "reexport_backend.h"
+#include "reexport.h"
#include "xcommon.h"
#include "xlog.h"
-#define FSID_SOCKET_NAME "fsid.sock"
-
static struct event_base *evbase;
static struct reexpdb_backend_plugin *dbbackend = &sqlite_plug_ops;
@@ -167,11 +166,14 @@ int main(void)
sock_file = conf_get_str_with_def("reexport", "fsidd_socket", FSID_SOCKET_NAME);
- unlink(sock_file);
-
memset(&addr, 0, sizeof(struct sockaddr_un));
addr.sun_family = AF_UNIX;
strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
+ if (addr.sun_path[0] == '@')
+ /* "abstract" socket namespace */
+ addr.sun_path[0] = 0;
+ else
+ unlink(sock_file);
srv = socket(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK, 0);
if (srv == -1) {
diff --git a/support/reexport/reexport.c b/support/reexport/reexport.c
index eddc9bf413f6..d597a2f73c93 100644
--- a/support/reexport/reexport.c
+++ b/support/reexport/reexport.c
@@ -38,6 +38,9 @@ static bool connect_fsid_service(void)
memset(&addr, 0, sizeof(struct sockaddr_un));
addr.sun_family = AF_UNIX;
strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
+ if (addr.sun_path[0] == '@')
+ /* "abstract" socket namespace */
+ addr.sun_path[0] = 0;
s = socket(AF_UNIX, SOCK_SEQPACKET, 0);
if (s == -1) {
diff --git a/support/reexport/reexport.h b/support/reexport/reexport.h
index 3bed03a9a0bb..856c3085a1dd 100644
--- a/support/reexport/reexport.h
+++ b/support/reexport/reexport.h
@@ -13,6 +13,6 @@ int reexpdb_fsidnum_by_path(char *path, uint32_t *fsidnum, int may_create);
int reexpdb_apply_reexport_settings(struct exportent *ep, char *flname, int flline);
void reexpdb_uncover_subvolume(uint32_t fsidnum);
-#define FSID_SOCKET_NAME "fsid.sock"
+#define FSID_SOCKET_NAME "@/run/fsid.sock"
#endif /* REEXPORT_H */
assert() is not guaranteed to evaluate its arg. When compiled with
-DNDEBUG, the evaluation is skipped. We don't currently compile with
-DNDEBUG, but relying on that is poor form, particularly as this is
described as "sample code" in the git log.
So introduce assert_safe() and use that when there are side-effects.
Signed-off-by: NeilBrown <[email protected]>
---
support/reexport/fsidd.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c
index 410b3a370d6d..3fef1ef3512b 100644
--- a/support/reexport/fsidd.c
+++ b/support/reexport/fsidd.c
@@ -26,6 +26,11 @@
static struct event_base *evbase;
static struct reexpdb_backend_plugin *dbbackend = &sqlite_plug_ops;
+/* assert_safe() always evalutes it argument, as it might have
+ * a side-effect. assert() won't if compiled with NDEBUG
+ */
+#define assert_safe(__sideeffect) (__sideeffect ? 0 : ({assert(0) ; 0;}))
+
static void client_cb(evutil_socket_t cl, short ev, void *d)
{
struct event *me = d;
@@ -56,12 +61,11 @@ static void client_cb(evutil_socket_t cl, short ev, void *d)
if (dbbackend->fsidnum_by_path(req_path, &fsidnum, false, &found)) {
if (found)
- assert(asprintf(&answer, "+ %u", fsidnum) != -1);
+ assert_safe(asprintf(&answer, "+ %u", fsidnum) != -1);
else
- assert(asprintf(&answer, "+ ") != -1);
-
+ assert_safe(asprintf(&answer, "+ ") != -1);
} else {
- assert(asprintf(&answer, "- %s", "Command failed") != -1);
+ assert_safe(asprintf(&answer, "- %s", "Command failed") != -1);
}
(void)send(cl, answer, strlen(answer), 0);
@@ -78,13 +82,13 @@ static void client_cb(evutil_socket_t cl, short ev, void *d)
if (dbbackend->fsidnum_by_path(req_path, &fsidnum, true, &found)) {
if (found) {
- assert(asprintf(&answer, "+ %u", fsidnum) != -1);
+ assert_safe(asprintf(&answer, "+ %u", fsidnum) != -1);
} else {
- assert(asprintf(&answer, "+ ") != -1);
+ assert_safe(asprintf(&answer, "+ ") != -1);
}
} else {
- assert(asprintf(&answer, "- %s", "Command failed") != -1);
+ assert_safe(asprintf(&answer, "- %s", "Command failed") != -1);
}
(void)send(cl, answer, strlen(answer), 0);
@@ -106,15 +110,15 @@ static void client_cb(evutil_socket_t cl, short ev, void *d)
}
if (bad_input) {
- assert(asprintf(&answer, "- %s", "Command failed: Bad input") != -1);
+ assert_safe(asprintf(&answer, "- %s", "Command failed: Bad input") != -1);
} else {
if (dbbackend->path_by_fsidnum(fsidnum, &path, &found)) {
if (found)
- assert(asprintf(&answer, "+ %s", path) != -1);
+ assert_safe(asprintf(&answer, "+ %s", path) != -1);
else
- assert(asprintf(&answer, "+ ") != -1);
+ assert_safe(asprintf(&answer, "+ ") != -1);
} else {
- assert(asprintf(&answer, "+ ") != -1);
+ assert_safe(asprintf(&answer, "+ ") != -1);
}
}
@@ -129,7 +133,7 @@ static void client_cb(evutil_socket_t cl, short ev, void *d)
} else {
char *answer = NULL;
- assert(asprintf(&answer, "- bad command") != -1);
+ assert_safe(asprintf(&answer, "- bad command") != -1);
(void)send(cl, answer, strlen(answer), 0);
free(answer);
Niel,
----- Ursprüngliche Mail -----
> Von: "NeilBrown" <[email protected]>
> nfs-utils in openSUSE was recently updated to a version with fsidd and a
> routine security audit was performed. No real security issues were
> found but a couple of oddities were highlighted. The follow two patches
> propose ways to clean up these oddities.
Nice! TBH, the series got faster merged than expected. ;-)
Acked-by: Richard Weinberger <[email protected]>
Thanks,
//richard
On 5/8/23 2:19 AM, NeilBrown wrote:
> nfs-utils in openSUSE was recently updated to a version with fsidd and a
> routine security audit was performed. No real security issues were
> found but a couple of oddities were highlighted. The follow two patches
> propose ways to clean up these oddities.
>
> NeilBrown
>
>
> ---
>
> NeilBrown (2):
> fsidd: don't use assert() on expr with side-effect.
> fsidd: provide better default socket name.
>
>
> support/reexport/fsidd.c | 38 +++++++++++++++++++++----------------
> support/reexport/reexport.c | 3 +++
> support/reexport/reexport.h | 2 +-
> 3 files changed, 26 insertions(+), 17 deletions(-)
>
> --
> Signature
>
Committed... (tag: nfs-utils-2-6-4-rc1)
steved