threads / discuss / 58685

FORCE_DIR_SET_GID denied inside nix's build sandbox

Subject: FORCE_DIR_SET_GID denied inside nix's build sandbox

## tl;dr

3 messages between Oct 25, 2022 and Oct 25, 2022.

replies: 2people: 2as markdown or json

Julien Moutinho· Oct 25, 2022, 16:30 UTC · lore
Hi,

While running public-inbox's tests inside nix's build sandbox I've noticed that when core.sharedRepository is set to 0600 git still tries in adjust_shared_perm() to set the g+s bit on directories. https://github.com/git/git/blob/1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886/path.c#L901-L905

However, nix's build sandbox denies g+s (such chmod returning EPERM) because it could be leveraged to break the isolation of the build user. https://github.com/NixOS/nix/blob/b3d2a05c59266688aa904d5fb326394cbb7e9e90/src/libstore/sandbox-defaults.sb#L5-L7

So, on meta@public-inbox.org we were wondering whether git should maybe strip S_ISGID and retry to chmod() if it hits EPERM? https://public-inbox.org/meta/20221025101756.M341966@dcvr/T/#m3f4e9eba9b903a263221ab82ce7ddcd44248d033

Any thoughts on that matter? Thanks!

Junio C Hamano· Oct 25, 2022, 16:54 UTC · re: Julien Moutinho · lore

Re: FORCE_DIR_SET_GID denied inside nix's build sandbox

Julien Moutinho <julm+git@sourcephile.fr> writes:
Show 8 quoted lines
> While running public-inbox's tests inside nix's build sandbox
> I've noticed that when core.sharedRepository is set to 0600
> git still tries in adjust_shared_perm()
> to set the g+s bit on directories.
> https://github.com/git/git/blob/1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886/path.c#L901-L905
> ...
> So, on meta@public-inbox.org we were wondering whether
> git should maybe strip S_ISGID and retry to chmod() if it hits EPERM?

But that can leave the repository unusable when the files and directories has to be owned by the same group as its containing directory.

It does sound like we are bit too aggressive when setting the g+s bit.

Is g+s only necessary if the directory is group writable? Not necessarily. When I am the only writer (and owner) with memberships in multiple groups, a shared repository that I intend to give read (but not write) permission bit to members of a specific group must be owned by that group, and setting g+s is how we ensure it. So it is insufficient to give when new_mode says it is group writable. We also should do g+s if it is readable by the group.

But if you use 0600, then the group ownership should not matter, so
    /* Copy read bits to execute bits */
    new_mode |= (new_mode & 0444) >> 2;
    if (new_mode & 060)
	new_mode |= FORCE_DIR_SET_GID;
might be what you want?
Julien Moutinho· Oct 25, 2022, 22:40 UTC · re: Junio C Hamano · lore

Re: FORCE_DIR_SET_GID denied inside nix's build sandbox

Le mar. 25 oct. 2022 09h54 -0700, Junio C Hamano a écrit :
Show 8 quoted lines
> But if you use 0600, then the group ownership should not matter, so
> 
>     /* Copy read bits to execute bits */
>     new_mode |= (new_mode & 0444) >> 2;
>     if (new_mode & 060)
> 	new_mode |= FORCE_DIR_SET_GID;
> 
> might be what you want?

That change makes more sense indeed, and does relax the chmod enough to solve our problem when running public-inbox tests into nix's build sandbox.

Thank you!
PS: please let me know if you need me to submit a proper patch,
otherwise I'm leaving this up to you if you don't mind.

← back to recent threads