Commit Graph

102 Commits

Author SHA1 Message Date
openshift-merge-bot[bot] 83b5dcf219
Merge pull request #5753 from nalind/newline-error
chroot: add newlines at the end of printed error messages
2024-10-23 13:55:54 +00:00
Kir Kolyshkin 95f2e1020f vendor: switch to moby/sys/capability
github.com/moby/sys/capability is a fork of the (no longer maintained)
github.com/syndtr/gocapability package.

For the list of changes since the fork took place, see
https://github.com/moby/sys/blob/main/capability/CHANGELOG.md

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2024-09-30 13:19:03 -07:00
Kir Kolyshkin 37a6bde534 Don't set ambient capabilities
Ambient capabilities can't be raised without inheritable ones, and since we
don't raise inheritable, we should not raise ambient either.

This went unnoticed because of a bug in syndtr/gocapability which is
only fixed in its fork (see the next commit).

Amends commit e7e55c988.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2024-09-30 13:19:03 -07:00
Nalin Dahyabhai 2ff91b8b08 chroot: add newlines at the end of printed error messages
If we print an error message to stderr, end it with a newline.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-09-24 17:11:16 -04:00
Nalin Dahyabhai 0ad0ffaad9 Fix some govet linter warnings
govet warned about some places where we were passing something other
than a literal string to a function that took format specifiers.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-08-15 16:09:24 -04:00
Nalin Dahyabhai 8ae99121c1 CI: enable the gofumpt linter
Turn on the gofumpt linter.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-08-15 13:17:44 -04:00
Kir Kolyshkin fd3aba9917 *: use gofmt -s, add gofmt linter
1. Use gofmt to format the code:

	git ls-files \*.go | grep -Ev '/?vendor/' | xargs gofmt -w -s

