Merge pull request #15 from git-pkgs/validate-metadata-urls

Reject non-https download URLs from registry metadata

Author: andrew

fetch/resolver.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
 	"strings"
 	"unicode"
 
@@ -14,6 +15,7 @@ import (
 var (
 	ErrUnsupportedEcosystem = errors.New("unsupported ecosystem")
 	ErrNoDownloadURL        = errors.New("no download URL available")
+	ErrUnsafeURL            = errors.New("unsafe download URL from registry metadata")
 )
 
 // Registry provides package metadata and URL information for artifact resolution.
@@ -140,21 +142,15 @@ func (r *Resolver) resolveFromMetadata(ctx context.Context, reg Registry, name,
 			continue
 		}
 
-		// Look for download URL in metadata
+		// Look for download URL in metadata. These come from the
+		// registry's API response, not from us, so they need checking
+		// before anyone fetches them.
 		if v.Metadata != nil {
-			if url, ok := v.Metadata["download_url"].(string); ok && url != "" {
-				return &ArtifactInfo{
-					URL:       url,
-					Filename:  filenameFromURL(url),
-					Integrity: v.Integrity,
-				}, nil
+			if u, ok := v.Metadata["download_url"].(string); ok && u != "" {
+				return artifactFromMetadataURL(u, v.Integrity)
 			}
-			if url, ok := v.Metadata["tarball"].(string); ok && url != "" {
-				return &ArtifactInfo{
-					URL:       url,
-					Filename:  filenameFromURL(url),
-					Integrity: v.Integrity,
-				}, nil
+			if u, ok := v.Metadata["tarball"].(string); ok && u != "" {
+				return artifactFromMetadataURL(u, v.Integrity)
 			}
 		}
 
@@ -164,6 +160,35 @@ func (r *Resolver) resolveFromMetadata(ctx context.Context, reg Registry, name,
 	return nil, ErrNotFound
 }
 
+func artifactFromMetadataURL(raw, integrity string) (*ArtifactInfo, error) {
+	if err := checkMetadataURL(raw); err != nil {
+		return nil, err
+	}
+	return &ArtifactInfo{
+		URL:       raw,
+		Filename:  filenameFromURL(raw),
+		Integrity: integrity,
+	}, nil
+}
+
+// checkMetadataURL rejects download URLs from registry responses that
+// could direct the fetcher somewhere it shouldn't go. A compromised or
+// MITM'd registry could otherwise hand back file:// or a cloud metadata
+// endpoint.
+func checkMetadataURL(raw string) error {
+	u, err := url.Parse(raw)
+	if err != nil {
+		return fmt.Errorf("%w: %v", ErrUnsafeURL, err)
+	}
+	if u.Scheme != "https" {
+		return fmt.Errorf("%w: scheme %q", ErrUnsafeURL, u.Scheme)
+	}
+	if u.Hostname() == "" {
+		return fmt.Errorf("%w: empty host", ErrUnsafeURL)
+	}
+	return nil
+}
+
 func filenameFromURL(url string) string {
 	if idx := strings.LastIndex(url, "/"); idx >= 0 {
 		return url[idx+1:]

fetch/resolver_test.go

@@ -2,7 +2,11 @@ package fetch
 
 import (
 	"context"
+	"errors"
 	"testing"
+
+	"github.com/git-pkgs/registries"
+	"github.com/git-pkgs/registries/client"
 )
 
 func TestResolveWithoutRegistry(t *testing.T) {
@@ -118,6 +122,111 @@ func TestEncodeGoModule(t *testing.T) {
 	}
 }
 
+func TestCheckMetadataURL(t *testing.T) {
+	tests := []struct {
+		url string
+		ok  bool
+	}{
+		{"https://files.pythonhosted.org/packages/ab/cd/pkg-1.0.tar.gz", true},
+		{"https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz", true},
+		{"https://github.com/owner/repo/archive/v1.0.tar.gz", true},
+
+		{"http://files.pythonhosted.org/pkg.tar.gz", false},
+		{"file:///etc/passwd", false},
+		{"file://etc/passwd", false},
+		{"gopher://evil.example/", false},
+		{"ftp://ftp.example.com/pkg.tar.gz", false},
+		{"javascript:alert(1)", false},
+		{"https://", false},
+		{"https:///path/only", false},
+		{"//169.254.169.254/latest/meta-data/", false},
+		{"169.254.169.254/latest/meta-data/", false},
+		{"", false},
+		{"\x00https://evil", false},
+	}
+
+	for _, tt := range tests {
+		err := checkMetadataURL(tt.url)
+		if tt.ok && err != nil {
+			t.Errorf("checkMetadataURL(%q) = %v, want nil", tt.url, err)
+		}
+		if !tt.ok {
+			if err == nil {
+				t.Errorf("checkMetadataURL(%q) = nil, want error", tt.url)
+			} else if !errors.Is(err, ErrUnsafeURL) {
+				t.Errorf("checkMetadataURL(%q) error not ErrUnsafeURL: %v", tt.url, err)
+			}
+		}
+	}
+}
+
+type fakeRegistry struct {
+	versions []registries.Version
+}
+
+func (f *fakeRegistry) Ecosystem() string       { return "fake" }
+func (f *fakeRegistry) URLs() client.URLBuilder { return &client.BaseURLs{} } //nolint:ireturn // satisfying Registry interface
+func (f *fakeRegistry) FetchVersions(ctx context.Context, name string) ([]registries.Version, error) {
+	return f.versions, nil
+}
+
+func TestResolveFromMetadataRejectsUnsafeURL(t *testing.T) {
+	tests := []struct {
+		name  string
+		field string
+		url   string
+	}{
+		{"file scheme via download_url", "download_url", "file:///etc/passwd"},
+		{"http via tarball", "tarball", "http://169.254.169.254/latest/meta-data/"},
+		{"empty host via download_url", "download_url", "https:///just/a/path"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			reg := &fakeRegistry{
+				versions: []registries.Version{{
+					Number:   "1.0.0",
+					Metadata: map[string]any{tt.field: tt.url},
+				}},
+			}
+			r := NewResolver()
+			r.RegisterRegistry(reg)
+
+			info, err := r.Resolve(context.Background(), "fake", "pkg", "1.0.0")
+			if !errors.Is(err, ErrUnsafeURL) {
+				t.Fatalf("Resolve = (%v, %v), want ErrUnsafeURL", info, err)
+			}
+		})
+	}
+}
+
+func TestResolveFromMetadataAcceptsSafeURL(t *testing.T) {
+	want := "https://files.pythonhosted.org/packages/ab/cd/pkg-1.0.tar.gz"
+	reg := &fakeRegistry{
+		versions: []registries.Version{{
+			Number:    "1.0.0",
+			Integrity: "sha256-deadbeef",
+			Metadata:  map[string]any{"download_url": want},
+		}},
+	}
+	r := NewResolver()
+	r.RegisterRegistry(reg)
+
+	info, err := r.Resolve(context.Background(), "fake", "pkg", "1.0.0")
+	if err != nil {
+		t.Fatalf("Resolve failed: %v", err)
+	}
+	if info.URL != want {
+		t.Errorf("URL = %q, want %q", info.URL, want)
+	}
+	if info.Integrity != "sha256-deadbeef" {
+		t.Errorf("Integrity = %q, want sha256-deadbeef", info.Integrity)
+	}
+	if info.Filename != "pkg-1.0.tar.gz" {
+		t.Errorf("Filename = %q, want pkg-1.0.tar.gz", info.Filename)
+	}
+}
+
 func TestFilenameFromURL(t *testing.T) {
 	tests := []struct {
 		url  string