[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Alexey Abramov via Guix-patches via]

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

[bug#55034] [PATCH 1/1] gnu: openssh: Trust /gnu/store directory

[Alexey Abramov via Guix-patches via]

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

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Ludovic Courtès]

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

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Alexey Abramov via Guix-patches via]

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


-- 
Alexey

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Ludovic Courtès]

Hi,

Alexey Abramov <levenson@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’.

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Alexey Abramov via Guix-patches via]

Hi Ludo,

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

> Hi,
>
> Alexey Abramov <levenson@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

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Ludovic Courtès]

Hi,

Alexey Abramov <levenson@mmer.org> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi,
>>
>> Alexey Abramov <levenson@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’.

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Tobias Geerinckx-Rice via Guix-patches via]

On 20 April 2022 08:47:24 UTC, Alexey Abramov via Guix-patches via <guix-patches@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.

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Tobias Geerinckx-Rice via Guix-patches via]

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

[bug#55034] [PATCH 0/1] Let openssh trust /gnu/store

[Alexey Abramov via Guix-patches via]

Hi Tobias,

Thanks for the review.

Tobias Geerinckx-Rice <me@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

[bug#55034] [PATCH v2] gnu: openssh: Trust Guix store directory

[Alexey Abramov via Guix-patches via]

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

bug#55034: [PATCH 0/1] Let openssh trust /gnu/store

[Ludovic Courtès]

Hi,

Alexey Abramov <levenson@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’.