GNU bug report logs - #55034
[PATCH 0/1] Let openssh trust /gnu/store

Previous Next

Package: guix-patches;

Reported by: Alexey Abramov <levenson <at> mmer.org>

Date: Wed, 20 Apr 2022 08:48:02 UTC

Severity: normal

Tags: patch

Done: Ludovic Courtès <ludo <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55034 in the body.
You can then email your comments to 55034 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 08:48:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Alexey Abramov <levenson <at> mmer.org>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Wed, 20 Apr 2022 08:48:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: guix-patches <at> gnu.org
Subject: [PATCH 0/1] Let openssh trust /gnu/store 
Date: Wed, 20 Apr 2022 10:47:24 +0200
This patch allows users to use /gnu/store objects for AuthorizedKeysCommand
and similar options. According to the sshd_config(5):

> The program must be owned by root, not writable by group or others, and
> specified by an absolute path.

However, this is not the case for Guix, even though it is RO. OpenSSH doesn't
check if the location mounted or ended up on the RO mount point.

I think implementing a check for RO location is much harder here, rather
than to trust /gnu/store path. The same way OpenSSH does with users' home
directory.

Let me know what you think.

Alexey Abramov (1):
  gnu: openssh: Trust /gnu/store directory

 gnu/local.mk                                  |  1 +
 .../openssh-trust-gnu-store-directory.patch   | 35 +++++++++++++++++++
 gnu/packages/ssh.scm                          |  3 +-
 3 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/openssh-trust-gnu-store-directory.patch

-- 
2.34.0





Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 08:50:01 GMT) Full text and rfc822 format available.

Message #8 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: 55034 <at> debbugs.gnu.org.
Subject: [PATCH 1/1] gnu: openssh: Trust /gnu/store directory
Date: Wed, 20 Apr 2022 10:49:13 +0200
* gnu/local.mk (dist_patch_DATA): Add the patch
* gnu/packages/patches/openssh-trust-gnu-store-directory.patch: Patch it
* gnu/packages/ssh.scm (openssh[source]): Use it.
---
 gnu/local.mk                                  |  1 +
 .../openssh-trust-gnu-store-directory.patch   | 35 +++++++++++++++++++
 gnu/packages/ssh.scm                          |  3 +-
 3 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/openssh-trust-gnu-store-directory.patch

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 <at> 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"))))
-- 
2.34.0





Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 09:57:01 GMT) Full text and rfc822 format available.

Message #11 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Alexey Abramov <levenson <at> mmer.org>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store 
Date: Wed, 20 Apr 2022 11:56:49 +0200
Hi,

Alexey Abramov <levenson <at> mmer.org> skribis:

> This patch allows users to use /gnu/store objects for AuthorizedKeysCommand
> and similar options. According to the sshd_config(5):
>
>> The program must be owned by root, not writable by group or others, and
>> specified by an absolute path.

That’s the case with programs in /gnu/store.  Why isn’t it working?

> However, this is not the case for Guix, even though it is RO. OpenSSH doesn't
> check if the location mounted or ended up on the RO mount point.
>
> I think implementing a check for RO location is much harder here, rather
> than to trust /gnu/store path. The same way OpenSSH does with users' home
> directory.

(RO = read-only, right?)

I’m not sure why checking whether a file is read-only is much harder.
Am I overlooking something?

Thanks,
Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 10:03:01 GMT) Full text and rfc822 format available.

Message #14 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Alexey Abramov <levenson <at> mmer.org>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store 
Date: Wed, 20 Apr 2022 12:02:12 +0200
Alexey Abramov <levenson <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 10:18:02 GMT) Full text and rfc822 format available.

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Alexey Abramov <levenson <at> mmer.org>,
 Alexey Abramov via Guix-patches via <guix-patches <at> gnu.org>,
 55034 <at> debbugs.gnu.org
