On Fri, May 21, 2021 at 3:30 PM Moritz Muehlenhoff <j...@debian.org> wrote:
> Package: golang-github-containers-image > Severity: important > Tags: security > X-Debbugs-Cc: Debian Security Team <t...@security.debian.org> > > This was assigned CVE-2021-20291: > > https://github.com/containers/storage/commit/306fcabc964470e4b3b87a43a8f6b7d698209ee1 > > > Moritz, here is some more context on severity impact of this issue: https://bugzilla.redhat.com/show_bug.cgi?id=1939485#c23 > For applications like podman the affect is minimal - podman pull and it seemingly waits to download the image forever, cancel and the affect is negated. For something like crio the affect is more severe, with the malicious image locking the service up, BUT it is still somewhat responsive. The service then must be killed before returning back to normal. The referenced commit changes the containers/storage code to no longer fork()/exec() out to the xz executable, but instead use the golang-native implementation of golang-github-ulikunitz-xz-dev, which addresses the deadlock situation by avoiding awkward unix process coordination. Upstream switched to version 0.5.10, whereas we only have 0.5.6 in Debian. That version is at least susceptible against: - https://github.com/ulikunitz/xz/issues/35 - CVE-2020-16845 - https://github.com/ulikunitz/xz/commit/69c6093c7b2397b923acf82cb378f55ab2652b9b (similar to/same as CVE-2020-16845) - https://github.com/ulikunitz/xz/issues/40 -- no CVE, but could also lead to a DoS situation, I guess Given the limited impact of this issue (it could leave podman hanging, leading to a DoS situation in some scenarios), the absence of any unit tests, and the fact that we'd need to rebuild podman and friends anyways, I'm pondering whether making this change is worth the risk. Moritz, what do you think? If we decided to proceed, the debdiff would look like this: diff --git a/debian/changelog b/debian/changelog index 837efeeb1..ad17e4867 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,10 @@ +golang-github-containers-storage (1.24.8+dfsg1-2) unstable; urgency=high + + * Build against system copy of golang-github-ulikunitz-xz-dev, + Adresses: CVE-2021-20291, Closes: #988942 + + -- Reinhard Tartler <siret...@tauware.de> Fri, 21 May 2021 16:04:46 -0400 + golang-github-containers-storage (1.24.8+dfsg1-1) unstable; urgency=medium * New upstream release, focused on targetted bugfixes for podman 3.0 diff --git a/debian/control b/debian/control index 086dbcb3d..c5c362961 100644 --- a/debian/control +++ b/debian/control @@ -24,6 +24,7 @@ Build-Depends: debhelper-compat (= 11), golang-github-pquerna-ffjson-dev, golang-github-sirupsen-logrus-dev, golang-github-stretchr-testify-dev, + golang-github-ulikunitz-xz-dev, golang-github-vbatts-tar-split-dev, golang-go (>> 2:1.14~~), golang-go-patricia-dev, diff --git a/debian/patches/CVE-2021-20291.patch b/debian/patches/CVE-2021-20291.patch new file mode 100644 index 000000000..f87427443 --- /dev/null +++ b/debian/patches/CVE-2021-20291.patch @@ -0,0 +1,212 @@ +From 306fcabc964470e4b3b87a43a8f6b7d698209ee1 Mon Sep 17 00:00:00 2001 +From: Nalin Dahyabhai <na...@redhat.com> +Date: Wed, 17 Mar 2021 17:24:14 -0400 +Subject: [PATCH] Use an xz library instead of shelling out to xz for + decompression + +When decompressing layers compressed with xz, use a library rather than +shelling out to the xz CLI. + +Signed-off-by: Nalin Dahyabhai <na...@redhat.com> + +--- a/go.mod ++++ b/go.mod +@@ -23,6 +23,7 @@ + github.com/stretchr/testify v1.6.1 + github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2 + github.com/tchap/go-patricia v2.3.0+incompatible ++ github.com/ulikunitz/xz v0.5.10 + github.com/vbatts/tar-split v0.11.1 + golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 + golang.org/x/sys v0.0.0-20201201145000-ef89a241ccb3 +--- a/pkg/archive/archive.go ++++ b/pkg/archive/archive.go +@@ -9,7 +9,6 @@ + "io" + "io/ioutil" + "os" +- "os/exec" + "path/filepath" + "runtime" + "strings" +@@ -18,7 +17,6 @@ + + "github.com/containers/storage/pkg/fileutils" + "github.com/containers/storage/pkg/idtools" +- "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/pools" + "github.com/containers/storage/pkg/promise" + "github.com/containers/storage/pkg/system" +@@ -26,6 +24,7 @@ + rsystem "github.com/opencontainers/runc/libcontainer/system" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" ++ "github.com/ulikunitz/xz" + ) + + type ( +@@ -167,12 +166,6 @@ + return Uncompressed + } + +-func xzDecompress(archive io.Reader) (io.ReadCloser, <-chan struct{}, error) { +- args := []string{"xz", "-d", "-c", "-q"} +- +- return cmdStream(exec.Command(args[0], args[1:]...), archive) +-} +- + // DecompressStream decompresses the archive and returns a ReaderCloser with the decompressed archive. + func DecompressStream(archive io.Reader) (io.ReadCloser, error) { + p := pools.BufioReader32KPool +@@ -205,15 +198,12 @@ + readBufWrapper := p.NewReadCloserWrapper(buf, bz2Reader) + return readBufWrapper, nil + case Xz: +- xzReader, chdone, err := xzDecompress(buf) ++ xzReader, err := xz.NewReader(buf) + if err != nil { + return nil, err + } + readBufWrapper := p.NewReadCloserWrapper(buf, xzReader) +- return ioutils.NewReadCloserWrapper(readBufWrapper, func() error { +- <-chdone +- return readBufWrapper.Close() +- }), nil ++ return readBufWrapper, nil + case Zstd: + return zstdReader(buf) + default: +@@ -1306,35 +1296,6 @@ + return nil + } + +-// cmdStream executes a command, and returns its stdout as a stream. +-// If the command fails to run or doesn't complete successfully, an error +-// will be returned, including anything written on stderr. +-func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, <-chan struct{}, error) { +- chdone := make(chan struct{}) +- cmd.Stdin = input +- pipeR, pipeW := io.Pipe() +- cmd.Stdout = pipeW +- var errBuf bytes.Buffer +- cmd.Stderr = &errBuf +- +- // Run the command and return the pipe +- if err := cmd.Start(); err != nil { +- return nil, nil, err +- } +- +- // Copy stdout to the returned pipe +- go func() { +- if err := cmd.Wait(); err != nil { +- pipeW.CloseWithError(fmt.Errorf("%s: %s", err, errBuf.String())) +- } else { +- pipeW.Close() +- } +- close(chdone) +- }() +- +- return pipeR, chdone, nil +-} +- + // NewTempArchive reads the content of src into a temporary file, and returns the contents + // of that file as an archive. The archive can only be read once - as soon as reading completes, + // the file will be deleted. +--- a/pkg/archive/archive_test.go ++++ b/pkg/archive/archive_test.go +@@ -12,7 +12,6 @@ + "runtime" + "strings" + "testing" +- "time" + + "github.com/containers/storage/pkg/idtools" + "github.com/stretchr/testify/assert" +@@ -209,59 +208,6 @@ + } + } + +-func TestCmdStreamLargeStderr(t *testing.T) { +- cmd := exec.Command("sh", "-c", "dd if=/dev/zero bs=1k count=1000 of=/dev/stderr; echo hello") +- out, _, err := cmdStream(cmd, nil) +- if err != nil { +- t.Fatalf("Failed to start command: %s", err) +- } +- errCh := make(chan error) +- go func() { +- _, err := io.Copy(ioutil.Discard, out) +- errCh <- err +- }() +- select { +- case err := <-errCh: +- if err != nil { +- t.Fatalf("Command should not have failed (err=%.100s...)", err) +- } +- case <-time.After(5 * time.Second): +- t.Fatalf("Command did not complete in 5 seconds; probable deadlock") +- } +-} +- +-func TestCmdStreamBad(t *testing.T) { +- // TODO Windows: Figure out why this is failing in CI but not locally +- if runtime.GOOS == windows { +- t.Skip("Failing on Windows CI machines") +- } +- badCmd := exec.Command("sh", "-c", "echo hello; echo >&2 error couldn\\'t reverse the phase pulser; exit 1") +- out, _, err := cmdStream(badCmd, nil) +- if err != nil { +- t.Fatalf("Failed to start command: %s", err) +- } +- if output, err := ioutil.ReadAll(out); err == nil { +- t.Fatalf("Command should have failed") +- } else if err.Error() != "exit status 1: error couldn't reverse the phase pulser\n" { +- t.Fatalf("Wrong error value (%s)", err) +- } else if s := string(output); s != "hello\n" { +- t.Fatalf("Command output should be '%s', not '%s'", "hello\\n", output) +- } +-} +- +-func TestCmdStreamGood(t *testing.T) { +- cmd := exec.Command("sh", "-c", "echo hello; exit 0") +- out, _, err := cmdStream(cmd, nil) +- if err != nil { +- t.Fatal(err) +- } +- if output, err := ioutil.ReadAll(out); err != nil { +- t.Fatalf("Command should not have failed (err=%s)", err) +- } else if s := string(output); s != "hello\n" { +- t.Fatalf("Command output should be '%s', not '%s'", "hello\\n", output) +- } +-} +- + func TestUntarPathWithInvalidDest(t *testing.T) { + tempFolder, err := ioutil.TempDir("", "storage-archive-test") + require.NoError(t, err) +--- /dev/null ++++ b/tests/apply-junk.bats +@@ -0,0 +1,25 @@ ++#!/usr/bin/env bats ++ ++load helpers ++ ++@test "applyjunk" { ++ # Create and try to populate layers with... garbage. It should be ++ # rejected cleanly. ++ for compressed in cat gzip bzip2 xz zstd ; do ++ storage create-layer --id layer-${compressed} ++ ++ echo [[${compressed} /etc/os-release]] ++ ${compressed} < /etc/os-release > junkfile ++ run storage apply-diff --file junkfile layer-${compressed} ++ echo "$output" ++ [[ "$status" -ne 0 ]] ++ [[ "$output" =~ "invalid tar header" ]] || [[ "$output" =~ "unexpected EOF" ]] ++ ++ echo [[${compressed}]] ++ echo "sorry, not even enough info for a tar header here" | ${compressed} > junkfile ++ run storage apply-diff --file junkfile layer-${compressed} ++ echo "$output" ++ [[ "$status" -ne 0 ]] ++ [[ "$output" =~ "unexpected EOF" ]] ++ done ++} diff --git a/debian/patches/series b/debian/patches/series index d802103b9..2b57c2636 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1 +1,2 @@ test.patch +CVE-2021-20291.patch regards, Reinhard
