Skip to content

Commit

Permalink
Forbid symlinks to files outside the registry during transfer.
Browse files Browse the repository at this point in the history
Malicious users could create dangling symlinks to files that they can't
read but the Gobbler-owning account can; these files would then be
copied into a world-readable registry upon upload.
  • Loading branch information
LTLA committed Feb 15, 2024
1 parent 30b3d0a commit b30e596
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 33 deletions.
35 changes: 17 additions & 18 deletions transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ func Transfer(source, registry, project, asset, version string) error {
}
insize := restat.Size()

// Symlinks to files inside the destination directory are preserved.
// Symlinks to files inside the registry are preserved.
if restat.Mode() & os.ModeSymlink == os.ModeSymlink {
target, err := os.Readlink(path)
if err != nil {
Expand All @@ -284,28 +284,27 @@ func Transfer(source, registry, project, asset, version string) error {
if err != nil {
return fmt.Errorf("failed to stat link target %q; %w", target, err)
}
if tstat.IsDir() {
return fmt.Errorf("symbolic links to directories are not supported (%q); %w", target, err)
}

inside, err := filepath.Rel(registry, target)
if err == nil && !strings.HasPrefix(inside, "../") {
obj, err := resolveSymlink(registry, project, asset, version, inside, manifest_cache, summary_cache)
if err != nil {
return fmt.Errorf("failed to resolve the symlink at '" + path + "'; %w", err)
}
manifest[rel] = *obj
if err != nil || strings.HasPrefix(inside, "../") {
return fmt.Errorf("symbolic links to files outside the registry (%q) are not supported", target)
}
if tstat.IsDir() {
return fmt.Errorf("symbolic links to directories (%q) are not supported", target)
}

err = createRelativeSymlink(inside, rel, final)
if err != nil {
return fmt.Errorf("failed to create a symlink for '" + rel + "'; %w", err)
}
addLink(rel, obj.Link)
return nil
obj, err := resolveSymlink(registry, project, asset, version, inside, manifest_cache, summary_cache)
if err != nil {
return fmt.Errorf("failed to resolve the symlink at '" + path + "'; %w", err)
}
manifest[rel] = *obj

// Otherwise we need to compute the actual size.
insize = tstat.Size()
err = createRelativeSymlink(inside, rel, final)
if err != nil {
return fmt.Errorf("failed to create a symlink for '" + rel + "'; %w", err)
}
addLink(rel, obj.Link)
return nil
}

insum, err := computeChecksum(path)
Expand Down
19 changes: 4 additions & 15 deletions transfer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ func TestTransferLinkFailures(t *testing.T) {
t.Fatalf("failed to create the registry; %v", err)
}

// Links to irrelevant files are copied.
// Links to irrelevant files are forbidden.
{
src, err := setupSourceForTransferTest()
if err != nil {
Expand Down Expand Up @@ -674,19 +674,8 @@ func TestTransferLinkFailures(t *testing.T) {
asset := "PIKAPIKA"
version := "SILVER"
err = Transfer(src, reg, project, asset, version)
if err != nil {
t.Fatal(err)
}

destination := filepath.Join(reg, project, asset, version)
man, err := readManifest(destination)
if err != nil {
t.Fatalf("failed to read the manifest; %v", err)
}

err = verifyNotSymlink(man, destination, "asdasd", "gotta catch em all")
if err != nil {
t.Fatal(err)
if err == nil || !strings.Contains(err.Error(), "outside the registry") {
t.Fatal("expected transfer failure for files outside the registry")
}
}

Expand Down Expand Up @@ -813,7 +802,7 @@ func TestTransferLinkFailures(t *testing.T) {
t.Fatalf("failed to create the temporary directory; %v", err)
}

mock, err := os.MkdirTemp("", "")
mock, err := os.MkdirTemp(reg, "")
if err != nil {
t.Fatalf("failed to create the temporary directory as a link target; %v", err)
}
Expand Down

0 comments on commit b30e596

Please sign in to comment.