Re: [chrony-dev] [GIT] chrony/chrony.git annotated tag 3.5.1 created. 3.5.1

[Vincent Blut <vincent.debian@xxxxxxx>]

Hey there,

On 2020-08-20T08:55+0200, git@xxxxxxxxxxxxx wrote:
> This is an automated email from git. It was generated because a ref
> change was pushed to the "chrony/chrony.git" repository.
>
> The annotated tag, 3.5.1 has been created
>        at  571a7971cad5537a0e98a3c90aa0b02f5d047c3f (tag)
>   tagging  04328ceeadf7e2b68f246cb4deab9348f4e02825 (commit)
>  replaces  3.5
> tagged by  Miroslav Lichvar
>        on  Wed Aug 19 16:53:10 2020 +0200
>
> - Log -----------------------------------------------------------------
> Release 3.5.1
> -----BEGIN PGP SIGNATURE-----
>
> iHIEABECADIWIQSLH0qa2nPUAeMIWgtf8G8puh4BOwUCXz082hQcbWxpY2h2YXJA
> cmVkaGF0LmNvbQAKCRBf8G8puh4BO843AJ97EdjPFcj06vE76JskCH7D+oL5wgCe
> MVXCwbYu4XckS+CoM7MjOF/vNSI=
> =Bhas
> -----END PGP SIGNATURE-----
>
> Miroslav Lichvar (2):
>      main: create new file when writing pidfile

Miroslav, Matthias, thanks a lot for working on this.
I backported this patch to chrony 3.0 for our previous stable release 
(Debian 9). Would someone please check that everything is ok?

Cheers,
Vincent

From f00fed20092b6a42283f29c6ee1f58244d74b545 Mon Sep 17 00:00:00 2001
From: Miroslav Lichvar <mlichvar@xxxxxxxxxx>
Date: Thu, 6 Aug 2020 09:31:11 +0200
Subject: main: create new file when writing pidfile

When writing the pidfile, open the file with the O_CREAT|O_EXCL flags
to avoid following a symlink and writing the PID to an unexpected file,
when chronyd still has the root privileges.

The Linux open(2) man page warns about O_EXCL not working as expected on
NFS versions before 3 and Linux versions before 2.6. Saving pidfiles on
a distributed filesystem like NFS is not generally expected, but if
there is a reason to do that, these old kernel and NFS versions are not
considered to be supported for saving files by chronyd.

This is a minimal backport specific to this issue of the following
commits:
- commit 2fc8edacb810 ("use PATH_MAX")
- commit f4c6a00b2a11 ("logging: call exit() in LOG_Message()")
- commit 7a4c396bba8f ("util: add functions for common file operations")
- commit e18903a6b563 ("switch to new util file functions")

Reported-by: Matthias Gerstner <mgerstner@xxxxxxx>

--- a/logging.c
+++ b/logging.c
@@ -168,6 +168,7 @@ void LOG_Message(LOG_Severity severity,
           log_message(1, severity, buf);
         }
       }
+      exit(1);
       break;
     default:
       assert(0);
--- a/main.c
+++ b/main.c
@@ -288,13 +288,9 @@ write_lockfile(void)
   const char *pidfile = CNF_GetPidFile();
   FILE *out;
 
-  out = fopen(pidfile, "w");
-  if (!out) {
-    LOG_FATAL(LOGF_Main, "could not open lockfile %s for writing", pidfile);
-  } else {
-    fprintf(out, "%d\n", (int)getpid());
-    fclose(out);
-  }
+  out = UTI_OpenFile(NULL, pidfile, NULL, 'W', 0644);
+  fprintf(out, "%d\n", (int)getpid());
+  fclose(out);
 }
 
 /* ================================================== */
--- a/sysincl.h
+++ b/sysincl.h
@@ -36,6 +36,7 @@
 #include <float.h>
 #include <glob.h>
 #include <grp.h>
+#include <limits.h>
 #include <math.h>
 #include <netdb.h>
 #include <netinet/in.h>
