flatpak / flatpak-builder

Tool to build flatpaks from source
GNU Lesser General Public License v2.1
141 stars 93 forks source link

/sbin/chown: Invalid argument when building sudo #613

Closed stsp closed 4 months ago

stsp commented 4 months ago

Checklist

flatpak-builder version

1.4.1

Flatpak version

1.15.8

How to reproduce

  - name: sudo
    buildsystem: autotools
    sources:
      - type: git
        url: https://github.com/sudo-project/sudo.git
        branch: main

Expected Behavior

Build and installs.

Actual Behavior

Multiple errors like /sbin/chown: changing ownership of '/app/libexec/sudo/libsudo_util.so.0.0.0': Invalid argument when installing.

Additional Information

sudo explicitly specifies group and ownership to install when installing. That leads to explicit chmods, even though the files are root-owned anyway. I am not sure why that fails with "Invalid argument". I suppose flatpak uses some FS that doesn't support chown?

TingPing commented 4 months ago

I assume it is just a permission issue.

sudo is completely useless inside of flatpak anyway.

stsp commented 4 months ago

There are scripts that call it. But maybe I can adjust them. How can the script check that he is running inside flatpak?

TingPing commented 4 months ago

If /.flatpak-info exists.

Depends on why the script needs sudo, it may just fundamentally be broken.

stsp commented 4 months ago

It needs sudo to create tap device, and a few more things. But I think this all is not relevant under docker.

smcv commented 4 months ago

I am not sure why that fails with "Invalid argument". I suppose flatpak uses some FS that doesn't support chown?

Not exactly. Inside the Flatpak sandbox, there is only one uid available: your own uid. All other uids get mapped to the kernel's overflow uid, usually nobody/65534. You can think of those two as "me" and "not me". This is a kernel restriction: the kernel does not allow Flatpak to create a sandbox environment with more uids than your own.

Any other uid simply does not exist inside the sandbox environment, so it is literally true that it's an invalid argument.

But I think this all is not relevant under docker.

That's correct, but Flatpak is not Docker, so what would work in Docker is not relevant here. Docker is highly privileged and does not have to obey the same restrictions that Flatpak does.

sudo explicitly specifies group and ownership to install when installing. That leads to explicit chmods, even though the files are root-owned anyway.

This cannot work, and no, the files are not root-owned: they are owned by the user that is running make install or equivalent, which in our case is not root (there is no such thing as root in the sandbox).

sudo is completely useless inside of flatpak anyway.

Yes. The sandbox is running in a mode where setuid programs like sudo behave as though they were not setuid, so it is not possible to use tools like sudo to elevate privileges. Even if it was possible, it would not be possible to elevate privileges to root, because the root user doesn't exist inside the sandbox. You cannot usefully build sudo into a Flatpak app.

There are scripts that call [sudo]. But maybe I can adjust them.

Yes, you will need to do that, if possible.

It needs sudo to create tap device, and a few more things

It is probably not possible to do this inside a Flatpak sandbox. Either you will need to find a way to avoid needing to do that, or your app is unsuitable for being packaged as a Flatpak app.

If your app is something like a VPN client that does system-wide things with TAP devices, then that's out of scope for what Flatpak can support: you will probably need to package it as non-sandboxed, at system-level, instead. Flatpak is not designed for this purpose.

stsp commented 4 months ago

Thanks for a detailed explanation. I have the following questions:

kernel's overflow uid, usually nobody/65534. You can think of those two as "me" and "not me".

Can I chown the binary to that nobody then?

not possible to use tools like sudo to elevate privileges. Even if it was possible, it would not be possible to elevate privileges to root, because the root user doesn't exist inside the sandbox.

Which likely means the chown to nobody is not possible either?

If your app is something like a VPN client that does system-wide things with TAP devices,

TAP devices can be omitted, no problems. libslirp is the solution to that. The problem comes when I can't even chown the binaries during sudo make instasll.

smcv commented 4 months ago

Can I chown the binary to that nobody then?

I don't think so. Try it and find out? But even if you can, it would not be useful to do so, because it will not give you any additional privileges.

The problem comes when I can't even chown the binaries during sudo make instasll

It is neither necessary nor possible to change the ownership of the binaries.

flatpak-builder doesn't do a sudo make install, just make install, into a directory that is writeable by the current user.

stsp commented 4 months ago

I don't think so. Try it and find out?

I can't. :) Any chown needs sudo.

But even if you can, it would not be useful to do so, because it will not give you any additional privileges.

The point is to drop privs, not get any additional. The technique is this:

sudo chown unpriv_user:unpriv_group binary_file
sudo chmod ug+s binary_file

After that, binary_file gets the ability to switch between the user who started it, and an unpriv_user. unpriv_user can't touch any files of the original user, so the program forks an RPC server, which later passes the FDs to an unpriv process. I am not sure I can replicate that under flatpak. :( So flatpak fights for the security, but at the same time there is no way (known to me, at least) how to drop privs under flatpak.

flatpak-builder doesn't do a sudo make install, just make install, into a directory that is writeable by the current user.

Yes but all the chown/chmod magic happens under sudo make install.

stsp commented 4 months ago

Any chown needs sudo.

Unless under flatpak the user already have some caps?

TingPing commented 4 months ago