2. Add gofmt to golangci-lint (it's not enabled by default).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2024-08-09 17:05:58 -07:00
Kir Kolyshkin 24da18800e *: fix build tags
This change is generated by `go1.23rc2 fix ./...`.

Had to use go1.23rc2, since all released go versions have a bug
preventing it from working with `go 1.22.0` in go.mod (opened
https://github.com/golang/go/issues/68825,
https://github.com/golang/go/issues/68824 for awareness).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2024-08-09 17:05:30 -07:00
Nalin Dahyabhai fdf1c75cd3 linters: unused arguments shouldn't have names
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-08-07 10:10:35 -04:00
Nalin Dahyabhai ae0d9d002e linters: don't name variables "cap"
Rename some variables so that we don't use a keyword as a variable name.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-08-07 10:10:35 -04:00
Jonah Bull 939a58b967
fix secret mounts for env vars when using chroot isolation
Before #5083, when running with chroot isolation ro mounts like secrets
from env vars would explicitly have the unix.MS_NOEXEC, unix.MS_NOSUID
and unix.MS_NODEV flags set when they were remounted. Now when running
with chroot isolation ro mounts like secrets from env vars are not
getting those same flags set and so the remount operation fails.
Specifically it looks like we are missing the unix.MS_NOSUID and
unix.MS_NODEV flags.

This change adds special handling for read-only mounts when we need to do
a remount to try to get the desired flags to stick. If we've requested
a read-only mount (unix.ST_RDONLY is set in requestFlags), then we add any
possibleImportantFlags that are set in fs.Flags to remountFlags so the remount
operation doesn't fail because they are missing. I've also added a test to
bud.bats that covers this case.

Signed-off-by: Jonah Bull <jonah.bull@elastic.co>
2024-05-25 15:49:51 -05:00
Giuseppe Scrivano 9dcd1cc9a6
chroot: use fileutils.(Le|E)xists
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2024-04-12 09:58:13 +02:00
Nalin Dahyabhai 4f0b619dd2 Use golang.org/x/exp/slices.Contains
... instead of github.com/containers/common/pkg/util.StringInSlice,
per linters.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-02-07 11:50:41 -05:00
Nalin Dahyabhai 04847f57f6 Set CONTAINERS_CONF in the chroot-mount-flags integration test
... in an attempt to try to get UID 0 in a user namespace to stop trying
to read files from root's home directory, where the permissions error is
treated as a hard failure.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2024-02-07 11:48:29 -05:00
Philip Dubé a42bfd0191 Replace map[K]bool with map[K]struct{} where it makes sense
Signed-off-by: Philip Dubé <philip@peerdb.io>
2024-01-05 15:58:43 +00:00
Nalin Dahyabhai 2a3a956cbb chroot.setupChrootBindMounts: pay more attention to flags
Pay better attention to dev/nodev/exec/noexec/suid/nosuid/ro/rw flags on
bind, overlay, and tmpfs mounts when any of them are specified.  Stop
quietly adding "nodev" when it isn't asked for.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2023-10-20 09:46:58 -04:00
Michal Biesek 5643a7fa8c
chroot: `setSeccomp` add support for `ArchPARISC(64)` and `ArchRISCV64`
Signed-off-by: Michal Biesek <michalbiesek@gmail.com>
2023-08-17 09:11:16 +02:00
Giuseppe Scrivano e8d11201a9
chroot: lock thread before setPdeathsig
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2023-05-16 20:07:57 +02:00
Chris Evich 46eea31588
Replace io/ioutil calls with os calls
In golang 1.19, `io/ioutil` is fully deprecated preventing Buildah from
compiling.  Replace all calls with equivalent calls from the `os`
package.

Signed-off-by: Chris Evich <cevich@redhat.com>
2022-12-06 14:29:32 -05:00
Giuseppe Scrivano ffb00243f1
chroot: fix mounting of ro bind mounts
a bind mount cannot be made RDONLY in the same mount operation as it
is created.  For that we need a second operation.

Closes: https://github.com/containers/buildah/issues/4203

[NO NEW TESTS NEEDED] it fails in Buildah in a container

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2022-11-01 21:20:15 +01:00
Daniel J Walsh 8d5d763213
Fix stutters
Podman adds an Error: to every error message. So starting an error
message with "error" ends up being reported to the user as

Error: error ...

This patch removes the stutter.

Also ioutil.ReadFile errors report the Path, so wrapping the err message
with the path causes a stutter.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

[NO NEW TESTS NEEDED]

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
2022-09-19 07:11:44 -04:00
Doug Rabson 9c147ab8a9 chroot: Fix cross build break
Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:48 +01:00
Doug Rabson 309f714b5f chroot: Move isDevNull to run_common.go
Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:48 +01:00
Doug Rabson 363bf9c5bc chroot: Fix setRlimit build on FreeBSD
On FreeBSD, members of the rlimit structure are signed, not unsigned.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:48 +01:00
Doug Rabson dc49c3cb4d chroot: Move parseRLimits and setRlimits to run_common.go
Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson b64d814325 chroot: Fix runUsingChrootExecMain on FreeBSD
This adds no-op stubs for various things and adds an optional override
for creating the container chroot - on FreeBSD we use a jail to allow
setting the container hostname.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson 4d963eb5e1 chroot: Move runUsingChrootExecMain to run_common.go
Again, this breaks the FreeBSD build and this will be addressed by
refactoring in the next commit.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson 0a61e4b280 chroot: Factor out Linux-specific unshare options from runUsingChroot
Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson 3b42b51524 chroot: Move runUsingChroot to run_common.go
This intentionally breaks the FreeBSD so that I can move the code
unmodified which will help with future merge conflicts. A subsequent
commit will resolve this by factoring out Linux-specific code.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson 350c509ecf chroot: Move RunUsingChroot and runUsingChrootMain to run_common.go
This leaves runUsingChrootSubprocOptions in the platform-specific file
since syscall.SysProcIDMap isn't available on FreeBSD.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson 188dcc3608 chroot: Factor out /dev/ptmx pty implementation
The ptmx device is fairly common and this code could be used on
platforms other than Linux.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Doug Rabson f9814601dd chroot: Add FreeBSD support for run with chroot isolation
This copies a large amount of code from run_linux.go. Later diffs in
this stack will factor out duplicate code where possible.

Terminal handling is implemented using the posix_openpt API. We could
use the Linux implementation which uses the /dev/ptmx but that is not
present on standard FreeBSD installs - its supplied by an optional
kernel module. Conversely, posix_openpt could be used for both platforms
but has a downside of requiring cgo so its probably better to use this
only on FreeBSD.

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-09-01 13:50:47 +01:00
Eng Zer Jun 0c4b19ba83
test: use `T.TempDir` to create temporary test directory
This commit replaces `ioutil.TempDir` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.

Prior to this commit, temporary directory created using `ioutil.TempDir`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
	defer func() {
		if err := os.RemoveAll(dir); err != nil {
			t.Fatal(err)
		}
	}
is also tedious, but `t.TempDir` handles this for us nicely.

Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
2022-08-20 21:13:27 +08:00
Nalin Dahyabhai bb149ea686 Use errors.Is() instead of os.Is{Not,}Exist
If errors for which os.IsExist() or os.IsNotExist() would have returned
true have been wrapped using fmt.Errorf()'s "%w" verb, os.IsExist() and
os.IsNotExist(), not having been retrofitted to use errors.Is(), will
return false.

Use errors.Is() to check if an error is an os.ErrExist or os.ErrNotExist
error instead of calling os.IsExist() or os.IsNotExist().

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2022-07-26 15:36:58 -04:00
Doug Rabson ac4b39a220 Rename chroot/run.go to chroot/run_linux.go
This is a precursor to adding isolation=chroot support for
FreeBSD.

[NO NEW TESTS NEEDED]

Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-07-25 11:28:51 +01:00
Doug Rabson 76a0c82131 Sync call signature for RunUsingChroot with chroot/run.go
Signed-off-by: Doug Rabson <dfr@rabson.org>
2022-07-12 09:14:21 +01:00
Giuseppe Scrivano 9445aa12ad
chroot: honor DefaultErrnoRet
honor the default errno ret value specified for the seccomp profile.

[NO NEW TESTS NEEDED]

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2022-07-08 10:51:12 +02:00
Sascha Grunert ce384684c0
Switch to golang native error wrapping
We now use the golang error wrapping format specifier `%w` instead of
the deprecated github.com/pkg/errors package.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
2022-07-07 11:41:47 +02:00
dependabot[bot] 8bbe7a6066
build(deps): bump github.com/opencontainers/runc from 1.1.2 to 1.1.3
[NO NEW TESTS NEEDED]

Bumps [github.com/opencontainers/runc](https://github.com/opencontainers/runc) from 1.1.2 to 1.1.3.
- [Release notes](https://github.com/opencontainers/runc/releases)
- [Changelog](https://github.com/opencontainers/runc/blob/v1.1.3/CHANGELOG.md)
- [Commits](https://github.com/opencontainers/runc/compare/v1.1.2...v1.1.3)

---
updated-dependencies:
- dependency-name: github.com/opencontainers/runc
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
2022-06-09 09:33:00 -04:00
Nalin Dahyabhai 8b0f5be4b8 run: set parent-death signals and forward SIGHUP/SIGINT/SIGTERM
Restore setting of the parent-death signal when we're running
subprocesses in Run(), so that if we get killed, the child processes
will also get killed.

While a child process is running, if we receive SIGHUP, SIGINT, or
SIGTERM, forward the signal to our child process unless it's the command
we're executing, which we SIGKILL without mercy, and finish the current
routine, which will then notice that the child process has exited and
return an error to its caller.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2022-03-30 13:54:38 -04:00
Giuseppe Scrivano e7e55c988c
do not set the inheritable capabilities
The kernel never sets the inheritable capabilities for a process, they
are only set by userspace.  Emulate the same behavior.

Closes: CVE-2022-27651

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2022-03-23 09:09:39 +01:00
Ruben Jenster e6afa0cd5d
caps: fix buildah run --cap-add=all
Fixes #3755

[NO NEW TESTS NEEDED]

Signed-off-by: Ruben Jenster <r.jenster@drachenfels.de>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
2022-02-03 08:44:28 -05:00
Kir Kolyshkin 3d93408a06 all: fix capabilities.NewPid deprecation warnings
Since commit 3eef1ed0bd (January 2019) using
capabilities.NewPid() is deprecated.

Replace with NewPid2().

Note that in chroot/run.go we used to load then clear all capabilities
bits. With NewPid2, this is no longer needed -- we do not load caps, so
there is no need to clear.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2022-01-18 12:56:35 -08:00
Nalin Dahyabhai 81f2af5021
chroot: don't use the generate default seccomp filter for unit tests
When we link our test helper statically using the external linker, the
hardwired default seccomp filter we get from the runtime-tools generator
triggers a hang in it at startup.

Rather than switch to the internal linker, which seems to work around
this, start using the same seccomp filter for unit tests that we
actually use in real life, leaving analysis of which difference between
the two is responsible for it for another day.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-11-18 14:07:50 -05:00
Nalin Dahyabhai 1135bcac5e Fix an error message: unlocking vs locking
Don't complain about being unable to lock a descriptor when we attempted
to unlock it.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-11-11 14:22:11 -05:00
Nalin Dahyabhai 86c1cd4610 chroot: accept an "rw" option
crun and runc accept a "rw" option, so we should expect to see it from
time to time.  Recognize "dev", "suid", and "exec", too.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-11-11 14:19:48 -05:00
Nalin Dahyabhai c6e2a5e87d Replace fmt.Sprintf("%d", x) with strconv.Itoa(x)
Replace calls to fmt.Sprintf("%d", x) with strconv.Itoa(x), which is
slightly faster.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-09-27 18:27:22 -04:00
Nalin Dahyabhai 1ec4983d50 Replace golang.org/x/crypto/ssh/terminal with golang.org/x/term
The golang.org/x/crypto/ssh/terminal package has been deprecated and
replaced upstream by golang.org/x/term, so switch to that.  It's a
simple 1:1 replacement.

[NO NEW TESTS NEEDED]

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-08-24 15:35:12 -04:00
Nalin Dahyabhai a468ce0ffd chroot: fix environment value leakage to intermediate processes
Blake Burkhart reports that when running processes using "chroot"
isolation, the process being run can examine the environment of its
immediate parent and grandparent processes (CVE-2021-3602).

When run in a container in a CI/CD environment, the environment may
include sensitive information which was shared with the container in
order to be used only by buildah itself.  The command being executed is
able to read such information.

This patch reduces the set of environment variables passed to these
intermediate processes, from all variables to the one which is used to
control the level of debug logging.  It also corrects a misleading debug
message and expands the description of chroot isolation in man pages.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
2021-07-15 10:08:38 -04:00
Urvashi Mohnani 3598ffb167 Check for symlink in builtin volume
Check if a builtin volume is a symlink. If it is,
follow the symlink and ensure that the destination
exists.
Add tests for symlink and no symlink case.

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
2021-04-01 13:38:46 -04:00