We drop our own capabilities, but it's possible that processes we exec()
could gain extra privilege via file capabilities. It shouldn't be possible
for us to exec() anyway due to seccomp() and our filesystem isolation. But
just in case, zero the bounding and inheritable capability sets to prevent
any such child from gainin privilege.
Note that we do this *after* spawning the pasta shell/command (if any),
because we do want the user to be able to give that privilege if they want.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The current implementation of drop_caps() doesn't really work because it
attempts to drop capabilities from the bounding set. That's not the set
that really matters, it's about limiting the abilities of things we might
later exec() rather than our own capabilities. It also requires
CAP_SETPCAP which we won't usually have.
Replace it with a new version which uses setcap(2) to drop capabilities
from the effective and permitted sets. For now we leave the inheritable
set as is, since we don't want to preclude the user from passing
inheritable capabilities to the command spawed by pasta.
Correctly dropping caps reveals that we were relying on some capabilities
we'd supposedly dropped. Re-divide the dropping of capabilities between
isolate_initial(), isolate_user() and isolate_prefork() to make this work.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Currently, isolate_user() exits early if the --netns-only option is given.
That works for now, but shortly we're going to want to add some logic to
go at the end of isolate_user() that needs to run in all cases: joining a
given userns, creating a new userns, or staying in our original userns
(--netns-only).
To avoid muddying those changes, here we reorganize isolate_user() to have
a common exit path for all cases.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In a few places we use the FWRITE() macro to open a file, replace it's
contents with a given string and close it again. There's no real
reason this needs to be a macro rather than just a function though.
Turn it into a function 'write_file()' and make some ancillary
cleanups while we're there:
- Add a return code so the caller can handle giving a useful error message
- Handle the case of short write()s (unlikely, but possible)
- Add O_TRUNC, to make sure we replace the existing contents entirely
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have a number of steps of self-isolation scattered across our code.
Improve function names and add comments to make it clearer what the self
isolation model is, what the steps do, and why they happen at the points
they happen.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
drop_caps() has a number of bugs which mean it doesn't do what you'd
expect. However, even if we fixed those, the call in pasta_start_ns()
doesn't do anything useful:
* In the common case, we're UID 0 at this point. In this case drop_caps()
doesn't accomplish anything, because even with capabilities dropped, we
are still privileged.
* When attaching to an existing namespace with --userns or --netns-only
we might not be UID 0. In this case it's too early to drop all
capabilities: we need at least CAP_NET_ADMIN to configure the
tap device in the namespace.
Remove this call - we will still drop capabilities a little later in
sandbox().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The end of pasta_start_ns() has a test against pasta_child_pid, testing
if we're in the parent or the child. However we started the child running
the pasta_setup_ns function which always exec()s or exit()s, so if we
return from the clone() we are always in the parent, making that test
unnecessary.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When invoked so as to spawn a shell, pasta checks explicitly for the
shell being bash and if so, adds a "-l" option to make it a login shell.
This is not ideal, since this is a bash specific option and requires pasta
to know about specific shell variants.
There's a general convention for starting a login shell, which is to
prepend a "-" to argv[0]. Use this approach instead, so we don't need bash
specific logic.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The distro and performance tests are by far the slowest part of the passt
testsuite. Move them to the end of the testsuite run, so that it's easier
to do a quick test during development by letting the other tests run then
interrupting the test runner.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
clang says:
./log.h:23:18: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
We need token pasting here just because of the 'format' in trace():
drop it.
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
While building with clang:
./util.h:176:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
If we ping a link-local address, we need to pass this to sendto(), as
it will obviously fail with -EINVAL otherwise.
If we ping other addresses, it's probably a good idea anyway to
specify the configured outbound interface here.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
With default options, when we pass --config-net, the IPv6 address is
actually going to be recycled from the init namespace, so it is in
fact duplicated, but duplicate address detection has no way to find
out.
With a different configured address, that's not the case, but anyway
duplicate address detection will be unable to see this.
In both cases, we're wasting time for nothing.
Pass the IFA_F_NODAD flag as we configure globally scoped IPv6
addresses via netlink.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
If the user specifies an explicit loopback address for a port
binding, we're going to use that address for the 'tap' socket, and
the same exact address for the 'spliced' socket (because those are,
by definition, only bound to loopback addresses).
This means that the second binding will fail, and, unexpectedly, the
port is forwarded, but via tap device, which means the source address
in the namespace won't be a loopback address.
Make it explicit under which conditions we're creating which kind of
socket, by refactoring tcp_sock_init() into two separate functions
for IPv4 and IPv6 and gathering those conditions at the beginning.
Also, don't create spliced sockets if the user specifies explicitly
a non-loopback address, those are harmless but not desired either.
Fixes: 3c6ae62510 ("conf, tcp, udp: Allow address specification for forwarded ports")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In pasta mode, when we receive a new inbound connection, we need to
select a socket that was created in the namespace to proceed and
connect() it to its final destination.
The existing condition might pick a wrong socket, though, if the
destination port is remapped, because we'll check the bitmap of
inbound ports using the remapped port (stored in the epoll reference)
as index, and not the original port.
Instead of using the port bitmap for this purpose, store this
information in the epoll reference itself, by adding a new 'outbound'
bit, that's set if the listening socket was created the namespace,
and unset otherwise.
Then, use this bit to pick a socket on the right side.
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Fixes: 33482d5bf2 ("passt: Add PASTA mode, major rework")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
For tcp_sock_init_ns(), "inbound" connections used to be the ones
being established toward any listening socket we create, as opposed
to sockets we connect().
Similarly, tcp_splice_new() used to handle "inbound" connections in
the sense that they originated from listening sockets, and they would
in turn cause a connect() on an "outbound" socket.
Since commit 1128fa03fe ("Improve types and names for port
forwarding configuration"), though, inbound connections are more
broadly defined as the ones directed to guest or namepsace, and
outbound the ones originating from there.
Update comments for those two functions.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
First off, as we swap endianness for source ports in
udp_fill_data_v{4,6}(), we want host endianness, not network
endianness. It doesn't actually matter if we use htons() or ntohs()
here, but the current version is confusing.
In the IPv4 path, when we remap DNS answers, we already swapped the
endianness as needed for the source port: don't swap it again,
otherwise we'll not map DNS answers for IPv4.
In the IPv6 path, when we remap DNS answers, we want to check that
they came from our upstream DNS server, not the one configured via
--dns-forward (which doesn't even need to exist for this
functionality to work).
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
This is a minor optimisation possibility I spotted while trying to
debug a hang in tap4_handler(): if we run out of space for packet
sequences, it's fine to add packets to an existing per-sequence pool.
We should check the count of packet sequences only once we realise
that we actually need a new packet sequence.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
An n-sized pool, or a pool with n entries, doesn't include index n,
only up to n - 1.
I'm not entirely sure this sanity check actually covers any
practical case, but I spotted this while debugging a hang in
tap4_handler() (possibly due to malformed sequence entries from
qemu).
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Fixes: 745a9ba428 ("pasta: By default, quit if filesystem-bound net namespace goes away")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Since kernel version 5.7, commit c427bfec18f2 ("net: core: enable
SO_BINDTODEVICE for non-root users"), we can bind sockets to
interfaces, if they haven't been bound yet (as in bind()).
Introduce an optional interface specification for forwarded ports,
prefixed by %, that can be passed together with an address.
Reported use case: running local services that use ports we want
to have externally forwarded:
https://github.com/containers/podman/issues/14425
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
This is practical to avoid explicit lifecycle management in users,
e.g. libvirtd, and is trivial to implement.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Coverity now noticed we're checking most lseek() return values, but
not this. Not really relevant, but it doesn't hurt to check we can
actually seek before reading lines.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
In some environments, such as KubeVirt pods, we might not have a
system logger available. We could choose to run in foreground, but
this takes away the convenient synchronisation mechanism derived from
forking to background when interfaces are ready.
Add optional logging to file with -l/--log-file and --log-size.
Unfortunately, this means we need to duplicate features that are more
appropriately implemented by a system logger, such as rotation. Keep
that reasonably simple, by using fallocate() with range collapsing
where supported (Linux kernel >= 3.15, extent-based ext4 and XFS) and
falling back to an unsophisticated block-by-block moving of entries
toward the beginning of the file once we reach the (mandatory) size
limit.
While at it, clarify the role of LOG_EMERG in passt.c.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
This saves some hassle when including passt.h, as we need ETH_ALEN
there.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
This originated as a result of copy and paste to introduce a second
stage for processing options related to port forwarding, has already
bitten David in the past, and just gave me hours of fun.
As a matter of fact, the second set of optstring assignments was
already incorrect, but it didn't matter because the first one was
more restrictive, not allowing optional arguments for -P, -D, -S.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Logging to file is going to add some further complexity that we don't
want to squeeze into util.c.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
To keep this simple, only support tests that have corresponding setup
and teardown functions implied by their path. For example:
./run passt/ndp
will trigger the 'passt' setup and teardown functions.
This is not really elegant, but it looks robust, and while David is
considering proper alternatives, it should be quite useful.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
With gcc 11 and 12, passing -flto, or -flto=auto, and -O2,
intra-procedural optimisation gets rid of a fundamental bit in ndp():
the store of hop_limit in the IPv6 header, before the checksum is
calculated, which on x86_64 looks like this:
ip6hr->hop_limit = IPPROTO_ICMPV6;
b8c0: c6 44 24 35 3a movb $0x3a,0x35(%rsp)
Here, hop_limit is temporarily set to the protocol number, to
conveniently get the IPv6 pseudo-header for ICMPv6 checksum
calculation in memory.
With LTO, the assignment just disappears from the binary.
This is rather visible as NDP messages get a wrong checksum, namely
the expected checksum plus 58, and they're ignored by the guest or
in the namespace, meaning we can't get any IPv6 routes, as reported
by Wenli Quan.
The issue affects a significant number of distribution builds,
including the ones for CentOS Stream 9, EPEL 9, Fedora >= 35,
Mageia Cauldron, and openSUSE Tumbleweed.
As a quick workaround, declare csum_unaligned() as "noipa" for gcc
11 and 12, with -flto and -O2. This disables inlining and cloning,
which causes the assignment to be compiled again.
Leave a TODO item: we should figure out if a gcc issue has already
been reported, and report one otherwise. There's no apparent
justification as to why the store could go away.
Reported-by: Wenli Quan <wquan@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2129713
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Commit c318ffcb4c ("udp: Ignore bogus -Wstringop-overread for
write() from gcc 12.1") uses a gcc pragma to ignore a bogus warning,
which started appearing on gcc 12.1 (aarch64 and x86_64) due to:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103483
...but gcc 12.2 has the same issue. Not just that: if LTO is enabled,
the pragma itself is ignored (this wasn't the case with gcc 12.1),
because of:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922
Drop the pragma, and assign a frame (in the networking sense) pointer
from the offset of the Ethernet header in the buffer, then pass it to
write() and pcap(), so that gcc doesn't obsess anymore with the fact
that an Ethernet header is 14 bytes and we're sending more than that.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Commit 1a563a0cbd ("passt: Address gcc 11 warnings") works around an
issue where the remote address passed to hash functions is seen as
uninitialised by gcc, with -flto and -O2.
It turns out we get the same exact behaviour on gcc 12.1 and 12.2, so
extend the applicability of the same workaround to gcc 12.
Don't go further than that, though: should the issue reported at:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78993
happen to be fixed in a later version of gcc, we won't need the
noinline attributes anymore. Otherwise, we'll notice.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
It's unclear what original suppressions these unmatchedSuppression
suppressions were supposed to go with. They don't trigger any warnings on
the current code that I can tell, so remove them. If we find a problem
with some cppcheck versions in future, replace them with inline
suppressions so it's clearer exactly where the issue is originating.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have a couple of functions that are unused (for now) by design.
Although at least one has a flag so that gcc doesn't warn, cppcheck has its
own warnings about this. Add specific inline suppressions for these rather
than a blanket suppression in the Makefile.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
I can't get this warning to trigger, even without the suppression, so
remove it. If it shows up again on some cppcheck version, we can replace
it with inline suppressions so it's clear where the issue lay.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
I can't get these warnings to trigger on the cppcheck versions I have, so
remove them. If we find in future we need to replace these, they should be
replaced with inline suppressions so its clear what's the section of code
at issue.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
I can't get this warning to trigger, so I think this suppression must be
out of date. Whether that's because we've changed our code to no longer
have the problem, or because cppcheck itself has been updated to remove a
false positive I don't know.
If we find that we do need a suppression like this for some cppcheck
version, we should replace it with an inline suppression so it's clear
what exactly is triggering the warning.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Recent versions of cppcheck give warnings if using a bitwise OR (|) where
some of the arguments are zero. We're triggering these warnings in our
generated seccomp.h header, because BPF_LD and BPF_W are zero-valued.
However putting these defines in makes the generate code clearer, even
though they could be left out without changing the values. So, add
cppcheck suppressions to the generated code.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
seccomp.sh generates seccomp.h, so if we change it, we should re-build
seccomp.h as well. Add this to the make dependencies so it happens.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Recent versions of cppcheck give a warning due to the NULL buffer passed
to recv() in tcp_sock_consume(). Since this apparently works, I assume
it's actually valid, but cppcheck doesn't know that recv() can take a NULL
buffer. So, use a suppression to get rid of the error.
Also add an unmatchedSuppression suppression since only some cppcheck
versions complain about this.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
TIMER_INTERVAL is the minimum of two separately defined intervals which
happen to have the same value at present. This results in an expression
which has the same value in both branches of a ternary operator, which
cppcheck warngs about. This is logically sound in this case, so suppress
the error (we appear to already have a similar suppression for clang-tidy).
Also add an unmatchedSuppression suppression, since only some cppcheck
versions complain about this instance.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
clang-tidy warns that in passing getenv("PATH") to strncpy() we could be
passing a NULL pointer. While it's unusual for PATH to be unset, it's not
impossible and this would indeed cause getenv() to return NULL.
Handle this case by never recognizing argv[2] as a qemu binary name if
PATH is not set. This is... no flakier than the detection of whether
it's a binary name already is.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Since bf95322f "conf: Make the argument to --pcap option mandatory" we
no longer call localtime() in pcap.c, so we no longer need the matching
cppcheck suppression.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In a number of places in passt we use structures to represent over the wire
or in-file data with a fixed layout. After initialization we don't access
the fields individually and just write the structure as a whole to its
destination.
Unfortunately cppcheck doesn't cope with this pattern and thinks all the
structure members are unused. We already have suppressions for this in
pcap.c and dhcp.c However, it also appears in dhcp.c and netlink.c at
least. Since this is likely to be common, it seems wiser to just suppress
the error globally.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We use a number of complex structures to format messages to send to
netlink. In some cases we add imaginary 'end' members not because they
actually mean something on the wire, but so that we can use offsetof() on
the member to determine the relevant size.
Adding extra things to the structures for this is kinda nasty. We can use
a different construct with offsetof and sizeof to avoid them. As a bonus
this removes some cppcheck warnings about unused struct members.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
strtok() is non-reentrant and old-fashioned, so cppcheck would complains
about its use in conf.c if it weren't suppressed. We're single threaded
and strtok() is convenient though, so it's not really worth reworking at
this time. Convert this to an inline suppression so it's adjacent to the
code its annotating.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
qrap.c uses several old-fashioned functions that cppcheck complains about.
Since it's headed for obselesence anyway, just suppress these rather than
attempting to modernize the code.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We define our own ffsl() as a weak symbol, in case our C library doesn't
include it. On glibc systems which *do* include it, this causes a cppcheck
warning because unsurprisingly our version doesn't pick the same argument
names. Convert the suppression for this into an inline suppression.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