Subject: Re: [bug#55034] [PATCH 0/1] Let openssh trust /gnu/store
Date: Wed, 20 Apr 2022 10:17:16 +0000
On 20 April 2022 08:47:24 UTC, Alexey Abramov via Guix-patches via <guix-patches <at> gnu.org> wrote:
>This patch allows users to use /gnu/store objects for AuthorizedKeysCommand
>and similar options. According to the sshd_config(5):
>
>> The program must be owned by root, not writable by group or others, and
>> specified by an absolute path.
>
>However, this is not the case for Guix, even though it is RO. OpenSSH doesn't
>check if the location mounted or ended up on the RO mount point.

The RO bind mount is not a hard guarantee, and a footgun protector against accidental writes, not primarily a security feature (IMO).

By design, *anyone* can write *anything* to the store by talking to the daemon.  They just can't choose the file name.  A much weaker guarantee than OpenSSH assumes, at the very least.

With that in mind, could this highly intrusive patch be used to compromise a system?  It seems so very likely.  If it is, Guix will be rightly derided for what amounts to ifdeffing out the securities, even if OpenBSD's can be frustratingly theatrical at times.

>I think implementing a check for RO location is much harder here

Why is 'RO location' relevant here?

If the snippet you quote above is complete, which requirement does the un-bind-mounted store not meet?  I can't think of one off the top o' me head?

> , rather
>than to trust /gnu/store path.

That's a lot of trust.  Tens of gigabytes on average.

We explicitly rejected that idea in IceCat for example, instead whitelisting only specific store subdirectories.  Why is OpenSSH different?

> The same way OpenSSH does with users' home
>directory.
>
>Let me know what you think.

The rationale and its assumptions (also) belong in the patch itself, not just a separate mail.

Hi Alexey,

Thanks for the patch suggestion!

Kind regards,

T G-R

Sent on the go.  Excuse or enjoy my brevity.




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 10:18:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 10:21:02 GMT) Full text and rfc822 format available.

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Alexey Abramov <levenson <at> mmer.org>,
 Alexey Abramov via Guix-patches via <guix-patches <at> gnu.org>,
 55034 <at> debbugs.gnu.org
Subject: Re: [bug#55034] [PATCH 0/1] Let openssh trust /gnu/store
Date: Wed, 20 Apr 2022 10:20:02 +0000
> $EVERYTHING
>
>Hi Alexey,
>
>Thanks for the patch suggestion!
>
>Kind regards,
>
>T G-R

is a very annoying thing my MUA does.  Apologies.


Kind regards,

T G-R

Sent on the go.  Excuse or enjoy my brevity.




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 20 Apr 2022 10:21:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Fri, 22 Apr 2022 06:46:01 GMT) Full text and rfc822 format available.

Message #29 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store
Date: Fri, 22 Apr 2022 08:44:56 +0200
Hi Ludo,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Alexey Abramov <levenson <at> mmer.org> skribis:
>
>> This patch allows users to use /gnu/store objects for AuthorizedKeysCommand
>> and similar options. According to the sshd_config(5):
>>
>>> The program must be owned by root, not writable by group or others, and
>>> specified by an absolute path.
>
> That’s the case with programs in /gnu/store.  Why isn’t it working?

The reason is that safe_path in openssh takes a full path of the file
and checks every directory one by one. The constrain fails on /gnu/store
directory due to write permissions for group.

openssh reports the following message:

Unsafe AuthorizedKeysCommand "/gnu/store/xxxx-echo-sshkey.sh": bad
ownership or modes for directory /gnu/store.

>> However, this is not the case for Guix, even though it is RO. OpenSSH doesn't
>> check if the location mounted or ended up on the RO mount point.
>>
>> I think implementing a check for RO location is much harder here, rather
>> than to trust /gnu/store path. The same way OpenSSH does with users' home
>> directory.
>
> (RO = read-only, right?)

Yes. 

> I’m not sure why checking whether a file is read-only is much harder.
> Am I overlooking something?

As I mentioned before, the check not just checking the file path itself,
but also follows down to the root and check every single directory for
the constrain. Me dunno, was thinking about an extra check against mount
locations, and in case it has read-only mount options along the way, I
could trust the executable. It also implies cross-compilation...

May be I overthink the thing? Maybe it is me who overlooking something?

-- 
Alexey




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Fri, 22 Apr 2022 07:04:01 GMT) Full text and rfc822 format available.