@stsp This will require extra work but you can run a subprocess with fewer privileges by using flatpak-spawn at runtime.

stsp commented 4 months ago

Ok, so it can create the copy of the container but with the tighter permissions, and has --forward-fd=FD to pass an RPC fd... This almost looks like it could work in theory, but in practice this means the spawned process has no access to the state of the original one, and can't access anything that the original process could access during the initialization, including the config file (otherwise there is the risk it will maliciously change that config after being hacked). So all the initialization will have to be repeated, but this time solely based on an RPC fd...

This will require extra work

That is to say the least. :) The classic model "init-->fork-->drop_priv" should be replaced with "start unpriv'd and try to init yourself using only an RPC calls". This is a HUGE extra work, and I am not sure it has some benefits, i.e. it doesn't make the security better IMHO. It just allows to work with flatpak... :(

But thanks for providing all the valuable info!

TingPing commented 4 months ago

but in practice this means the spawned process has no access to the state of the original one, and can't access anything that the original process could access during the initialization, including the config file (otherwise there is the risk it will maliciously change that config after being hacked).

Sure some state would have to be serialized. For files you can use --sandbox-expose-ro=NAME.

The classic model "init-->fork-->drop_priv" should be replaced with "start unpriv'd and try to init yourself using only an RPC calls". This is a HUGE extra work, and I am not sure it has some benefits, i.e. it doesn't make the security better IMHO.

Well, it depends, but its potentially better security than you had previously. It is in a new namespace with a bit more control.

stsp commented 4 months ago

For files you can use --sandbox-expose-ro=NAME.

Hmm, at least for whatever is needed during the initialization (configs and whatever else) this may work. And if the starter process, after parsing all configs, carefully grants the R/O startup files and R/W-allowed files to the flatpak-spawn'ed process, then the flatpak-spawned process doesn't even need any changes at all. (wow) So if the changes can be limited to just the starter process, then this may probably be worth a try... maybe.

It is in a new namespace with a bit more control.

I can't even imagine the amount of magic happening behind the scene. You use portals to fork the container somehow - is this even fast enough? And you have a full chroot environment, as otherwise this can't work. You have user namespaces, likely also mount namespaces... All that just to replace the plain old and small RPC server approach familiar to everyone? :) Oh well... I am surprised this monstrosity can even work, but since you offer a seemingly realistic way to check that on my own... maybe I will. :)

stsp commented 4 months ago

Sure some state would have to be serialized.

That should be the command-line options perhaps, as the rest can be replicated from the configs. I guess there is a clever way to pass the cmdline options via flatpak-spawn so that not to serialize even these by hands?

stsp commented 4 months ago

I guess there is a clever way to pass the cmdline options via flatpak-spawn so that not to serialize even these by hands?

Yeah, its obviously there. So cmdline can be passed directly, configs can be granted R/O and re-parsed... and then I don't know what remains for an extra serialization.

TingPing commented 4 months ago

I can't even imagine the amount of magic happening behind the scene. You use portals to fork the container somehow - is this even fast enough?

What type of performance do you need? It takes 100ms on my machine. Probably don't put it in a tight loop running a million times.

TingPing commented 4 months ago

All that just to replace the plain old and small RPC server approach familiar to everyone? :)

Anyway this wasn't done for fun. Changing users, making a chroot, etc is simply a sandbox escape. It can't be done.

stsp commented 4 months ago

:) That performance is OK.

Changing users, making a chroot, etc is simply a sandbox escape.

Why changing users inside a container is a sandbox escape? Maybe you can provide an URL to some rtfm for me, as I don't suppose this can be briefly explained.

stsp commented 4 months ago

Sure some state would have to be serialized. For files you can use --sandbox-expose-ro=NAME.

--sandbox-expose-ro=NAME

    Expose readonly access to a file in the sandbox.

    Note that absolute paths or subdirectories are not allowed. 

Hmm, why is that? I need to export files and subdirectories. By an absolute paths. Why all of that disallowed? AFAICS this all stays within the initial sandbox anyway, nothing is additionally mounted. So why not allowing subdirs by absolute paths?

stsp commented 4 months ago

Ping! So I have all the code ready to try your suggestions, but I can't do that unless I pass directories. What to do? Why directories not allowed? Or should I just ignore that and try anyway?

TingPing commented 4 months ago

What to do? Why directories not allowed?

https://docs.flatpak.org/en/latest/portal-api-reference.html#gdbus-method-org-freedesktop-portal-Flatpak.Spawn

Either the file is in a specific subdirectory, or it is passed as an fd.

I'm not certain why subdirectories are forbidden for the former, but they wouldn't work for the latter.

stsp commented 4 months ago

I'm not certain why subdirectories are forbidden for the former,

Any hopes to change that?

TingPing commented 4 months ago

I'll say nobody is working on it unless somebody, such as yourself, looks into it.

I think it should be fine, but have not looked at it deeply.

stsp commented 4 months ago

https://github.com/flatpak/flatpak/blob/main/portal/flatpak-portal.c#L565 It seems they inhibit any slash. I probably don't understand the usage then. So it wants a plain filename? But in what dir?

stsp commented 4 months ago

Maybe this means they allow any file and dir from the /app, but not any subdir or a file with non-toplevel path?