Commit 30230989 authored by Arnaud Rebillout's avatar Arnaud Rebillout

Revert cve patches to their original version, add debian patch to skip privileged tests

The cve-2018-15664-* patches come from upstream, but the tests units
were removed, as they require root.

I find it a bit misleading to have modified version of upstream patches,
especially if there's no message in the patch header indicating that the
patches were modified.

This commit brings the patches back to their original form, identical
to upstream, and introduce a third patch to disable the tests if user is
not root.
Signed-off-by: Arnaud Rebillout's avatarArnaud Rebillout <arnaud.rebillout@collabora.com>
parent 6983fb5b
...@@ -15,8 +15,10 @@ Origin: upstream, https://github.com/moby/moby/pull/39292 ...@@ -15,8 +15,10 @@ Origin: upstream, https://github.com/moby/moby/pull/39292
daemon/archive.go | 7 ++- daemon/archive.go | 7 ++-
pkg/chrootarchive/archive.go | 24 ++++++-- pkg/chrootarchive/archive.go | 24 ++++++--
pkg/chrootarchive/archive_unix.go | 34 ++++++++++-- pkg/chrootarchive/archive_unix.go | 34 ++++++++++--
pkg/chrootarchive/archive_unix_test.go | 77 ++++++++++++++++++++++++++
pkg/chrootarchive/archive_windows.go | 2 +- pkg/chrootarchive/archive_windows.go | 2 +-
5 files changed, 55 insertions(+), 12 deletions(-) 5 files changed, 132 insertions(+), 12 deletions(-)
create mode 100644 pkg/chrootarchive/archive_unix_test.go
diff --git a/engine/daemon/archive.go b/engine/daemon/archive.go diff --git a/engine/daemon/archive.go b/engine/daemon/archive.go
index 9c7971b56ea3..9f56ca750392 100644 index 9c7971b56ea3..9f56ca750392 100644
...@@ -171,6 +173,89 @@ index 5df8afd66205..96f07c4bb4d6 100644 ...@@ -171,6 +173,89 @@ index 5df8afd66205..96f07c4bb4d6 100644
//write the options to the pipe for the untar exec to read //write the options to the pipe for the untar exec to read
if err := json.NewEncoder(w).Encode(options); err != nil { if err := json.NewEncoder(w).Encode(options); err != nil {
w.Close() w.Close()
diff --git a/engine/pkg/chrootarchive/archive_unix_test.go b/engine/pkg/chrootarchive/archive_unix_test.go
new file mode 100644
index 000000000000..d81c19048179
--- /dev/null
+++ b/engine/pkg/chrootarchive/archive_unix_test.go
@@ -0,0 +1,77 @@
+// +build !windows
+
+package chrootarchive
+
+import (
+ "bytes"
+ "io"
+ "io/ioutil"
+ "os"
+ "path/filepath"
+ "testing"
+
+ "github.com/docker/docker/pkg/archive"
+ "golang.org/x/sys/unix"
+ "gotest.tools/assert"
+)
+
+// Test for CVE-2018-15664
+// Assures that in the case where an "attacker" controlled path is a symlink to
+// some path outside of a container's rootfs that we do not copy data to a
+// container path that will actually overwrite data on the host
+func TestUntarWithMaliciousSymlinks(t *testing.T) {
+ dir, err := ioutil.TempDir("", t.Name())
+ assert.NilError(t, err)
+ defer os.RemoveAll(dir)
+
+ root := filepath.Join(dir, "root")
+
+ err = os.MkdirAll(root, 0755)
+ assert.NilError(t, err)
+
+ // Add a file into a directory above root
+ // Ensure that we can't access this file while tarring.
+ err = ioutil.WriteFile(filepath.Join(dir, "host-file"), []byte("I am a host file"), 0644)
+ assert.NilError(t, err)
+
+ // Create some data which which will be copied into the "container" root into
+ // the symlinked path.
+ // Before this change, the copy would overwrite the "host" content.
+ // With this change it should not.
+ data := filepath.Join(dir, "data")
+ err = os.MkdirAll(data, 0755)
+ assert.NilError(t, err)
+ err = ioutil.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0644)
+ assert.NilError(t, err)
+
+ safe := filepath.Join(root, "safe")
+ err = unix.Symlink(dir, safe)
+ assert.NilError(t, err)
+
+ rdr, err := archive.TarWithOptions(data, &archive.TarOptions{IncludeFiles: []string{"local-file"}, RebaseNames: map[string]string{"local-file": "host-file"}})
+ assert.NilError(t, err)
+
+ // Use tee to test both the good case and the bad case w/o recreating the archive
+ bufRdr := bytes.NewBuffer(nil)
+ tee := io.TeeReader(rdr, bufRdr)
+
+ err = UntarWithRoot(tee, safe, nil, root)
+ assert.Assert(t, err != nil)
+ assert.ErrorContains(t, err, "open /safe/host-file: no such file or directory")
+
+ // Make sure the "host" file is still in tact
+ // Before the fix the host file would be overwritten
+ hostData, err := ioutil.ReadFile(filepath.Join(dir, "host-file"))
+ assert.NilError(t, err)
+ assert.Equal(t, string(hostData), "I am a host file")
+
+ // Now test by chrooting to an attacker controlled path
+ // This should succeed as is and overwrite a "host" file
+ // Note that this would be a mis-use of this function.
+ err = UntarWithRoot(bufRdr, safe, nil, safe)
+ assert.NilError(t, err)
+
+ hostData, err = ioutil.ReadFile(filepath.Join(dir, "host-file"))
+ assert.NilError(t, err)
+ assert.Equal(t, string(hostData), "pwn3d")
+}
diff --git a/engine/pkg/chrootarchive/archive_windows.go b/engine/pkg/chrootarchive/archive_windows.go diff --git a/engine/pkg/chrootarchive/archive_windows.go b/engine/pkg/chrootarchive/archive_windows.go
index f2973132a391..bd5712c5c04c 100644 index f2973132a391..bd5712c5c04c 100644
--- a/engine/pkg/chrootarchive/archive_windows.go --- a/engine/pkg/chrootarchive/archive_windows.go
......
...@@ -13,9 +13,10 @@ Origin: upstream, https://github.com/moby/moby/pull/39292 ...@@ -13,9 +13,10 @@ Origin: upstream, https://github.com/moby/moby/pull/39292
daemon/export.go | 2 +- daemon/export.go | 2 +-
pkg/chrootarchive/archive.go | 8 +++ pkg/chrootarchive/archive.go | 8 +++
pkg/chrootarchive/archive_unix.go | 98 +++++++++++++++++++++++++- pkg/chrootarchive/archive_unix.go | 98 +++++++++++++++++++++++++-
pkg/chrootarchive/archive_unix_test.go | 94 ++++++++++++++++++++++++
pkg/chrootarchive/archive_windows.go | 7 ++ pkg/chrootarchive/archive_windows.go | 7 ++
pkg/chrootarchive/init_unix.go | 1 + pkg/chrootarchive/init_unix.go | 1 +
6 files changed, 117 insertions(+), 7 deletions(-) 7 files changed, 211 insertions(+), 7 deletions(-)
diff --git a/engine/daemon/archive.go b/engine/daemon/archive.go diff --git a/engine/daemon/archive.go b/engine/daemon/archive.go
index 9f56ca750392..109376b4b566 100644 index 9f56ca750392..109376b4b566 100644
...@@ -219,6 +220,120 @@ index 96f07c4bb4d6..ea2879dc002f 100644 ...@@ -219,6 +220,120 @@ index 96f07c4bb4d6..ea2879dc002f 100644
+ +
+ return tarR, nil + return tarR, nil
+} +}
diff --git a/engine/pkg/chrootarchive/archive_unix_test.go b/engine/pkg/chrootarchive/archive_unix_test.go
index d81c19048179..f39a88ad3814 100644
--- a/engine/pkg/chrootarchive/archive_unix_test.go
+++ b/engine/pkg/chrootarchive/archive_unix_test.go
@@ -3,11 +3,14 @@
package chrootarchive
import (
+ gotar "archive/tar"
"bytes"
"io"
"io/ioutil"
"os"
+ "path"
"path/filepath"
+ "strings"
"testing"
"github.com/docker/docker/pkg/archive"
@@ -75,3 +78,94 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) {
assert.NilError(t, err)
assert.Equal(t, string(hostData), "pwn3d")
}
+
+// Test for CVE-2018-15664
+// Assures that in the case where an "attacker" controlled path is a symlink to
+// some path outside of a container's rootfs that we do not unwittingly leak
+// host data into the archive.
+func TestTarWithMaliciousSymlinks(t *testing.T) {
+ dir, err := ioutil.TempDir("", t.Name())
+ assert.NilError(t, err)
+ // defer os.RemoveAll(dir)
+ t.Log(dir)
+
+ root := filepath.Join(dir, "root")
+
+ err = os.MkdirAll(root, 0755)
+ assert.NilError(t, err)
+
+ hostFileData := []byte("I am a host file")
+
+ // Add a file into a directory above root
+ // Ensure that we can't access this file while tarring.
+ err = ioutil.WriteFile(filepath.Join(dir, "host-file"), hostFileData, 0644)
+ assert.NilError(t, err)
+
+ safe := filepath.Join(root, "safe")
+ err = unix.Symlink(dir, safe)
+ assert.NilError(t, err)
+
+ data := filepath.Join(dir, "data")
+ err = os.MkdirAll(data, 0755)
+ assert.NilError(t, err)
+
+ type testCase struct {
+ p string
+ includes []string
+ }
+
+ cases := []testCase{
+ {p: safe, includes: []string{"host-file"}},
+ {p: safe + "/", includes: []string{"host-file"}},
+ {p: safe, includes: nil},
+ {p: safe + "/", includes: nil},
+ {p: root, includes: []string{"safe/host-file"}},
+ {p: root, includes: []string{"/safe/host-file"}},
+ {p: root, includes: nil},
+ }
+
+ maxBytes := len(hostFileData)
+
+ for _, tc := range cases {
+ t.Run(path.Join(tc.p+"_"+strings.Join(tc.includes, "_")), func(t *testing.T) {
+ // Here if we use archive.TarWithOptions directly or change the "root" parameter
+ // to be the same as "safe", data from the host will be leaked into the archive
+ var opts *archive.TarOptions
+ if tc.includes != nil {
+ opts = &archive.TarOptions{
+ IncludeFiles: tc.includes,
+ }
+ }
+ rdr, err := Tar(tc.p, opts, root)
+ assert.NilError(t, err)
+ defer rdr.Close()
+
+ tr := gotar.NewReader(rdr)
+ assert.Assert(t, !isDataInTar(t, tr, hostFileData, int64(maxBytes)), "host data leaked to archive")
+ })
+ }
+}
+
+func isDataInTar(t *testing.T, tr *gotar.Reader, compare []byte, maxBytes int64) bool {
+ for {
+ h, err := tr.Next()
+ if err == io.EOF {
+ break
+ }
+ assert.NilError(t, err)
+
+ if h.Size == 0 {
+ continue
+ }
+ assert.Assert(t, h.Size <= maxBytes, "%s: file size exceeds max expected size %d: %d", h.Name, maxBytes, h.Size)
+
+ data := make([]byte, int(h.Size))
+ _, err = io.ReadFull(tr, data)
+ assert.NilError(t, err)
+ if bytes.Contains(data, compare) {
+ return true
+ }
+ }
+
+ return false
+}
diff --git a/engine/pkg/chrootarchive/archive_windows.go b/engine/pkg/chrootarchive/archive_windows.go diff --git a/engine/pkg/chrootarchive/archive_windows.go b/engine/pkg/chrootarchive/archive_windows.go
index bd5712c5c04c..de87113e9544 100644 index bd5712c5c04c..de87113e9544 100644
--- a/engine/pkg/chrootarchive/archive_windows.go --- a/engine/pkg/chrootarchive/archive_windows.go
......
From: Arnaud Rebillout <arnaud.rebillout@collabora.com>
Subject: Disable privileged tests for cve-2018-15664
Origin: vendor, Debian
Forwarded: not-needed, Debian-specific
--- a/engine/pkg/chrootarchive/archive_unix_test.go
+++ b/engine/pkg/chrootarchive/archive_unix_test.go
@@ -15,15 +15,17 @@
"github.com/docker/docker/pkg/archive"
"golang.org/x/sys/unix"
"gotest.tools/assert"
+ "gotest.tools/skip"
)
// Test for CVE-2018-15664
// Assures that in the case where an "attacker" controlled path is a symlink to
// some path outside of a container's rootfs that we do not copy data to a
// container path that will actually overwrite data on the host
func TestUntarWithMaliciousSymlinks(t *testing.T) {
+ skip.If(t, os.Getuid() != 0, "DM - skipping privileged test")
dir, err := ioutil.TempDir("", t.Name())
assert.NilError(t, err)
defer os.RemoveAll(dir)
@@ -83,8 +85,9 @@
// Assures that in the case where an "attacker" controlled path is a symlink to
// some path outside of a container's rootfs that we do not unwittingly leak
// host data into the archive.
func TestTarWithMaliciousSymlinks(t *testing.T) {
+ skip.If(t, os.Getuid() != 0, "DM - skipping privileged test")
dir, err := ioutil.TempDir("", t.Name())
assert.NilError(t, err)
// defer os.RemoveAll(dir)
t.Log(dir)
...@@ -14,6 +14,7 @@ cli-fix-registry-debug-message-go-1.11.patch ...@@ -14,6 +14,7 @@ cli-fix-registry-debug-message-go-1.11.patch
cve-2018-15664-01-pass-root-to-chroot-to-for-chroot-untar.patch cve-2018-15664-01-pass-root-to-chroot-to-for-chroot-untar.patch
cve-2018-15664-02-add-chroot-for-tar-packing-operations.patch cve-2018-15664-02-add-chroot-for-tar-packing-operations.patch
cve-2018-15664-03-debian-skip-privileged-tests.patch
engine-contrib-debootstrap-curl-follow-location.patch engine-contrib-debootstrap-curl-follow-location.patch
engine-test-noinstall.patch engine-test-noinstall.patch
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment