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

[Alexey Abramov via Guix-patches via <guix-patches@gnu.org>]

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