| Message ID | 20220420084913.4113-1-levenson@mmer.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Let openssh trust /gnu/store | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Alexey Abramov <levenson@mmer.org> skribis: > + safe_path(const char *name, struct stat *stp, const char *pw_dir, > + uid_t uid, char *err, size_t errlen) > + { > ++ static const char gnu_store[] = "/gnu/store"; > + char buf[PATH_MAX], homedir[PATH_MAX]; > + char *cp; > + int comparehome = 0; > +@@ -2178,6 +2179,10 @@ safe_path(const char *name, struct stat *stp, const char *pw_dir, > + } > + strlcpy(buf, cp, sizeof(buf)); > + > ++ /* If are past the Guix /gnu/store then we can stop */ > ++ if (strcmp(gnu_store, buf) == 0) > ++ break; We should not hard-code “/gnu/store” because it can be something else. I think you can do like what ‘gcc-dl-cache.patch’ does: replace the literal "/gnu/store" by @STORE_DIRECTORY@, and substitute it in a phase. Also note that the strcmp above is incorrect: it would accept /gnu/storesomethinglese. You probably need to add a trailing slash to be sure. Thanks, Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Alexey Abramov <levenson@mmer.org> skribis: > >> + safe_path(const char *name, struct stat *stp, const char *pw_dir, >> + uid_t uid, char *err, size_t errlen) >> + { >> ++ static const char gnu_store[] = "/gnu/store"; >> + char buf[PATH_MAX], homedir[PATH_MAX]; >> + char *cp; >> + int comparehome = 0; >> +@@ -2178,6 +2179,10 @@ safe_path(const char *name, struct stat *stp, const char *pw_dir, >> + } >> + strlcpy(buf, cp, sizeof(buf)); >> + >> ++ /* If are past the Guix /gnu/store then we can stop */ >> ++ if (strcmp(gnu_store, buf) == 0) >> ++ break; > > We should not hard-code “/gnu/store” because it can be something else. > > I think you can do like what ‘gcc-dl-cache.patch’ does: replace the > literal "/gnu/store" by @STORE_DIRECTORY@, and substitute it in a phase. This is great! That is what I was looking for. > Also note that the strcmp above is incorrect: it would accept > /gnu/storesomethinglese. You probably need to add a trailing slash to > be sure. Let me correct myself. In the previous email I wrote that the safe_path goes from top to bottom, but actually it walking upwards. This is an actual loop --8<---------------cut here---------------start------------->8--- /* for each component of the canonical path, walking upwards */ for (;;) { if ((cp = dirname(buf)) == NULL) { snprintf(err, errlen, "dirname() failed"); return -1; } strlcpy(buf, cp, sizeof(buf)); /* If are past the Guix /gnu/store then we can stop */ if (strcmp(gnu_store, buf) == 0) break; if (stat(buf, &st) == -1 || (!platform_sys_dir_uid(st.st_uid) && st.st_uid != uid) || (st.st_mode & 022) != 0) { snprintf(err, errlen, "bad ownership or modes for directory %s", buf); return -1; } /* If are past the homedir then we can stop */ if (comparehome && strcmp(homedir, buf) == 0) break; /* * dirname should always complete with a "/" path, * but we can be paranoid and check for "." too */ if ((strcmp("/", buf) == 0) || (strcmp(".", buf) == 0)) break; } return 0; --8<---------------cut here---------------end--------------->8--- As you can see, buffer is holding the result of dirname already, hence I used "/gnu/store".
diff --git a/gnu/local.mk b/gnu/local.mk index 0e721236d9..449a990846 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -1569,6 +1569,7 @@ dist_patch_DATA = \ %D%/packages/patches/openjdk-15-xcursor-no-dynamic.patch \ %D%/packages/patches/openmpi-mtl-priorities.patch \ %D%/packages/patches/openssh-hurd.patch \ + %D%/packages/patches/openssh-trust-gnu-store-directory.patch \ %D%/packages/patches/openresolv-restartcmd-guix.patch \ %D%/packages/patches/openrgb-unbundle-hueplusplus.patch \ %D%/packages/patches/opensles-add-license-file.patch \ diff --git a/gnu/packages/patches/openssh-trust-gnu-store-directory.patch b/gnu/packages/patches/openssh-trust-gnu-store-directory.patch new file mode 100644 index 0000000000..b50dc8fd6a --- /dev/null +++ b/gnu/packages/patches/openssh-trust-gnu-store-directory.patch @@ -0,0 +1,35 @@ +From a840e4b10961fb2b1b6b0f93e5b2b367887ed691 Mon Sep 17 00:00:00 2001 +From: Alexey Abramov <levenson@mmer.org> +Date: Sun, 21 Nov 2021 12:21:28 +0100 +Subject: [PATCH] Trust /gnu/store directory + +--- + misc.c | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/misc.c b/misc.c +index 0134d69..01f1660 100644 +--- a/misc.c ++++ b/misc.c +@@ -2146,6 +2146,7 @@ int + safe_path(const char *name, struct stat *stp, const char *pw_dir, + uid_t uid, char *err, size_t errlen) + { ++ static const char gnu_store[] = "/gnu/store"; + char buf[PATH_MAX], homedir[PATH_MAX]; + char *cp; + int comparehome = 0; +@@ -2178,6 +2179,10 @@ safe_path(const char *name, struct stat *stp, const char *pw_dir, + } + strlcpy(buf, cp, sizeof(buf)); + ++ /* If are past the Guix /gnu/store then we can stop */ ++ if (strcmp(gnu_store, buf) == 0) ++ break; ++ + if (stat(buf, &st) == -1 || + (!platform_sys_dir_uid(st.st_uid) && st.st_uid != uid) || + (st.st_mode & 022) != 0) { +-- +2.33.1 + diff --git a/gnu/packages/ssh.scm b/gnu/packages/ssh.scm index 8a61b6e97a..8dd7f1727a 100644 --- a/gnu/packages/ssh.scm +++ b/gnu/packages/ssh.scm @@ -189,7 +189,8 @@ (define-public openssh (method url-fetch) (uri (string-append "mirror://openbsd/OpenSSH/portable/" "openssh-" version ".tar.gz")) - (patches (search-patches "openssh-hurd.patch")) + (patches (search-patches "openssh-hurd.patch" + "openssh-trust-gnu-store-directory.patch")) (sha256 (base32 "1ry5prcax0134v6srkgznpl9ch5snkgq7yvjqvd8c5mbnxa7cjgx"))))