Message #32 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store
Date: Fri, 22 Apr 2022 09:02:49 +0200
Ludovic Courtès <ludo <at> gnu.org> writes:

> Alexey Abramov <levenson <at> 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".


-- 
Alexey




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Fri, 22 Apr 2022 07:34:02 GMT) Full text and rfc822 format available.

Message #35 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: [bug#55034] [PATCH 0/1] Let openssh trust /gnu/store
Date: Fri, 22 Apr 2022 09:33:45 +0200
Hi Tobias,

Thanks for the review.

Tobias Geerinckx-Rice <me <at> tobias.gr> writes:

[...]

> The RO bind mount is not a hard guarantee, and a footgun protector
> against accidental writes, not primarily a security feature (IMO).
>
> By design, *anyone* can write *anything* to the store by talking to
> the daemon.  They just can't choose the file name.  A much weaker
> guarantee than OpenSSH assumes, at the very least.

Even though I knew how the daemon works, I find your explanation very
nice and clear. Thank you.


[...]

>
> Why is 'RO location' relevant here?
>
> If the snippet you quote above is complete, which requirement does the
> un-bind-mounted store not meet?  I can't think of one off the top o'
> me head?

Here is a comment from safe_path function

--8<---------------cut here---------------start------------->8---
/*
 * Check a given path for security. This is defined as all components of
 * the path to the file must be owned by either the owner of of the file
 * or root and no directories must be group or world writable.
 *
 * XXX Should any specific check be done for sym links ?
 *
 * Takes a file name, its stat information (preferably from fstat() to
 * avoid races), the uid of the expected owner, their home directory and
 * an error buffer plus max size as arguments.
 *
 * Returns 0 on success and -1 on failure /
--8<---------------cut here---------------end--------------->8---

I probably had to post it first, to avoid
misunderstanding. sshd_config(5) is not that clear unfortunately. Due to
group write permissions on the /gnu/store directory, safe_path doesn't
allow openssh execute it. 

Couple of months ago I posted this problem on IRC, and you mentioned the
read-only mount thingy. So I was trying to take advantage of that.

What other options do I have?

> That's a lot of trust.  Tens of gigabytes on average.

=)

> We explicitly rejected that idea in IceCat for example, instead
> whitelisting only specific store subdirectories.  Why is OpenSSH
> different?

I didn't know that. I don't treat OpenSSH any different than other
software either. Whitelist some specific directory is a really good
option here, even though It introduces some secret knowledge.

> The rationale and its assumptions (also) belong in the patch itself,
> not just a separate mail.

True. Let me put some more context on what I am trying to do. We have
LDAP server which also holds users' ssh keys. I package a simple wrapper
for LDAP search which returns them. I would like to use it with OpenSSH,
however due to the way it checks executable in the configuration, I
don't see the way to use it.

I assume it is possible to copy that store object somewhere, but it
doesn't look right to me.

-- 
Alexey




Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Tue, 26 Apr 2022 07:27:02 GMT) Full text and rfc822 format available.

Message #38 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Alexey Abramov <levenson <at> mmer.org>
To: 55034 <at> debbugs.gnu.org.
Subject: [PATCH v2] gnu: openssh: Trust Guix store directory
Date: Tue, 26 Apr 2022 09:25:50 +0200
* gnu/local.mk (dist_patch_DATA): Add the patch
* gnu/packages/patches/openssh-trust-guix-store-directory.patch: Patch it
* gnu/packages/ssh.scm (openssh[source]): Use it.
---
 gnu/local.mk                                  |  1 +
 .../openssh-trust-guix-store-directory.patch  | 40 +++++++++++++++++++
 gnu/packages/ssh.scm                          |  8 +++-
 3 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/openssh-trust-guix-store-directory.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 9bad87710c..1d8e39138e 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1567,6 +1567,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-guix-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-guix-store-directory.patch b/gnu/packages/patches/openssh-trust-guix-store-directory.patch
new file mode 100644
index 0000000000..b3a9c1bdfc
--- /dev/null
+++ b/gnu/packages/patches/openssh-trust-guix-store-directory.patch
@@ -0,0 +1,40 @@
+From 0d85bbd42ddcd442864a9ba4719aca8b70d68048 Mon Sep 17 00:00:00 2001
+From: Alexey Abramov <levenson <at> mmer.org>
+Date: Fri, 22 Apr 2022 11:32:15 +0200
+Subject: [PATCH] Trust guix store directory
+
+To be able to execute binaries defined in OpenSSH configuration, we
+need to tell OpenSSH that we can trust Guix store objects. safe_path
+procedure takes a canonical path and for each component, walking
+upwards, checks ownership and permissions constrains which are: must
+be owned by root, not writable by group or others.
+---
+ misc.c | 5 +++++
+ 1 file changed, 5 insertions(+)
+
+diff --git a/misc.c b/misc.c
+index 0134d69..7131d5e 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 guix_store[] = @STORE_DIRECTORY@;
+ 	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 we are past the Guix store then we can stop */
++		if (strcmp(guix_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.34.0
+
diff --git a/gnu/packages/ssh.scm b/gnu/packages/ssh.scm
index 8a61b6e97a..7f3b02013e 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-guix-store-directory.patch"))
              (sha256
               (base32
                "1ry5prcax0134v6srkgznpl9ch5snkgq7yvjqvd8c5mbnxa7cjgx"))))
