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

[Alexey Abramov]

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