When interface names are specified in forwarding specs, we need to check
the length of the given interface name against the limit of IFNAMSIZ - 1
(15) characters. However, we managed to have 3 separate off-by-one errors
here meaning we only accepted interface names up to 12 characters.
1. At the point of the check 'ifname' was still on the '%' character, not
the first character of the name, meaning we overestimated the length by
one
2. At the point of the check 'spec' had been advanced one character past
the '/' which terminates the interface name, meaning we overestimated
the length by another one
3. We checked if the (miscalculated) length was >= IFNAMSIZ - 1, that is
>= 15, whereas lengths equal to 15 should be accepted.
Correct all 3 errors.
Link: https://bugs.passt.top/show_bug.cgi?id=61
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Network interface names must fit in a buffer of IFNAMSIZ bytes, including
the terminating \0. IFNAMSIZ is 16 on Linux, so interface names can be
up to (and including) 15 characters long.
We validate this for the -I option, but we have an off by one error. We
pass (IFNAMSIZ - 1) as the buffer size to snprintf(), but that buffer size
already includes the terminating \0, so this actually truncates the value
to 14 characters. The return value returned from snprintf() however, is
the number of characters that would have been printed *excluding* the
terminating \0, so by comparing it >= IFNAMSIZ - 1 we are giving an error
on names >= 15 characters rather than strictly > 15 characters.
Link: https://bugs.passt.top/show_bug.cgi?id=61
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
nl_addr() and nl_route() take an 'op' selector which affects a number of
parameters to the netlink call. Unfortunately when we introduced this
option a bug was introduced so that we always use the interface index for
the host side, rather than the one for the pasta namespace.
Really, the entire interface to nl_addr() and nl_route() is pretty bad:
it's tightly coupled with the use cases of its callers. This is a minimal
fix which doesn't address that, but also doesn't make it significantly
worse.
Link: https://bugs.passt.top/show_bug.cgi?id=59
Fixes: 2fe0461856 ("netlink: Add functionality to copy routes from outer namespace")
Fixes: e89da3cf03 ("netlink: Add functionality to copy addresses from outer namespace")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When the open() or setns() calls fails pasta exits early and prints an
error. However it did not include the errno so it was impossible to know
why the syscall failed.
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Split print to fit 80 columns in pasta_open_ns()]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When pasta is started from an existing userns and tries to join the
netns from another process it fails to open /proc/$pid/ns/net due the
missing CAP_SYS_PTRACE capability in the --netns-only case.
A simple reproducer for this.
First create a userns:
$ unshare -r
Then create a new netns inside it and try to join that netns with pasta.
$ unshare -n sleep inf &
$ pasta --config-net --netns /proc/$!/ns/net
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
While --no-copy-addrs and --no-copy-routes only make sense with
--config-net, and they are implied on -g and -a, respectively, that
doesn't mean we should refuse -a or -g without --config-net: they are
still relevant for a number of things (including DHCP/DHCPv6/NDP
configuration).
Reported-by: Gianluca Stivan <me@yawnt.com>
Fixes: cc9d16758b ("conf, pasta: With --config-net, copy all addresses by default")
Fixes: da54641f14 ("conf, pasta: With --config-net, copy all routes by default")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
I wrote it in commit message and man page, but not in conf()...
Note that -g/--gateway correctly implies --no-copy-routes already.
This fixes Podman's tests:
podman networking with pasta(1) - IPv4 address assignment
podman networking with pasta(1) - IPv4 default route assignment
where we pass -a and -g to assign an address and a default gateway
that's compatible with it, but -a doesn't disable the copy of
addresses, so we ignore -a, and the default gateway is incompatible
with the addresses we copy -- hence no routes in the container.
Link: https://github.com/containers/podman/pull/18612
Fixes: cc9d16758b ("conf, pasta: With --config-net, copy all addresses by default")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
seccomp.sh generates seccomp.h piece by piece using >> directives. This
means that if two instances of seccomp.h are run concurrently a corrupted
version of seccomp.h will be generated. Amongst other problems this can
cause spurious failures on clang-tidy.
Alter seccomp.sh to build the output in a temporary file and atomic move it
to seccomp.h, so concurrent invocations will still result in valud output.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Erik suggests that this makes it easier to grep for options, and with
--help we're anyway printing usage information as expected, not as
part of an error report.
While at it: on -h, we should exit with 0.
Reported-by: Erik Sjölund <erik.sjolund@gmail.com>
Link: https://bugs.passt.top/show_bug.cgi?id=52
Link: https://bugs.passt.top/show_bug.cgi?id=53
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Since commit 0515adceaa ("passt, pasta: Namespace-based sandboxing,
defer seccomp policy application"), it makes no sense to close and
reopen the tap device on error: we don't have access to /dev/net/tun
after the initial setup phase.
If we hit ENOBUFS while writing (as reported: in one case because
the kernel actually ran out of memory, with another case under
investigation), or ENOSPC, we're supposed to drop whatever data we
were trying to send: there's no room for it.
Handle EINTR just like we handled EAGAIN/EWOULDBLOCK: there's no
particular reason why sending the same data should fail again.
Anything else I can think of would be an unrecoverable error: exit
with failure then.
While at it, drop a useless cast on the write() call: it takes a
const void * anyway.
Reported-by: Gianluca Stivan <me@yawnt.com>
Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu>
Fixes: 0515adceaa ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
a7359f0948 ("conf: Don't exit if sourced default route has no gateway")
was supposed to allow passt/pasta to run even if given a template interface
which has no default gateway. However a mistake in the patch means it
still requires the gateway, but doesn't require a global address for the
guest which we really do need.
This is one part (but not the only part) of the problem seen in
https://bugs.passt.top/show_bug.cgi?id=50.
Reported-by: Justin Jereza <justinjereza@gmail.com>
Fixes: a7359f0948 ("conf: Don't exit if sourced default route has no gateway")
Link: https://bugs.passt.top/show_bug.cgi?id=50
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
nstool loops on accept(), but failed to close the accepted socket fds
before continuing on. So, with repeated commands it would eventually die
with an EMFILE.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Normal filesystem paths can be very long (PATH_MAX is around 8k), however
Unix domain sockets can only use relatively short paths (UNIX_PATH_MAX is
108 on Linux). Currently nstool will simply truncate paths that are too
long, leading to difficult to understand failures.
Make such failures clearer, with an explicit error message if given a path
that's too long.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Use the newly-introduced NL_DUP mode for nl_addr() to copy all the
addresses associated to the template interface in the outer
namespace, unless --no-copy-addrs (also implied by -a) is given.
This option is introduced as deprecated right away: it's not expected
to be of any use, but it's helpful to keep it around for a while to
debug any suspected issue with this change.
This is done mostly for consistency with routes. It might partially
cover the issue at:
https://bugs.passt.top/show_bug.cgi?id=47
Support multiple addresses per address family
for some use cases, but not the originally intended one: we'll still
use a single outbound address (unless the routing table specifies
different preferred source addresses depending on the destination),
regardless of the address used in the target namespace.
Link: https://bugs.passt.top/show_bug.cgi?id=47
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Similarly to what we've just done with routes, support NL_DUP for
addresses (currently not exposed): nl_addr() can optionally copy
mulitple addresses to the target namespace, by fixing up data from
the dump with appropriate flags and interface index, and repeating
it back to the kernel on the socket opened in the target namespace.
Link-local addresses are not copied: the family is set to AF_UNSPEC,
which means the kernel will ignore them. Same for addresses from a
mismatching address (pre-4.19 kernels without support for
NETLINK_GET_STRICT_CHK).
Ignore IFA_LABEL attributes by changing their type to IFA_UNSPEC,
because in general they will report mismatching names, and we don't
really need to use labels as we already know the interface index.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
If we use a template interface without a gateway on the default
route, we can still offer almost complete functionality, except that,
of course, we can't map the gateway address to the outer namespace or
host, and that we have no obvious server address or identifier for
use in DHCP's siaddr and option 54 (Server identifier, mandatory).
Continue, if we have a default route but no default gateway, and
imply --no-map-gw and --no-dhcp in that case. NDP responder and
DHCPv6 should be able to work as usual because we require a
link-local address to be present, and we'll fall back to that.
Together with the previous commits implementing an actual copy of
routes from the outer namespace, this should finally fix the
operation of 'pasta --config-net' for cases where we have a default
route on the host, but no default gateway, as it's the case for
tap-style routes, including typical Wireguard endpoints.
Reported-by: me@yawnt.com
Link: https://bugs.passt.top/show_bug.cgi?id=49
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
This reverts commit 7656a6f888: now, by
default, we copy all the routes associated to the outbound interface
into the routing table of the container, so there's no need for this
horrible workaround anymore.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Use the newly-introduced NL_DUP mode for nl_route() to copy all the
routes associated to the template interface in the outer namespace,
unless --no-copy-routes (also implied by -g) is given.
This option is introduced as deprecated right away: it's not expected
to be of any use, but it's helpful to keep it around for a while to
debug any suspected issue with this change.
Otherwise, we can't use default gateways which are not, address-wise,
on the same subnet as the container, as reported by Callum.
Reported-by: Callum Parsey <callum@neoninteger.au>
Link: https://github.com/containers/podman/issues/18539
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Instead of just fetching the default gateway and configuring a single
equivalent route in the target namespace, on 'pasta --config-net', it
might be desirable in some cases to copy the whole set of routes
corresponding to a given output interface.
For instance, in:
https://github.com/containers/podman/issues/18539
IPv4 Default Route Does Not Propagate to Pasta Containers on Hetzner VPSes
configuring the default gateway won't work without a gateway-less
route (specifying the output interface only), because the default
gateway is, somewhat dubiously, not on the same subnet as the
container.
This is a similar case to the one covered by commit 7656a6f888
("conf: Adjust netmask on mismatch between IPv4 address/netmask and
gateway"), and I'm not exactly proud of that workaround.
We also have:
https://bugs.passt.top/show_bug.cgi?id=49
pasta does not work with tap-style interface
for which, eventually, we should be able to configure a gateway-less
route in the target namespace.
Introduce different operation modes for nl_route(), including a new
NL_DUP one, not exposed yet, which simply parrots back to the kernel
the route dump for a given interface from the outer namespace, fixing
up flags and interface indices on the way, and requesting to add the
same routes in the target namespace, on the interface we manage.
For n routes we want to duplicate, send n identical netlink requests
including the full dump: routes might depend on each other and the
kernel processes RTM_NEWROUTE messages sequentially, not atomically,
and repeating the full dump naturally resolves dependencies without
the need to actually calculate them.
I'm not kidding, it actually works pretty well.
Link: https://github.com/containers/podman/issues/18539
Link: https://bugs.passt.top/show_bug.cgi?id=49
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
In pasta_wait_for_ns(), open() failing with ENOENT is expected: we're
busy-looping until the network namespace appears. But any other
failure is not something we're going to recover from: return right
away if we don't get either success or ENOENT.
Now that pasta_wait_for_ns() can actually fail, handle that in
pasta_start_ns() by reporting the issue and exiting.
Looping on EPERM, when pasta doesn't actually have the permissions to
join a given namespace, isn't exactly a productive thing to do.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If pasta spawns a child process while running as UID 0, which is only
allowed from a non-init namespace, we need to keep CAP_SETFCAP before
pasta_start_ns() is called: otherwise, starting from Linux 5.12, we
won't be able to update /proc/self/uid_map with the intended mapping
(from 0 to 0). See user_namespaces(7).
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
If we want /proc contents to be consistent after pasta spawns a child
process in a new PID namespace (only for operation without a
pre-existing namespace), we need to mount /proc after the clone(2)
call with CLONE_NEWPID, and we enable the child to do that by
passing, in the same call, the CLONE_NEWNS flag, as described by
pid_namespaces(7).
This is not really a remount: in fact, passing MS_REMOUNT to mount(2)
would make the call fail. We're in another mount namespace now, so
it's a fresh mount that has the effect of hiding the existing one.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
We'll need this in isolate_initial(). While at it, don't rely on
BUFSIZ: the earlier issue we had with musl reminded me it's not a
magic "everything will fit" value. Size the read buffer to what we
actually need from uid_map, and check for the final newline too,
because uid_map is organised in lines.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When we receive packets from the tap side, we update the addr_seen fields
to reflect the last known address of the guest or ns. For ip4.addr_seen
we, sensibly, only update if the address we've just seen isn't 0 (0.0.0.0).
This case can occur during early DHCP transactions.
We have no equivalent case for IPv6. We're less likely to hit this,
because DHCPv6 uses link-local addresses, however we can see an source
address of :: with certain multicast operations. This can bite us if we
try to make an incoming connection very early after starting pasta with
--config-net: we may have only seen some of those multicast packets,
updated addr_seen to :: and not had any "real" packets to update it to a
global address. I've seen this with some of the avocado test conversions.
In any case, it can never make sense to update addr_seen to ::, so
explicitly exclude that case.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Valtteri reports that if SIGPIPE already has a disposition set by the
parent process, such as systemd with the default setting of
IgnoreSIGPIPE=yes, signal() will return the previous value, not zero,
and this is not an error: check for SIG_ERR instead.
While at it, split messages for failures of sigaction() and signal(),
and report the actual error.
Reported-by: Valtteri Vuorikoski <vuori@notcom.org>
Fixes: 8534be076c ("Catch failures when installing signal handlers")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If we enter a mount namespace with nstool exec our working directory will
be changed to / in the new mount ns. This is surprising if we haven't
actually altered any mounts yet in the new ns. Instead, change the working
directory to match that of the holder process in this situation.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This is possible useful in nstool info and has further uses for nstool
exec.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Using this, rather than using "nstool info" to get the pid then manually
connecting with nsenter makes things a little simpler.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Unlike ${DEBUG} we don't initialize ${TRACE} to 0 if not set, which cases
failures when testing it later. That failure acts as though it is false,
however it emits spurious errors in script.log, which can make it harder to
spot real errors.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This allows you to run commands within a user namespace with the
privilege that comes from owning that userns.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This combines nstool info -pw <sock> with nsenter with various options for
a more convenient and less verbose of entering existing nstool managed
namespaces.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Will make things a bit less verbose in future.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
So that we'll probably give a better error if you point it at something
that's not an nstool hold control socket.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Give nstool the ability to detect what namespaces the target process is in,
relative to where it's called. That is, those namespace types for which
the target is not in the same namespace as the caller. For now, just
print this information with "info", which can be useful for debugging.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The new subcommand gives more information about the holder process and its
namespace, and may be further extended in future. Add some options which
give the old behaviour for existing scripts.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This will make it easier to differentiate the options to those commands
further in future.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Having the "subcommand" first is more conventional and will make it more
natural for future extensions I have planned.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In preparation for extending what it does.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
context_run() has a race condition if two commands are run in close
proximity (generally involving at least one in the background). Because we
always use the same name for the temporary fifo files, if another command
is issued while the fifos for the first still exist, mkfifo will fail,
typically causing the entire test script to jam.
Create unique names for the temporary fifos to avoid this problem.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In practical terms, passt doesn't benefit from the additional
protection offered by the AGPL over the GPL, because it's not
suitable to be executed over a computer network.
Further, restricting the distribution under the version 3 of the GPL
wouldn't provide any practical advantage either, as long as the passt
codebase is concerned, and might cause unnecessary compatibility
dilemmas.
Change licensing terms to the GNU General Public License Version 2,
or any later version, with written permission from all current and
past contributors, namely: myself, David Gibson, Laine Stump, Andrea
Bolognani, Paul Holzinger, Richard W.M. Jones, Chris Kuhn, Florian
Weimer, Giuseppe Scrivano, Stefan Hajnoczi, and Vasiliy Ulyanov.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
That was meant to be an example, and I just dropped it in the
previous commit -- passt.if should be more than enough as a possible
example.
Reported-by: Carl G. <carlg@fedoraproject.org>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2182145
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This was meant to be an example, but I managed to add syntax errors
to it. Drop it altogether.
Reported-by: Carl G. <carlg@fedoraproject.org>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2182145
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Somebody might want to bind listening sockets to a specific
interface, but not a specific address, and there isn't really a
reason to prevent that. For example:
-t %eth0/2022
Alternatively, we support options such as -t 0.0.0.0%eth0/2022 and
-t ::%eth0/2022, but not together, for the same port.
Enable this kind of syntax and add examples to the man page.
Reported-by: Paul Holzinger <pholzing@redhat.com>
Link: https://github.com/containers/podman/issues/14425#issuecomment-1485192195
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Since commit cc6d8286d1 ("tcp: Reset ACK_FROM_TAP_DUE flag only as
needed, update timer"), we don't clear ACK_FROM_TAP_DUE whenever we
process an ACK segment, but, more correctly, only if we're really not
waiting for a further ACK segment, that is, only if the acknowledged
sequence matches what we sent.
In the new function implementing this, tcp_update_seqack_from_tap(),
we also reset the retransmission counter and store the updated ACK
sequence. Both should be done iff forward progress is acknowledged,
implied by the fact that the new ACK sequence is greater than the
one we previously stored.
At that point, it looked natural to also include the statements that
clear and set the ACK_FROM_TAP_DUE flag inside the same conditional
block: if we're not making forward progress, the need for an ACK, or
lack thereof, should remain unchanged.
There might be cases where this isn't true, though: without the
previous commit 4e73e9bd65 ("tcp: Don't special case the handling
of the ack of a syn"), this would happen if a tap-side client
initiated a connection, and the server didn't send any data.
At that point we would never, in the established state of the
connection, call tcp_update_seqack_from_tap() with reported forward
progress.
That issue itself is fixed by the previous commit, now, but clearing
ACK_FROM_TAP_DUE only on ACK sequence progress doesn't really follow
any logic.
Clear the ACK_FROM_TAP_DUE flag regardless of reported forward
progress. If we clear it when it's already unset, conn_flag() will do
nothing with it.
This doesn't fix any known functional issue, rather a conceptual one.
Fixes: cc6d8286d1 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer")
Reported-by: David Gibson <david@gibson.dropbear.id.au>
Analysed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
