This means we can systematically pass the environment variables
through bwrap(1), even if it is setuid and thus is filtering out
security-sensitive environment variables. bwrap itself ends up being
run with an empty environment instead.
This fixes a regression when CVE-2021-21261 was fixed: before the
CVE fixes, LD_LIBRARY_PATH would have been passed through like this
and appeared in the `flatpak build` shell, but during the CVE fixes,
the special case that protected LD_LIBRARY_PATH was removed in favour
of the more general flatpak_bwrap_envp_to_args(). That reasoning only
works if we use flatpak_bwrap_envp_to_args(), consistently, everywhere
that we run the potentially-setuid bwrap.
Fixes: 6d1773d2 "run: Convert all environment variables into bwrap arguments"
Resolves: https://github.com/flatpak/flatpak/issues/4080
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 9a61d2c44f)
For the portal's use of --env-fd= to be safe, we want the environment
variables that it sets to end up in the environment for the program
that is run by `bwrap` as process 2, but they must not go into the
environment that gets used to run `flatpak run` or `bwrap`. Assert
that this is the case.
For completeness, we're testing both --env= and --env-fd= here,
even though the earlier commit
"portal: Do not use caller-supplied variables in environment"
always uses --env-fd=.
Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2
Signed-off-by: Simon McVittie <smcv@collabora.com>
If the caller specifies a variable that can be used to inject arbitrary
code into processes, we must not allow it to enter the environment
block used to run `flatpak run`, which runs unsandboxed.
This change requires the previous commit "context: Add --env-fd option",
which adds infrastructure used here.
To be secure, this change also requires the previous commit
"run: Convert all environment variables into bwrap arguments", which
protects a non-setuid bwrap(1) from the same attack.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2
This avoids some of them being filtered out by a setuid bwrap. It also
means that if they came from an untrusted source, they cannot be used
to inject arbitrary code into a non-setuid bwrap via mechanisms like
LD_PRELOAD.
Because they get bundled into a memfd or temporary file, they do not
actually appear in argv, ensuring that they remain inaccessible to
processes running under a different uid (which is important if their
values are tokens or other secrets).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2
This is syntactic sugar added in GLib 2.67.0, which makes it more clearly
correct when we return TRUE after a GDBus error.
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit cc7f863707)
It is very much possible for an invalid ref name to occur, either due to
lack of validation on Flatpak creation like #3887, or just any
manually-written ref name due to skipping Flatpak tooling or malicious
intent. Regardless, this shouldn't crash, so check the names before
creating the transaction ops.
Fixes#3887.
When uninstalling a single app (an example of a transaction which is
entirely local-only), there’s no need to update the repository metadata
beforehand — but the code was doing that.
This avoids an unnecessary download of the `summary.sig` file (or other
repository metadata, depending on what’s out of date) when uninstalling.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Cache hits didn’t result in the cache entry’s timeout being extended,
and timeouts didn’t result in the entry being removed from the cache to
free up resources (even though it would never be returned as a cache hit
again).
Signed-off-by: Philip Withnall <withnall@endlessm.com>
(cherry picked from commit 63b4f3a5c1)
Also use a constant to clarify the code a little, although it’s
functionally equivalent.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
(cherry picked from commit 9bec865995)
After the --redirect-url= test is finished it leaves the remote with a
different url, which is confusing the other tests.
In particular, at some point we remove the remote and add it back,
which gets us back to the old url, but at that point the summary (from
the newer url) has a later mtime than the old one so the old one keeps
getting used.
So, we unset the redirect and set back the old url. Also, the following test
had to be tweaked for this change.
This adds a sleep(1) before each summary update (if there is a
pre-existing summary file). This avoids issues where a new summary
file get the same mtime (in seconds precision).
This is kind of a hacky work around, but it is good enought to get
the flatpak-1.8 branch working with latest ostree, and master has a better
fix already.
We enforce --no-update-summary when we create test apps and
runtimes, and then we ensure we always manually call update_repo
after all modifications are done.
This means we save work avoiding summary updates, but it also means we
can do special handling in update_summary and guarantee that this is
the only place this happens. For example, we want this to work around
the mtime handling of summary updates.
In case a transaction is uninstalling both an app and its runtime,
properly ensure the app is uninstalled first.
(cherry picked from commit c6647c2b14)
Every http operation (even when successfull) was spewing info about
whether to retry it which made it hard to read the logs.
(cherry picked from commit 3ebcd200ca)
Sometimes a server might return a HTTP error 500 (this seems to happen
sometimes with Microsoft’s VSCode server, for example). Map this to
`G_IO_ERROR_HOST_UNREACHABLE` for now, which is a bit more specific than
returning `G_IO_ERROR_FAILED`, but without the hassle of introducing a
new public error domain which could give more detail.
In particular, this should allow gnome-software to show an error message
to the user for such failed downloads, rather than hiding the error and
logging the following:
```
not handling error failed for action download: While downloading http://packages.microsoft.com/repos/vscode/pool/main/c/code/code_1.45.1-1589445302_amd64.deb: Server returned status 500: Internal Server Error
```
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry picked from commit 6c79a57114)
We were clearing the error from the anon test, and then not doing any
non-anon auth, so error was NULL, causing a crash when returning an
error message.
(cherry picked from commit 180d807d2a)
Fix lookup_installation_for_path() to not leave the GError pointer unset
on its error code path. This error is only used by the caller for a
debug message, and shouldn't be hit normally, but it could mean a NULL
pointer dereference when we try to print error->message.
(cherry picked from commit 075c86ca4f)
In case we need to authenticate for updates (in my test case i was
doing an OCI downgrade) we might need to download a commit object (or
in the OCI case a manifest json), so it did a request_required_tokens(),
but that noticed during the flatpak_transaction_normalize_ops() call
that the partial resolve to a particular commit actually was the
same as the local installed commit and marked op->skip = TRUE.
However, when we got back to resolving the op again we didn't actually
look at the skip, so it kept looping wanting (but never doing) auth.
The fix is to just directly resolve ops marked as skipped.
(cherry picked from commit d8086141fb)
Runtimes also have appstream data - with description, license information,
and so forth, so we should extract the appstream data from the index
for refs that start with runtime/ as well.
(cherry picked from commit 25ff00994f)
Without this patch, the remote-info command will sometimes emit a
critical error "g_utf8_strlen: assertion 'p != NULL || max == 0' failed"
and print (null) for the "Commit:" field, since the commit doesn't get
initialized properly.
(cherry picked from commit 238317d99b)
This commit is a follow-up for the commit "dir: Validate locally
configured collection IDs". Whereas in that commit we validate the
collection ID in flatpak_remote_state_fetch_commit_object(), here we do
it in _flatpak_dir_get_remote_state(), since fetch_commit_object() is
not called on the code path normally taken for a transaction (only if
try_resolve_op_from_metadata() fails and in other niche scenarios).
This should ensure that if someone erroneously sets a collection ID on a
remote, the error will be caught quickly. It also helps the eos-updater
unit tests pass.
(cherry picked from commit 332f75494b)
If the initial anonymous fails for any other reason than "not authorized"
we immediately fail the operation instead of asking for user/password.
The later is creating a very bad UX in case of e.g. networking or
infrastructure issues, as described in #3753.
(cherry picked from commit 09d57249f4)
These are system dbus calls that could potentially be somewhat slow
and its unlikely that the local config changes during runtime of the
session (because the desktop really needs a restart to pick up a new
locale). Also, if there are any issues with these (such as #3755) each
call will be the 25sec dbus timeout, which is not great.
Fixes https://github.com/flatpak/flatpak/issues/3792
