Allow disabling character set filtering #113
Conversation
This uses QREXEC_SERVICE_FULL_NAME to detect what the service was invoked as. Non-empty arguments are reserved for future use. This requires QubesOS/qubes-linux-utils#113. Fixes: QubesOS/qubes-issues#8332
See task description - please add a flag to disable this part too (do expose it as an option for |
Will do. I’ll keep validating the paths from the source VM, though. These are produced by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 77.35% 79.45% +2.09%
==========================================
Files 5 5
Lines 424 477 +53
==========================================
+ Hits 328 379 +51
- Misses 96 98 +2 ☔ View full report in Codecov by Sentry. |
DemiMarie
force-pushed
the
allow-less-secure
branch
from
f40aeb8 to
75dc93d
Compare
This requires QubesOS/qubes-linux-utils#113. It also adds a new argument parser based on getopt_long(), which is used instead of the old hand-rolled code unless there are at least two arguments and the first one starts with an ASCII digit. Part of QubesOS/qubes-issues#8332
DemiMarie
force-pushed
the
allow-less-secure
branch
from
75dc93d to
a4369a0
Compare
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024061510-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024052808-4.3&flavor=update
Failed tests13 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/101100#dependencies 37 fixed
Unstable tests
|
Doesn't look like a name that should be rejected in any mode,,, |
|
And while at it, can you change the error message for EILSEQ to something clearer for mere mortals? Maybe "File name contains disallowed characters"? |
It is accepted by |
Already done in afade05, but EILSEQ is also used for non-canonical symbolic links. |
|
Another line from logs (but doesn't really add here): |
DemiMarie
force-pushed
the
allow-less-secure
branch
from
a4369a0 to
21d88b9
Compare
Oops, I switched to the |
|
This (or the other PR) broke salt, specifically the step when files from dom0 are copied into dispvm: I don't have stderr logged to see what file it complained about, but I'll try to save it next time. |
|
I got the stderr, and I'm more confused: The path here is a normal directory. |
|
And relevant part of strace on the unpacker: |
This is most likely due to chroot. There is saved open procfd_fd, but this particular call is likely done by glibc ( |
It is highly critical and is not obvious.
They were rejected without a good reason. They pose no danger not already posed by symlinks of the form "../a".
This is obscure, but it is harmless. It simply means that the target of the symlink must be a directory (or symlink to a directory), otherwise opening the symlink will fail with ENOTDIR. Allowing non-symlink paths to end in '/' is also harmless: opendir_safe() will set the last path component to the empty string, and attempting to create a file, symlink, or directory with the empty string as a name will fail with ENOENT.
This is a theoretical ABI change, but hopefully no third-party app is linking against undocumented functions not part of any header file. Also remove an unused function.
qubes_pure_validate_symbolic_link() validates both the symlink path and target, so validating the path with a separate qubes_pure_validate_file_name() call is unnecessary.
DemiMarie
force-pushed
the
allow-less-secure
branch
from
21d88b9 to
abea7f0
Compare
This requires QubesOS/qubes-linux-utils#113. It also adds a new argument parser based on getopt_long(), which is used instead of the old hand-rolled code unless there are at least two arguments and the first one starts with an ASCII digit. Part of QubesOS/qubes-issues#8332
This requires QubesOS/qubes-linux-utils#113. It also adds a new argument parser based on getopt_long(), which is used instead of the old hand-rolled code unless there are at least two arguments and the first one starts with an ASCII digit. Part of QubesOS/qubes-issues#8332
The error message is not great, but at least it is not actively misleading anymore. Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker and qfile-agent.
This allows copying paths with names that would otherwise be forbidden. This requires adding new APIs, so take the opportunity to provide more useful information about why a path was rejected. To ensure that internal invariants are not violated, this uses a new COMPILETIME_UNREACHABLE macro to validate that a statement can be proven unreachable by the compiler. This is superior to abort() because it is checked at compile-time by the optimizer: if the compiler cannot prove that the code is unreachable, the build will fail. This technique is also used by BUILD_BUG_ON() in the Linux kernel. Since compilers do not promise to always be able to prove a piece of code unreachable, static checking is disabled by default. It can be enabled by including CHECK_UNREACHABLE=1 in the build environment. This is part of QubesOS/qubes-issues#8332 (less restricted qfile-copy), but that issue also requires changes to qfile-unpacker (part of qubes-core-agent-linux) to fix. The code is both backwards and forwards compatible: Old qfile-unpacker versions will work fine with the new library, and new qfile-unpacker versions will work fine with the old library. However, qubes.UnsafeFileCopy will behave like qubes.Filecopy unless the library has been updated. Initially, I decided to unconditionally disallow ASCII control characters in filenames, even if other character set filtering is disabled. This is because they are very useful for exploits and not very useful for other purposes. However, they can still arise in practice for completely legitimate reasons. These include copying and pasting into a file name in a GUI file manager and deliberately creating strangely-named files for test purposes. Therefore, disabling filtering now disables _all_ character set filtering. Restrictions to prevent directory traversal are still enforced, though, because violations of these are much more likely to be exploitable. While symlinks that point outside of the directory being copied might arise legitimately, they will not be meaningful after being copied and _do_ allow an attacker to cause mischief. For instance, if a symlink points to /home/user/.bashrc, "cp a b" in a directory copied by qvm-copy could overwrite ~/.bashrc with attacker-controlled data. The checks on symlink paths prevent this.
Instead of crashing on the first failed test, log an error message for each failed test that points to the location of the error.
This will make subsequent changes easier. No functional change intended.
This will later be exposed by qfile-unpacker, and is a safer alternative to disabling all symbolic link checking.
This turns off all checks for symbolic links, returning to the R4.1 behavior. This is unsafe, but it might be necessary in some cases. However, it will not be included by either qubes.Filecopy or qubes.UnsafeFileCopy.
They would result in a misleading ENOENT error due to passing an empty path to openat(). Instead, reject the path with EILSEQ to indicate a malformed pathname.
DemiMarie
force-pushed
the
allow-less-secure
branch
from
abea7f0 to
044c4d4
Compare
This requires QubesOS/qubes-linux-utils#113. It also adds a new argument parser based on getopt_long(), which is used instead of the old hand-rolled code unless there are at least two arguments and the first one starts with an ASCII digit. Part of QubesOS/qubes-issues#8332 (cherry picked from commit 3a0778b)
This allows copying paths with names that would otherwise be forbidden.
This requires adding new APIs, so take the opportunity to provide more
useful information about why a path was rejected.
To ensure that internal invariants are not violated, this uses a new
COMPILETIME_UNREACHABLE macro to validate that a statement can be proven
unreachable by the compiler. This is superior to abort() because it is
checked at compile-time by the optimizer: if the compiler cannot prove
that the code is unreachable, the build will fail. This technique is
also used by BUILD_BUG_ON() in the Linux kernel. Since compilers do not
promise to always be able to prove a piece of code unreachable, static
checking is disabled by default. It can be enabled by including
CHECK_UNREACHABLE=1 in the build environment.
This is part of QubesOS/qubes-issues#8332 (less restricted qfile-copy),
but that issue also requires changes to qfile-unpacker (part of
qubes-core-agent-linux) to fix. The code is both backwards and forwards
compatible: Old qfile-unpacker versions will work fine with the new
library, and new qfile-unpacker versions will work fine with the old
library. However, qubes.UnsafeFileCopy will behave like qubes.Filecopy
unless the library has been updated.
Initially, I decided to unconditionally disallow ASCII control
characters in filenames, even if other character set filtering is
disabled. This is because they are very useful for exploits and not
very useful for other purposes. However, they can still arise in
practice for completely legitimate reasons. These include copying and
pasting into a file name in a GUI file manager and deliberately creating
strangely-named files for test purposes. Therefore, disabling filtering
now disables all character set filtering. Restrictions to prevent
directory traversal are still enforced, though, because violations of
these are much more likely to be exploitable. While symlinks that point
outside of the directory being copied might arise legitimately, they
will not be meaningful after being copied and do allow an attacker to
cause mischief. For instance, if a symlink points to
/home/user/.bashrc, "cp a b" in a directory copied by qvm-copy could
overwrite ~/.bashrc with attacker-controlled data. The checks on
symlink paths prevent this.