From: Vincent Bernat <vincent@bernat.ch>
Date: Tue, 4 May 2021 13:55:21 +0000 (+0200)
Subject: client: use a dedicated file lock to prevent concurrent changes
X-Git-Tag: 1.0.12~22
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c38c53d012f3331d22ae6d8d7a2b130e72d9afb6;p=thirdparty%2Flldpd.git

client: use a dedicated file lock to prevent concurrent changes

We were using a lock on the Unix socket. This was working on Linux but
this is not portable. Therefore, we have to use a dedicated file for
this purpose. We use /var/lock by default.

We don't do a secure creation as the lock file is only opened in
append mode, so a symlink attack could only create empty file or reset
the timestamp of a file. No content can be erased this way.

Fix #445
---

diff --git a/NEWS b/NEWS
index 88c67a65..b9dd1c37 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,7 @@
+lldpd (1.0.12)
+  * Fix:
+    + Use a dedicated file lock to prevent concurrent changes from lldpcli.
+
 lldpd (1.0.11)
   * Changes:
     + Disable LLDP in firmware for Intel X7xx cards.
diff --git a/configure.ac b/configure.ac
index 5128569d..e7e15965 100644
--- a/configure.ac
+++ b/configure.ac
@@ -351,6 +351,7 @@ fi
 lldp_ARG_WITH([privsep-chroot], [Which directory to use to chroot lldpd], [${runstatedir}/lldpd])
 lldp_ARG_WITH([lldpd-ctl-socket], [Path to socket for communication with lldpd], [${runstatedir}/lldpd.socket])
 lldp_ARG_WITH([lldpd-pid-file], [Path to lldpd PID file], [${runstatedir}/lldpd.pid])
+lldp_ARG_WITH([lldpcli-lock-dir], [Which directory to use to put locks], [${localstatedir}/lock])
 
 # Netlink
 lldp_ARG_WITH_UNQUOTED([netlink-max-receive-bufsize], [Netlink maximum receive buffer size], [1024*1024])
diff --git a/src/client/commands.c b/src/client/commands.c
index 82317679..b7b77dde 100644
--- a/src/client/commands.c
+++ b/src/client/commands.c
@@ -19,8 +19,11 @@
 #include <string.h>
 #include <sys/queue.h>
 #include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
 #include <sys/socket.h>
 #include <sys/un.h>
+#include <libgen.h>
 
 /**
  * An element of the environment (a key and a value).
@@ -372,6 +375,7 @@ _commands_execute(struct lldpctl_conn_t *conn, struct writer *w,
 		.argp = 0
 	};
 	static int lockfd = -1;
+	static char *lockname = NULL;	/* Name of lockfile */
 	cmdenv_push(&env, root);
 	if (!completion)
 		for (n = 0; n < argc; n++)
@@ -445,26 +449,30 @@ _commands_execute(struct lldpctl_conn_t *conn, struct writer *w,
 		/* Push and execute */
 		cmdenv_push(&env, best);
 		if (best->execute) {
-			struct sockaddr_un su;
 			if (needlock) {
 				if (lockfd == -1) {
-					log_debug("lldpctl", "reopen %s for locking", ctlname);
-					if ((lockfd = socket(PF_UNIX, SOCK_STREAM, 0)) == -1) {
-						log_warn("lldpctl", "cannot open for lock %s", ctlname);
+					char *_ctlname = NULL;
+					if (lockname == NULL &&
+					    ((_ctlname = strdup(ctlname)) == NULL ||
+					    asprintf(&lockname, LLDPCLI_LOCK_DIR "/%s.lck",
+					    basename(_ctlname)) == -1)) {
+						log_warnx("lldpctl",
+						    "not enough memory to build lock filename");
 						rc = -1;
+						free(_ctlname);
 						goto end;
 					}
-					su.sun_family = AF_UNIX;
-					strlcpy(su.sun_path, ctlname, sizeof(su.sun_path));
-					if (connect(lockfd, (struct sockaddr *)&su, sizeof(struct sockaddr_un)) == -1) {
-						log_warn("lldpctl", "cannot connect to socket %s", ctlname);
+					free(_ctlname);
+					log_debug("lldpctl", "open %s for locking", lockname);
+					if ((lockfd = open(lockname,
+					    O_CREAT|O_RDWR|O_NOFOLLOW, 0666)) == -1) {
+						log_warn("lldpctl", "cannot open lock %s", lockname);
 						rc = -1;
-						close(lockfd); lockfd = -1;
 						goto end;
 					}
 				}
 				if (lockf(lockfd, F_LOCK, 0) == -1) {
-					log_warn("lldpctl", "cannot get lock on %s", ctlname);
+					log_warn("lldpctl", "cannot get lock on %s", lockname);
 					rc = -1;
 					close(lockfd); lockfd = -1;
 					goto end;
@@ -472,7 +480,7 @@ _commands_execute(struct lldpctl_conn_t *conn, struct writer *w,
 			}
 			rc = best->execute(conn, w, &env, best->arg) != 1 ? -1 : rc;
 			if (needlock && lockf(lockfd, F_ULOCK, 0) == -1) {
-				log_warn("lldpctl", "cannot unlock %s", ctlname);
+				log_warn("lldpctl", "cannot unlock %s", lockname);
 				close(lockfd); lockfd = -1;
 			}
 			if (rc == -1) goto end;