--- a/util.c
+++ b/util.c
@@ -1150,6 +1150,101 @@ UTI_CheckDirPermissions(const char *path
 
 /* ================================================== */
 
+static int
+join_path(const char *basedir, const char *name, const char *suffix,
+          char *buffer, size_t length, LOG_Severity severity)
+{
+  const char *sep;
+
+  if (!basedir) {
+    basedir = "";
+    sep = "";
+  } else {
+    sep = "/";
+  }
+
+  if (!suffix)
+    suffix = "";
+
+  if (snprintf(buffer, length, "%s%s%s%s", basedir, sep, name, suffix) >= length) {
+    LOG(severity, "File path %s%s%s%s too long", basedir, sep, name, suffix);
+    return 0;
+  }
+
+  return 1;
+}
+
+/* ================================================== */
+
+FILE *
+UTI_OpenFile(const char *basedir, const char *name, const char *suffix,
+             char mode, mode_t perm)
+{
+  const char *file_mode;
+  char path[PATH_MAX];
+  LOG_Severity severity;
+  int fd, flags;
+  FILE *file;
+
+  severity = mode >= 'A' && mode <= 'Z' ? LOGS_FATAL : LOGS_ERR;
+
+  if (!join_path(basedir, name, suffix, path, sizeof (path), severity))
+    return NULL;
+
+  switch (mode) {
+    case 'r':
+    case 'R':
+      flags = O_RDONLY;
+      file_mode = "r";
+      if (severity != LOGS_FATAL)
+        severity = LOGS_DEBUG;
+      break;
+    case 'w':
+    case 'W':
+      flags = O_WRONLY | O_CREAT | O_EXCL;
+      file_mode = "w";
+      break;
+    case 'a':
+    case 'A':
+      flags = O_WRONLY | O_CREAT | O_APPEND;
+      file_mode = "a";
+      break;
+    default:
+      assert(0);
+      return NULL;
+  }
+
+try_again:
+  fd = open(path, flags, perm);
+  if (fd < 0) {
+    if (errno == EEXIST) {
+      if (unlink(path) < 0) {
+        LOG(severity, "Could not remove %s : %s", path, strerror(errno));
+        return NULL;
+      }
+      DEBUG_LOG(LOGF_Util, "Removed %s", path);
+      goto try_again;
+    }
+    LOG(severity, "Could not open %s : %s", path, strerror(errno));
+    return NULL;
+  }
+
+  UTI_FdSetCloexec(fd);
+
+  file = fdopen(fd, file_mode);
+  if (!file) {
+    LOG(severity, "Could not open %s : %s", path, strerror(errno));
+    close(fd);
+    return NULL;
+  }
+
+  DEBUG_LOG(LOGF_Util, "Opened %s fd=%d mode=%c", path, fd, mode);
+
+  return file;
+}
+
+/* ================================================== */
+
 void
 UTI_DropRoot(uid_t uid, gid_t gid)
 {
--- a/util.h
+++ b/util.h
@@ -172,6 +172,17 @@ extern int UTI_CreateDirAndParents(const
    permissions and its uid/gid must match the specified values. */
 extern int UTI_CheckDirPermissions(const char *path, mode_t perm, uid_t uid, gid_t gid);
 
+/* Open a file.  The full path of the file is constructed from the basedir
+   (may be NULL), '/' (if basedir is not NULL), name, and suffix (may be NULL).
+   Created files have specified permissions (umasked).  Returns NULL on error.
+   The following modes are supported (if the mode is an uppercase character,
+   errors are fatal):
+   r/R - open an existing file for reading
+   w/W - open a new file for writing (remove existing file)
+   a/A - open an existing file for appending (create if does not exist) */
+extern FILE *UTI_OpenFile(const char *basedir, const char *name, const char *suffix,
+                          char mode, mode_t perm);
+
 /* Set process user/group IDs and drop supplementary groups */
 extern void UTI_DropRoot(uid_t uid, gid_t gid);

Attachment: signature.asc
Description: PGP signature