@@ -249,6 +250,11 @@ (define-public openssh
              (substitute* "Makefile"
                (("PRIVSEP_PATH=/var/empty")
                 (string-append "PRIVSEP_PATH=" out "/var/empty"))))))
+        (add-after 'configure 'set-store-location
+          (lambda* _
+            (substitute* "misc.c"
+              (("@STORE_DIRECTORY@")
+               (string-append "\"" (%store-directory) "\"")))))
         (add-before 'check 'patch-tests
          (lambda _
            (substitute* "regress/test-exec.sh"
-- 
2.34.0





Information forwarded to guix-patches <at> gnu.org:
bug#55034; Package guix-patches. (Wed, 27 Apr 2022 21:55:01 GMT) Full text and rfc822 format available.

Message #41 received at 55034 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Alexey Abramov <levenson <at> mmer.org>
Cc: 55034 <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store
Date: Wed, 27 Apr 2022 23:54:09 +0200
Hi,

Alexey Abramov <levenson <at> mmer.org> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Hi,
>>
>> Alexey Abramov <levenson <at> mmer.org> skribis:
>>
>>> This patch allows users to use /gnu/store objects for AuthorizedKeysCommand
>>> and similar options. According to the sshd_config(5):
>>>
>>>> The program must be owned by root, not writable by group or others, and
>>>> specified by an absolute path.
>>
>> That’s the case with programs in /gnu/store.  Why isn’t it working?
>
> The reason is that safe_path in openssh takes a full path of the file
> and checks every directory one by one. The constrain fails on /gnu/store
> directory due to write permissions for group.

Oh I see, makes sense.

[...]

>> 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

[...]

> As you can see, buffer is holding the result of dirname already, hence
> I used "/gnu/store".

Sounds good then!

Thanks for explaining,
Ludo’.




Reply sent to Ludovic Courtès <ludo <at> gnu.org>:
You have taken responsibility. (Thu, 28 Apr 2022 22:08:01 GMT) Full text and rfc822 format available.

Notification sent to Alexey Abramov <levenson <at> mmer.org>:
bug acknowledged by developer. (Thu, 28 Apr 2022 22:08:01 GMT) Full text and rfc822 format available.

Message #46 received at 55034-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Alexey Abramov <levenson <at> mmer.org>
Cc: 55034-done <at> debbugs.gnu.org
Subject: Re: bug#55034: [PATCH 0/1] Let openssh trust /gnu/store 
Date: Fri, 29 Apr 2022 00:07:12 +0200
Hi,

Alexey Abramov <levenson <at> mmer.org> skribis:

> * gnu/local.mk (dist_patch_DATA): Add the patch
> * gnu/packages/patches/openssh-trust-guix-store-directory.patch: Patch it
> * gnu/packages/ssh.scm (openssh[source]): Use it.

Applied, thanks!

Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 27 May 2022 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 306 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.