Now that our error messages are valid UTF-8, we should no longer need to
do this.
This reverts commit 242e9893e7.
Signed-off-by: Simon McVittie <smcv@collabora.com>
While I'm changing the translatable string anyway, this also
incorporates a suggestion from #6753 to quote the invalid character, so
that output is clearer in the case where the invalid character is
whitespace.
Thanks: Christian Stadelmann
Signed-off-by: Simon McVittie <smcv@collabora.com>
Until now we have been using `%c` to show invalid characters, but in
general that will corrupt our output if the input is non-ASCII,
because the individual bytes of a UTF-8 string are not valid UTF-8 alone.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Instead of announcing ahead of time how many tests will be run, just
log 1..32 or similar at the end, from the done_testing function.
This should go some way towards preventing unnecessary cherry-pick
conflicts when tests added to this script get backported.
Resolves: https://github.com/flatpak/flatpak/issues/6793
Signed-off-by: Simon McVittie <smcv@collabora.com>
This is the equivalent of the function of the same name in Perl's
Test::More. If we already emitted a test plan, it asserts that the
number of tests we planned to do equals the number we actually did.
If not, it assumes that however many tests we have done, that's all of
the tests that we intend to do - this can be useful in test scripts
that routinely increase in length, like test-run.sh which is becoming
rather long (and has frequent conflicts for the "plan" line when we
cherry-pick new test coverage to older branches).
Signed-off-by: Simon McVittie <smcv@collabora.com>
This is approximately the equivalent of 'plan' in Perl Test::More:
it announces how many tests we plan to run.
Signed-off-by: Simon McVittie <smcv@collabora.com>
This will help us to emit correct TAP syntax, without having to always
declare up-front how many tests we are going to run.
Signed-off-by: Simon McVittie <smcv@collabora.com>
We now have subprojects for a few dependencies. libglnx and
variant-schema-compiler used to be git subtrees and we recommend
distributions to use those vendored dependencies. We turned them into
meson wraps, which means they are no longer vendored in. We can include
them with --include-subprojects, but then we have to make sure that the
subprojects we recommend to use from the system (bwrap and
xdg-dbus-proxy) are disabled.
It is not a meson project, but we can use the patch_directory key to
overlay a meson.build file over the git checkout. It uses
meson.override_find_program so that in flatpak, a call to
find_program('variant-schema-compiler') will find the program.
After creating the symlink, open it with O_PATH to pin the inode and
verify the target matches. All subsequent operations (xattrs, chown) go
through the pinned fd instead of path-based operations.
From openat2(2):
EAGAIN how.resolve contains either RESOLVE_IN_ROOT or
RESOLVE_BENEATH, and the kernel could not ensure that a
".." component didn't escape (due to a race condition or
potential attack). The caller may choose to retry the
openat2() call.
We should handle this by simply retrying the syscall.
Reject summaries, subsummaries, and summary indexes that are not in
GVariant normal form before passing them to the generated variant
accessor framework, which performs direct pointer arithmetic without
structural validation.
The cache_dirs_in_use tracks the cache dirs, and which pull it is
associated with. It can also be associated with no pull anymore in which
case the value is NULL.
The iteration over cache_dirs_in_use thus needs to handle NULL values.
fd_map_remap_fd assigns a temporary fd number (++max_fd) that may later
be used as a conflict resolution target. If the source fd passed to
fd_map_remap_fd is higher than max_fd, conflict resolution can assign
a replacement value that collides with that source fd. In
child_setup_func, the dup2 for the conflicting entry then overwrites
the source fd before it is remapped, delivering the wrong fd to the
child process.
In practice this manifests as sandbox-expose-fd-ro passing a random fd
(e.g. a socket) instead of the intended O_PATH fd to flatpak run via
--ro-bind-fd, producing errors like:
error: /proc/self/fd/21 resolves to non-absolute path socket:[423446]
Similar error have been observed in Epiphany/WebKit:
https://bugs.webkit.org/show_bug.cgi?id=305344
We require glib 2.46, so we can't rely on g_hash_table_new_similar.
Fixes: 17cb1135 ("context: Keep fallback-x11 separate from x11 conditionals")
Closes: #6775
On architectures like i386, socket operations were historically
multiplexed through the socketcall syscall. The seccomp socket family
filter only applied to the direct socket syscall, so a binary using
socketcall could bypass it entirely.
Switch from seccomp_rule_add_exact to seccomp_rule_add, which lets
libseccomp also block socketcall. Since libseccomp cannot inspect
socketcall arguments (they are behind a userspace pointer), this
blocks all of socketcall, forcing userspace to use the direct socket
syscalls that have been available on i386 since Linux 4.3.
Older versions of Meson parsed TAP output very strictly, and would fail
the test when a `g_test_message()` mentions non-UTF-8, which results
in GLib adding a prefix on stdout that is not valid TAP syntax.
Ideally our error messages should all be valid UTF-8 even if the input
is not, but that's a larger refactor that shouldn't block security fix
releases, so for now just tell Meson to get the tests' pass/fail status
from their exit status rather than parsing their stdout.
Signed-off-by: Simon McVittie <smcv@collabora.com>
apply_extra_data() used GFile path-based operations that follow
symlinks: --ro-bind/--bind with resolved paths, chmod on the extra
directory path, and flatpak_cp_a for the extra/export merge.
These are not independently exploitable: apply_extra_data only runs
after extract_extra_data has already created files/extra as a real
directory, so there is no symlink left to follow. The apply_extra
script itself runs in a restricted sandbox with dropped caps and no
/proc. The extra/export merge only adds to the app's own export
directory, which is already app-controlled and whose desktop file
Exec keys are rewritten to flatpak run.
Replace with fd-relative operations as defense in depth:
glnx_chaseat with GLNX_CHASE_RESOLVE_NO_SYMLINKS and
GLNX_CHASE_RESOLVE_BENEATH for directory traversal,
--ro-bind-fd/--bind-fd for bwrap mounts, fchmod on the directory fd,
and flatpak_cp_a_at for the export merge.
Helps: https://github.com/flatpak/flatpak/security/advisories/GHSA-fqx6-vh4p-42cg
[smcv: Use glnx_fd_reopen() to reopen O_PATH fd as readable]
Co-authored-by: Simon McVittie <smcv@collabora.com>
extract_extra_data() had two vulnerabilities:
1. It resolved "files/extra" using g_file_resolve_relative_path()
which follows symlinks. A crafted OSTree commit with "files" as a
symlink causes extra-data blobs to be written at the symlink target.
On system installs this runs as root via the system helper, which
validates signatures and checksums but not tree structure.
2. It used g_file_get_child(extradir, name) where name comes from
xa.extra-data-sources in the commit metadata. Names containing ".."
escape the extra/ directory. This is exploitable through the normal
build flow: flatpak build-export rejects "/" but not "..".
Replace GFile path operations with fd-relative operations: open
"files" with glnx_chaseat using GLNX_CHASE_RESOLVE_NO_SYMLINKS,
create "extra" with glnx_chase_and_mkdirat using
GLNX_CHASE_RESOLVE_BENEATH, validate extra-data names against ".",
"..", and "/", and write with glnx_file_replace_contents_at anchored
to the extra directory fd.
[smcv: Open checkoutdir_dfd before trying to open its files subdir]
Co-authored-by: Simon McVittie <smcv@collabora.com>
Resolves: https://github.com/flatpak/flatpak/security/advisories/GHSA-fqx6-vh4p-42cg
Reject extra-data names containing ".", "..", or "/" early during the
download phase. The name is used to construct a local cache file path
via flatpak_build_file() which uses g_file_resolve_relative_path(),
so ".." components could traverse outside the cache directory.
In practice this is not exploitable: the local cache path is only
used for a g_file_query_exists() check followed by g_file_load_contents(),
and the loaded content is verified against the expected sha256 from
the commit metadata.
Helps: https://github.com/flatpak/flatpak/security/advisories/GHSA-fqx6-vh4p-42cg
ensure_extensions() used g_file_resolve_relative_path() with
ext->directory, which comes from runtime/SDK metadata and can contain
".." components. This allowed a malicious extension to write outside
the build directory.
Replace the GFile-based path resolution with glnx_chase_and_mkdirat()
using GLNX_CHASE_RESOLVE_BENEATH | GLNX_CHASE_RESOLVE_NO_SYMLINKS,
which rejects any path that escapes the directory fd.
Convert the rest of flatpak_builtin_build_init to fd-relative
operations (mkdirat, symlinkat, glnx_opendirat, glnx_shutil_rm_rf_at,
glnx_file_replace_contents_at, flatpak_cp_a_at) anchored to a
base_dfd opened from the build directory.
Resolves: https://github.com/flatpak/flatpak/security/advisories/GHSA-8qxj-x646-phcm
We have two use cases for removing refs: uninstalling, and pruning of
undeployed refs. Pruning undeployed refs is something we want anyone to
be able to do, because they can also pull updates and then not deploy
them. Uninstalling arbitrary refs on the other hand is problematic, and
its possible to remove the AppStream ref, and a deployed ref.
So we split removing a ref into a function which removes any ref, and
use it internally to implement e.g. uninstalling, and a function to
remove refs which are safe to remove. Only the latter one will escalate
through the system helper, making it harder for unprivileged users to
get into a state which can be exploited.
See the previous commit for such a state.
Resolves: https://github.com/flatpak/flatpak/security/advisories/GHSA-q4gr-vc25-57m5
Hardlink targets were not rebased to the destination directory,
allowing a crafted archive to hardlink arbitrary host files into the
extraction directory.
Also improve the documentation on how this all works, because it is all
a bit subtle.
Resolves: https://github.com/flatpak/flatpak/security/advisories/GHSA-9rww-v4mm-x4jg
razzeee