Merge pull request #5575 from dctrud/3.6.3-security-local

Merge tempdir security fixes to release-3.6

Author: dtrudg

Diff for: CHANGELOG.md

@@ -9,7 +9,31 @@ _With the release of `v3.0.0`, we're introducing a new changelog format in an at
 
 _The old changelog can be found in the `release-2.6` branch_
 
-# Changes since v3.6.2
+# v3.6.3 - [2020-09-15]
+
+## Security related fixes
+
+Singularity 3.6.3 addresses the following security issues.
+
+  - [CVE-2020-25039](https://github.com/hpcng/singularity/security/advisories/GHSA-w6v2-qchm-grj7):
+    When a Singularity action command (run, shell, exec) is run with
+    the fakeroot or user namespace option, Singularity will extract a
+    container image to a temporary sandbox directory. Due to insecure
+    permissions on the temporary directory it is possible for any user
+    with access to the system to read the contents of the
+    image. Additionally, if the image contains a world-writable file
+    or directory, it is possible for a user to inject arbitrary
+    content into the running container.
+
+  - [CVE-2020-25040](https://github.com/hpcng/singularity/security/advisories/GHSA-jv9c-w74q-6762):
+    When a Singularity command that results in a container build
+    operation is executed, it is possible for a user with access to
+    the system to read the contents of the image during the
+    build. Additionally, if the image contains a world-writable file
+    or directory, it is possible for a user to inject arbitrary
+    content into the running build, which in certain circumstances may
+    enable arbitrary code execution during the build and/or when the
+    built container is run.
 
 ## Bug Fixes
 

Diff for: INSTALL.md

@@ -89,7 +89,7 @@ $ mkdir -p ${GOPATH}/src/github.com/sylabs && \
 To build a stable version of Singularity, check out a [release tag](https://github.com/sylabs/singularity/tags) before compiling:
 
 ```
-$ git checkout v3.6.2
+$ git checkout v3.6.3
 ```
 
 ## Compiling Singularity
@@ -132,7 +132,7 @@ as shown above.  Then download the latest
 and use it to install the RPM like this: 
 
 ```
-$ export VERSION=3.6.2  # this is the singularity version, change as you need
+$ export VERSION=3.6.3  # this is the singularity version, change as you need
 
 $ wget https://github.com/sylabs/singularity/releases/download/v${VERSION}/singularity-${VERSION}.tar.gz && \
     rpmbuild -tb singularity-${VERSION}.tar.gz && \

Diff for: cmd/internal/cli/actions_linux.go

@@ -45,27 +45,30 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-func convertImage(filename string, unsquashfsPath string) (string, error) {
+// convertImage extracts the image found at filename to directory dir within a temporary directory
+// tempDir. If the unsquashfs binary is not located, the binary at unsquashfsPath is used. It is
+// the caller's responsibility to remove tempDir when no longer needed.
+func convertImage(filename string, unsquashfsPath string) (tempDir, imageDir string, err error) {
 	img, err := imgutil.Init(filename, false)
 	if err != nil {
-		return "", fmt.Errorf("could not open image %s: %s", filename, err)
+		return "", "", fmt.Errorf("could not open image %s: %s", filename, err)
 	}
 	defer img.File.Close()
 
 	part, err := img.GetRootFsPartition()
 	if err != nil {
-		return "", fmt.Errorf("while getting root filesystem in %s: %s", filename, err)
+		return "", "", fmt.Errorf("while getting root filesystem in %s: %s", filename, err)
 	}
 
 	// squashfs only
 	if part.Type != imgutil.SQUASHFS {
-		return "", fmt.Errorf("not a squashfs root filesystem")
+		return "", "", fmt.Errorf("not a squashfs root filesystem")
 	}
 
 	// create a reader for rootfs partition
 	reader, err := imgutil.NewPartitionReader(img, "", 0)
 	if err != nil {
-		return "", fmt.Errorf("could not extract root filesystem: %s", err)
+		return "", "", fmt.Errorf("could not extract root filesystem: %s", err)
 	}
 	s := unpacker.NewSquashfs()
 	if !s.HasUnsquashfs() && unsquashfsPath != "" {
@@ -82,18 +85,28 @@ func convertImage(filename string, unsquashfsPath string) (string, error) {
 	}
 
 	// create temporary sandbox
-	dir, err := ioutil.TempDir(tmpdir, "rootfs-")
+	tempDir, err = ioutil.TempDir(tmpdir, "rootfs-")
 	if err != nil {
-		return "", fmt.Errorf("could not create temporary sandbox: %s", err)
+		return "", "", fmt.Errorf("could not create temporary sandbox: %s", err)
+	}
+	defer func() {
+		if err != nil {
+			os.RemoveAll(tempDir)
+		}
+	}()
+
+	// create an inner dir to extract to, so we don't clobber the secure permissions on the tmpDir.
+	imageDir = filepath.Join(tempDir, "root")
+	if err := os.Mkdir(imageDir, 0755); err != nil {
+		return "", "", fmt.Errorf("could not create root directory: %s", err)
 	}
 
 	// extract root filesystem
-	if err := s.ExtractAll(reader, dir); err != nil {
-		os.RemoveAll(dir)
-		return "", fmt.Errorf("root filesystem extraction failed: %s", err)
+	if err := s.ExtractAll(reader, imageDir); err != nil {
+		return "", "", fmt.Errorf("root filesystem extraction failed: %s", err)
 	}
 
-	return dir, err
+	return tempDir, imageDir, err
 }
 
 // checkHidepid checks if hidepid is set on /proc mount point, when this
@@ -650,13 +663,13 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri
 			}
 			sylog.Verbosef("User namespace requested, convert image %s to sandbox", image)
 			sylog.Infof("Convert SIF file to sandbox...")
-			dir, err := convertImage(image, unsquashfsPath)
+			tempDir, imageDir, err := convertImage(image, unsquashfsPath)
 			if err != nil {
 				sylog.Fatalf("while extracting %s: %s", image, err)
 			}
-			engineConfig.SetImage(dir)
-			engineConfig.SetDeleteImage(true)
-			generator.AddProcessEnv("SINGULARITY_CONTAINER", dir)
+			engineConfig.SetImage(imageDir)
+			engineConfig.SetDeleteTempDir(tempDir)
+			generator.AddProcessEnv("SINGULARITY_CONTAINER", imageDir)
 
 			// if '--disable-cache' flag, then remove original SIF after converting to sandbox
 			if disableCache {

Diff for: internal/pkg/build/build.go

@@ -13,13 +13,13 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
+	"strings"
 	"syscall"
 
 	"github.com/sylabs/singularity/internal/pkg/util/fs"
 	"github.com/sylabs/singularity/pkg/util/fs/proc"
 	"github.com/sylabs/singularity/pkg/util/singularityconf"
 
-	uuid "github.com/satori/go.uuid"
 	"github.com/sylabs/singularity/internal/pkg/build/apps"
 	"github.com/sylabs/singularity/internal/pkg/build/assemblers"
 	"github.com/sylabs/singularity/internal/pkg/build/sources"
@@ -113,14 +113,16 @@ func newBuild(defs []types.Definition, conf Config) (*Build, error) {
 		if conf.Format == "sandbox" {
 			rootfsParent = filepath.Dir(conf.Dest)
 		}
-		rootfs := filepath.Join(rootfsParent, "rootfs-"+uuid.NewV1().String())
+		parentPath, err := ioutil.TempDir(rootfsParent, "build-temp-")
+		if err != nil {
+			return nil, fmt.Errorf("failed to create build parent dir: %w", err)
+		}
 
 		var s stage
-		var err error
 		if conf.Opts.EncryptionKeyInfo != nil {
-			s.b, err = types.NewEncryptedBundle(rootfs, conf.Opts.TmpDir, conf.Opts.EncryptionKeyInfo)
+			s.b, err = types.NewEncryptedBundle(parentPath, conf.Opts.TmpDir, conf.Opts.EncryptionKeyInfo)
 		} else {
-			s.b, err = types.NewBundle(rootfs, conf.Opts.TmpDir)
+			s.b, err = types.NewBundle(parentPath, conf.Opts.TmpDir)
 		}
 		if err != nil {
 			return nil, err
@@ -134,7 +136,7 @@ func newBuild(defs []types.Definition, conf Config) (*Build, error) {
 			// the old behavior which is to create the temporary rootfs inside
 			// $TMPDIR and copy the final root filesystem to the destination
 			// provided
-			if s.b.RootfsPath != rootfs {
+			if !strings.HasPrefix(s.b.RootfsPath, parentPath) {
 				sandboxCopy = true
 				sylog.Warningf("The underlying filesystem on which resides %q won't allow to set ownership, "+
 					"as a consequence the sandbox could not preserve image's files/directories ownerships", conf.Dest)

Diff for: internal/pkg/build/sources/conveyorPacker_oci.go

@@ -96,7 +96,7 @@ func (cp *OCIConveyorPacker) Get(ctx context.Context, b *sytypes.Bundle) (err er
 			cp.srcRef, err = ociarchive.ParseReference(ref)
 		} else {
 			// As non-root we need to do a dumb tar extraction first
-			tmpDir, err := ioutil.TempDir(cp.b.Opts.TmpDir, "temp-oci-")
+			tmpDir, err := ioutil.TempDir(b.TmpDir, "temp-oci-")
 			if err != nil {
 				return fmt.Errorf("could not create temporary oci directory: %v", err)
 			}

Diff for: internal/pkg/runtime/engine/singularity/cleanup_linux.go

@@ -50,9 +50,8 @@ func (e *EngineOperations) CleanupContainer(ctx context.Context, fatal error, st
 		}
 	}
 
-	if e.EngineConfig.GetDeleteImage() {
-		image := e.EngineConfig.GetImage()
-		sylog.Verbosef("Removing image %s", image)
+	if tempDir := e.EngineConfig.GetDeleteTempDir(); tempDir != "" {
+		sylog.Verbosef("Removing image tempDir %s", tempDir)
 		sylog.Infof("Cleaning up image...")
 
 		var err error
@@ -63,12 +62,12 @@ func (e *EngineOperations) CleanupContainer(ctx context.Context, fatal error, st
 			// context and can get permission denied error during
 			// image removal, so we execute "rm -rf /tmp/image" via
 			// the fakeroot engine
-			err = fakerootCleanup(image)
+			err = fakerootCleanup(tempDir)
 		} else {
-			err = os.RemoveAll(image)
+			err = os.RemoveAll(tempDir)
 		}
 		if err != nil {
-			sylog.Errorf("failed to delete container image %s: %s", image, err)
+			sylog.Errorf("failed to delete container image tempDir %s: %s", tempDir, err)
 		}
 	}
 

Diff for: pkg/build/types/bundle.go

@@ -30,6 +30,8 @@ type Bundle struct {
 
 	RootfsPath string `json:"rootfsPath"` // where actual fs to chroot will appear
 	TmpDir     string `json:"tmpPath"`    // where temp files required during build will appear
+
+	parentPath string // parent directory for RootfsPath
 }
 
 // Options defines build time behavior to be executed on the bundle.
@@ -73,13 +75,13 @@ type Options struct {
 }
 
 // NewEncryptedBundle creates an Encrypted Bundle environment.
-func NewEncryptedBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (b *Bundle, err error) {
-	return newBundle(rootfs, tempDir, keyInfo)
+func NewEncryptedBundle(parentPath, tempDir string, keyInfo *crypt.KeyInfo) (b *Bundle, err error) {
+	return newBundle(parentPath, tempDir, keyInfo)
 }
 
 // NewBundle creates a Bundle environment.
-func NewBundle(rootfs, tempDir string) (b *Bundle, err error) {
-	return newBundle(rootfs, tempDir, nil)
+func NewBundle(parentPath, tempDir string) (b *Bundle, err error) {
+	return newBundle(parentPath, tempDir, nil)
 }
 
 // RunSection iterates through the sections specified in a bundle
@@ -100,7 +102,7 @@ func (b *Bundle) RunSection(s string) bool {
 // Remove cleans up any bundle files.
 func (b *Bundle) Remove() error {
 	var errors []string
-	for _, dir := range []string{b.TmpDir, b.RootfsPath} {
+	for _, dir := range []string{b.TmpDir, b.parentPath} {
 		if err := fs.ForceRemoveAll(dir); err != nil {
 			errors = append(errors, fmt.Sprintf("could not remove %q: %v", dir, err))
 		}
@@ -141,11 +143,19 @@ func cleanupDir(path string) {
 	}
 }
 
-// newBundle creates a minimum bundle with root filesystem in rootfs.
+// newBundle creates a minimum bundle with root filesystem in parentPath.
 // Any temporary files created during build process will be in tempDir/bundle-temp-*
 // directory, that will be cleaned up after successful build.
-func newBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error) {
-	rootfsPath := rootfs
+
+//
+// TODO: much of the logic in this func should likely be re-factored to func newBuild in the
+// internal/pkg/build package, since it is the sole caller and has conditional logic which depends
+// on implementation details of this package. In particular, chown() handling should be done at the
+// build level, rather than the bundle level, to avoid repetition during multi-stage builds, and
+// clarify responsibility for cleanup of the various directories that are created during the build
+// process.
+func newBundle(parentPath, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error) {
+	rootfsPath := filepath.Join(parentPath, "rootfs")
 
 	tmpPath, err := ioutil.TempDir(tempDir, "bundle-temp-")
 	if err != nil {
@@ -155,6 +165,7 @@ func newBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error)
 
 	if err := os.MkdirAll(rootfsPath, 0755); err != nil {
 		cleanupDir(tmpPath)
+		cleanupDir(parentPath)
 		return nil, fmt.Errorf("could not create %q: %v", rootfsPath, err)
 	}
 
@@ -164,14 +175,25 @@ func newBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error)
 	if err != nil {
 		cleanupDir(tmpPath)
 		cleanupDir(rootfsPath)
+		cleanupDir(parentPath)
 		return nil, err
 	} else if !can {
-		defer cleanupDir(rootfsPath)
+		cleanupDir(rootfsPath)
+		cleanupDir(parentPath)
 
-		rootfsNewPath := filepath.Join(tempDir, filepath.Base(rootfsPath))
-		if rootfsNewPath != rootfsPath {
-			if err := os.MkdirAll(rootfsNewPath, 0755); err != nil {
+		// If the supplied rootfs was not inside tempDir (as is the case during a sandbox build),
+		// try tempDir as a fallback.
+		if !strings.HasPrefix(parentPath, tempDir) {
+			parentPath, err = ioutil.TempDir(tempDir, "build-temp-")
+			if err != nil {
+				cleanupDir(tmpPath)
+				return nil, fmt.Errorf("failed to create rootfs directory: %v", err)
+			}
+			// Create an inner dir, so we don't clobber the secure permissions on the surrounding dir.
+			rootfsNewPath := filepath.Join(parentPath, "rootfs")
+			if err := os.Mkdir(rootfsNewPath, 0755); err != nil {
 				cleanupDir(tmpPath)
+				cleanupDir(parentPath)
 				return nil, fmt.Errorf("could not create rootfs dir in %q: %v", rootfsNewPath, err)
 			}
 			// check that chown works with the underlying filesystem pointed
@@ -180,10 +202,12 @@ func newBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error)
 			if err != nil {
 				cleanupDir(tmpPath)
 				cleanupDir(rootfsNewPath)
+				cleanupDir(parentPath)
 				return nil, err
 			} else if !can {
 				cleanupDir(tmpPath)
 				cleanupDir(rootfsNewPath)
+				cleanupDir(parentPath)
 				sylog.Errorf("Could not set files/directories ownership, if %s is on a network filesystem, "+
 					"you must set TMPDIR to a local path (eg: TMPDIR=/var/tmp singularity build ...)", rootfsNewPath)
 				return nil, fmt.Errorf("ownership change not allowed in %s, aborting", tempDir)
@@ -195,6 +219,7 @@ func newBundle(rootfs, tempDir string, keyInfo *crypt.KeyInfo) (*Bundle, error)
 	sylog.Debugf("Created directory %q for the bundle", rootfsPath)
 
 	return &Bundle{
+		parentPath:  parentPath,
 		RootfsPath:  rootfsPath,
 		TmpDir:      tmpPath,
 		JSONObjects: make(map[string][]byte),

Diff for: pkg/runtime/engine/singularity/config/config.go

@@ -148,7 +148,7 @@ type JSONConfig struct {
 	NoPrivs           bool              `json:"noPrivs,omitempty"`
 	NoHome            bool              `json:"noHome,omitempty"`
 	NoInit            bool              `json:"noInit,omitempty"`
-	DeleteImage       bool              `json:"deleteImage,omitempty"`
+	DeleteTempDir     string            `json:"deleteTempDir,omitempty"`
 	Fakeroot          bool              `json:"fakeroot,omitempty"`
 	SignalPropagation bool              `json:"signalPropagation,omitempty"`
 }
@@ -711,14 +711,16 @@ func (e *EngineConfig) GetFakeroot() bool {
 	return e.JSON.Fakeroot
 }
 
-// GetDeleteImage returns if container image must be deleted after use.
-func (e *EngineConfig) GetDeleteImage() bool {
-	return e.JSON.DeleteImage
+// GetDeleteTempDir returns the path of the temporary directory containing the root filesystem
+// which must be deleted after use. If no deletion is required, the empty string is returned.
+func (e *EngineConfig) GetDeleteTempDir() string {
+	return e.JSON.DeleteTempDir
 }
 
-// SetDeleteImage sets if container image must be deleted after use.
-func (e *EngineConfig) SetDeleteImage(delete bool) {
-	e.JSON.DeleteImage = delete
+// SetDeleteTempDir sets dir as the path of the temporary directory containing the root filesystem,
+// which must be deleted after use.
+func (e *EngineConfig) SetDeleteTempDir(dir string) {
+	e.JSON.DeleteTempDir = dir
 }
 
 // SetSignalPropagation sets if engine must propagate signals from