From 34040bd0dfb389f25f686bcf8d9124662f2f755c Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sat, 10 Oct 2026 16:51:51 +0200 Subject: [PATCH] Fix false symlink loop detection This patch fixes valid symlink paths being mistaken for loops by [c51f0ed4](https://github.com/webmin/webmin/commit/c51f0ed4). It simplifies only the checked path prefix, preserving the unresolved remainder. Original loop cases still stop quickly, while long valid chains remain supported. I reproduced incorrect PHP selection in Virtualmin and WP Workbench, and GoAccess choosing its root log-creation branch instead of the domain-user helper. --- t/web-lib-funcs-resolve-links.t | 99 +++++++++++++++++++++++++++++++++ web-lib-funcs.pl | 9 ++- 2 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 t/web-lib-funcs-resolve-links.t diff --git a/t/web-lib-funcs-resolve-links.t b/t/web-lib-funcs-resolve-links.t new file mode 100644 index 000000000..d3fe9773d --- /dev/null +++ b/t/web-lib-funcs-resolve-links.t @@ -0,0 +1,99 @@ +#!/usr/bin/perl +# Check symlink resolution and cycle detection using isolated filesystem fixtures. + +use strict; +use warnings; +use Test::More; +use Cwd qw(abs_path); +use File::Basename qw(dirname basename); +use File::Path qw(make_path); +use File::Spec; +use File::Temp qw(tempdir); + +my $script = File::Spec->rel2abs( + File::Spec->catfile(dirname(__FILE__), '..', 'web-lib-funcs.pl')); +require $script; +my $root = abs_path(tempdir(CLEANUP => 1)); + +# resolve_counted(path) +# Returns the resolved path and call count, stopping runaway test recursion. +sub resolve_counted +{ +my ($path) = @_; +my $original = \&main::resolve_links; +my $calls = 0; +no warnings 'redefine'; +local *main::resolve_links = sub { + # A broken loop guard must fail the test before it can exhaust memory. + die "Symlink resolution exceeded 100 calls\n" if (++$calls > 100); + return $original->(@_); + }; +my $resolved = main::resolve_links($path); +return ($resolved, $calls); +} + +# Ordinary files, missing targets, and directory links keep their path spelling. +make_path("$root/data", "$root/real/sub"); +foreach my $file ("$root/data/file", "$root/real/x", "$root/target") { + open(my $fh, '>', $file) or die "create $file: $!"; + close($fh) or die "close $file: $!"; + } +foreach my $link ( + [ 'relative', 'data/file' ], [ 'absolute', "$root/data/file" ], + [ 'dangling', 'missing' ], [ 'alias', 'data' ], + [ 'dir', 'real/sub' ], [ 'x', 'dir/../x' ]) { + symlink($link->[1], "$root/$link->[0]") or die "symlink: $!"; + } +foreach my $case ( + [ 'data/file', 'data/file' ], [ 'relative', 'data/file' ], + [ 'absolute', 'data/file' ], [ 'dangling', 'missing' ], + [ 'alias/file', 'data/file' ], [ 'alias/./file', 'data/./file' ]) { + my ($resolved) = resolve_counted("$root/$case->[0]"); + is($resolved, "$root/$case->[1]", "resolves $case->[0]"); + } + +# The directory link must be expanded before the following .. is interpreted. +my ($resolved) = resolve_counted("$root/x"); +is($resolved, "$root/real/sub/../x", + 'resolves a valid link whose simplified spelling repeats'); +is(main::simplify_path($resolved), "$root/real/x", + 'callers can simplify the fully resolved path'); + +# An absolute target beyond .. must also remain visible to directory checks. +unlink("$root/real/x") or die "unlink: $!"; +symlink("$root/target", "$root/real/x") or die "symlink: $!"; +($resolved) = resolve_counted("$root/x"); +is($resolved, "$root/target", 'follows the final absolute link after ..'); +ok(main::is_under_directory($root, "$root/x"), + 'the valid target is still recognized as inside its directory'); + +# Revisiting a directory link with a shorter remaining path is not a cycle. +($resolved) = resolve_counted("$root/alias/../alias/file"); +is($resolved, "$root/data/../data/file", + 'resolves repeated uses of the same directory link'); + +# Long acyclic chains remain supported without imposing an OS-specific limit. +foreach my $i (0..44) { + my $target = $i == 44 ? 'target' : 'chain'.($i+1); + symlink($target, "$root/chain$i") or die "symlink: $!"; + } +($resolved) = resolve_counted("$root/chain0"); +is($resolved, "$root/target", 'resolves a 45-link chain'); + +# Equivalent loop spellings must still stop after only a few expansions. +foreach my $link ( + [ 'self', 'self' ], [ 'dotself', './dotself' ], + [ 'parentself', '../'.basename($root).'/parentself' ], + [ 'a', 'b' ], [ 'b', 'a' ], + [ 'absa', "$root/absb" ], [ 'absb', "$root/absa" ], + [ 'absolute-self', "$root/absolute-self" ], + [ 'via-dir', 'here/via-dir' ], [ 'here', '.' ]) { + symlink($link->[1], "$root/$link->[0]") or die "symlink: $!"; + } +foreach my $name (qw(self dotself parentself a absa absolute-self via-dir)) { + my ($loop, $calls) = resolve_counted("$root/$name"); + ok(-l $loop, "$name returns an unresolved loop path"); + cmp_ok($calls, '<=', 4, "$name stops promptly"); + } + +done_testing(); diff --git a/web-lib-funcs.pl b/web-lib-funcs.pl index 9bc99a655..1b3954017 100755 --- a/web-lib-funcs.pl +++ b/web-lib-funcs.pl @@ -10591,13 +10591,18 @@ my ($path, $seen) = @_; $seen ||= { }; $path =~ s/\/+/\//g; $path =~ s/\/$// if ($path ne "/"); -my $simple = &simplify_path($path) || $path; -return $path if ($seen->{$simple}++); my @p = split(/\/+/, $path); shift(@p); for(my $i=0; $i<@p; $i++) { my $sofar = "/".join("/", @p[0..$i]); my $lnk = readlink($sofar); + if (defined($lnk)) { + # Earlier components are resolved, so only simplify this prefix. + # Do not collapse .. past a symlink in the remaining path. + my $simple = &simplify_path($sofar) || $sofar; + my $key = $simple."/".join("/", @p[$i+1 .. $#p]); + return $path if ($seen->{$key}++); + } if ($lnk eq $sofar) { # Link to itself! Cannot do anything more really .. last;