CVE-2021-20291

Debian Bug report logs - #988942
CVE-2021-20291

Reply or subscribe to this bug.

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Moritz Muehlenhoff <jmm@debian.org>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: CVE-2021-20291

Date: Fri, 21 May 2021 21:27:36 +0200

Package: golang-github-containers-image
Severity: important
Tags: security
X-Debbugs-Cc: Debian Security Team <team@security.debian.org>

This was assigned CVE-2021-20291:
https://github.com/containers/storage/commit/306fcabc964470e4b3b87a43a8f6b7d698209ee1

Cheers,
	 Moritz

Message #10 received at 988942@bugs.debian.org (full text, mbox, reply):

From: Reinhard Tartler <siretart@gmail.com>

To: Moritz Muehlenhoff <jmm@debian.org>, 988942@bugs.debian.org

Subject: Re: Bug#988942: CVE-2021-20291

Date: Fri, 21 May 2021 16:31:20 -0400

[Message part 1 (text/plain, inline)]

On Fri, May 21, 2021 at 3:30 PM Moritz Muehlenhoff <jmm@debian.org> wrote:

> Package: golang-github-containers-image
> Severity: important
> Tags: security
> X-Debbugs-Cc: Debian Security Team <team@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 <siretart@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 <nalin@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 <nalin@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

[Message part 2 (text/html, inline)]

Send a report that this bug log contains spam.