Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Walk the entire DefaultRoot path, checking for symlinks of any compon… #444

Merged
merged 1 commit into from Mar 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Walk the entire DefaultRoot path, checking for symlinks of any compon…
…ent,

when AllowChrootSymlinks is disabled.
  • Loading branch information
Castaglia committed Mar 6, 2017
commit 349addc3be4fcdad9bd4ec01ad1ccd916c898ed8
73 changes: 58 additions & 15 deletions modules/mod_auth.c
Expand Up @@ -804,6 +804,59 @@ static const char *get_default_chdir(pool *p, xaset_t *conf) {
return dir;
}

static int is_symlink_path(pool *p, const char *path, size_t pathlen) {
int res, xerrno = 0;
struct stat st;
char *ptr;

if (pathlen == 0) {
return 0;
}

pr_fs_clear_cache2(path);
res = pr_fsio_lstat(path, &st);
if (res < 0) {
xerrno = errno;

pr_log_pri(PR_LOG_WARNING, "error: unable to check %s: %s", path,
strerror(xerrno));

errno = xerrno;
return -1;
}

if (S_ISLNK(st.st_mode)) {
errno = EPERM;
return -1;
}

/* To handle the case where a component further up the path might be a
* symlink (which lstat(2) will NOT handle), we walk the path backwards,
* calling ourselves recursively.
*/

ptr = strrchr(path, '/');
if (ptr != NULL) {
char *new_path;
size_t new_pathlen;

pr_signals_handle();

new_pathlen = ptr - path;
new_path = pstrndup(p, path, new_pathlen);

pr_log_debug(DEBUG10,
"AllowChrootSymlink: path '%s' not a symlink, checking '%s'", path,
new_path);
res = is_symlink_path(p, new_path, new_pathlen);
if (res < 0) {
return -1;
}
}

return 0;
}

/* Determine if the user (non-anon) needs a default root dir other than /. */
static int get_default_root(pool *p, int allow_symlinks, const char **root) {
config_rec *c = NULL;
Expand Down Expand Up @@ -847,7 +900,6 @@ static int get_default_root(pool *p, int allow_symlinks, const char **root) {

if (allow_symlinks == FALSE) {
char *path, target_path[PR_TUNABLE_PATH_MAX + 1];
struct stat st;
size_t pathlen;

/* First, deal with any possible interpolation. dir_realpath() will
Expand Down Expand Up @@ -878,22 +930,13 @@ static int get_default_root(pool *p, int allow_symlinks, const char **root) {
path[pathlen-1] = '\0';
}

pr_fs_clear_cache2(path);
res = pr_fsio_lstat(path, &st);
res = is_symlink_path(p, path, pathlen);
if (res < 0) {
xerrno = errno;

pr_log_pri(PR_LOG_WARNING, "error: unable to check %s: %s", path,
strerror(xerrno));

errno = xerrno;
return -1;
}
if (errno == EPERM) {
pr_log_pri(PR_LOG_WARNING, "error: DefaultRoot %s is a symlink "
"(denied by AllowChrootSymlinks config)", path);
}

if (S_ISLNK(st.st_mode)) {
pr_log_pri(PR_LOG_WARNING,
"error: DefaultRoot %s is a symlink (denied by AllowChrootSymlinks "
"config)", path);
errno = EPERM;
return -1;
}
Expand Down
23 changes: 14 additions & 9 deletions tests/t/lib/ProFTPD/TestSuite/Utils.pm
Expand Up @@ -1198,6 +1198,7 @@ sub test_setup {
$uid = 500 unless defined($uid);
my $gid = shift;
$gid = 500 unless defined($gid);
my $home_dir = shift;
my $config_file = "$tmpdir/$name.conf";
my $pid_file = File::Spec->rel2abs("$tmpdir/$name.pid");
Expand All @@ -1206,17 +1207,21 @@ sub test_setup {
my $auth_user_file = File::Spec->rel2abs("$tmpdir/$name.passwd");
my $auth_group_file = File::Spec->rel2abs("$tmpdir/$name.group");
my $home_dir = File::Spec->rel2abs($tmpdir);
# If the caller provides the home directory, it is ASSUMED that they will
# have created it.
unless (defined($home_dir)) {
$home_dir = File::Spec->rel2abs($tmpdir);
# Make sure that, if we're running as root, that the home directory has
# permissions/privs set for the account we create
if ($< == 0) {
unless (chmod(0755, $home_dir)) {
croak("Can't set perms on $home_dir to 0755: $!");
}
# Make sure that, if we're running as root, that the home directory has
# permissions/privs set for the account we create
if ($< == 0) {
unless (chmod(0755, $home_dir)) {
croak("Can't set perms on $home_dir to 0755: $!");
}

unless (chown($uid, $gid, $home_dir)) {
croak("Can't set owner of $home_dir to $uid/$gid: $!");
unless (chown($uid, $gid, $home_dir)) {
croak("Can't set owner of $home_dir to $uid/$gid: $!");
}
}
}

Expand Down
76 changes: 21 additions & 55 deletions tests/t/lib/ProFTPD/Tests/Config/DefaultRoot.pm
Expand Up @@ -621,65 +621,39 @@ sub defaultroot_allowchrootsymlinks_bug3852 {
my $self = shift;
my $tmpdir = $self->{tmpdir};

my $config_file = "$tmpdir/config.conf";
my $pid_file = File::Spec->rel2abs("$tmpdir/config.pid");
my $scoreboard_file = File::Spec->rel2abs("$tmpdir/config.scoreboard");

my $log_file = test_get_logfile();

my $auth_user_file = File::Spec->rel2abs("$tmpdir/config.passwd");
my $auth_group_file = File::Spec->rel2abs("$tmpdir/config.group");

my $user = 'proftpd';
my $passwd = 'test';
my $group = 'ftpd';
my $home_dir = File::Spec->rel2abs("$tmpdir/home.d/symlinks/$user");
my $uid = 500;
my $gid = 500;

my $intermed_dir = File::Spec->rel2abs("$tmpdir/home.d/symlinks");
mkpath($intermed_dir);

my $symlink_dst = File::Spec->rel2abs("$tmpdir/real/$user");
mkpath($symlink_dst);
my $symlink_dst = File::Spec->rel2abs("$tmpdir/real");

my $cwd = getcwd();

unless (chdir($intermed_dir)) {
die("Can't chdir to $intermed_dir: $!");
unless (chdir($tmpdir)) {
die("Can't chdir to $tmpdir: $!");
}

unless (symlink("../../real/$user", "./$user")) {
die("Can't symlink '../../real/$user' to './$user': $!");
unless (symlink("./real", "./home.d")) {
die("Can't symlink './real' to './home.d': $!");
}

unless (chdir($cwd)) {
die("Can't chdir to $cwd: $!");
}

# Make sure that, if we're running as root, that the home directory has
# permissions/privs set for the account we create
if ($< == 0) {
unless (chmod(0755, $symlink_dst)) {
die("Can't set perms on $symlink_dst to 0755: $!");
}
mkpath(File::Spec->rel2abs("$tmpdir/real/symlinks/$user"));

unless (chown($uid, $gid, $symlink_dst)) {
die("Can't set owner of $symlink_dst to $uid/$gid: $!");
}
}
auth_user_write($auth_user_file, $user, $passwd, $uid, $gid, $home_dir,
'/bin/bash');
auth_group_write($auth_group_file, $group, $gid, $user);
my $setup = test_setup($tmpdir, 'config', $user, undef, undef, $uid, $gid,
$home_dir);

my $config = {
PidFile => $pid_file,
ScoreboardFile => $scoreboard_file,
SystemLog => $log_file,
PidFile => $setup->{pid_file},
ScoreboardFile => $setup->{scoreboard_file},
SystemLog => $setup->{log_file},

AuthUserFile => $auth_user_file,
AuthGroupFile => $auth_group_file,
AuthUserFile => $setup->{auth_user_file},
AuthGroupFile => $setup->{auth_group_file},

AllowChrootSymlinks => 'off',
DefaultRoot => '~',
Expand All @@ -691,7 +665,8 @@ sub defaultroot_allowchrootsymlinks_bug3852 {
},
};

my ($port, $config_user, $config_group) = config_write($config_file, $config);
my ($port, $config_user, $config_group) = config_write($setup->{config_file},
$config);

# Open pipes, for use between the parent and child processes. Specifically,
# the child will indicate when it's done with its test by writing a message
Expand All @@ -709,7 +684,7 @@ sub defaultroot_allowchrootsymlinks_bug3852 {
if ($pid) {
eval {
my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port);
eval { $client->login($user, $passwd) };
eval { $client->login($user, $setup->{passwd}) };
unless ($@) {
die("Login succeeded unexpectedly");
}
Expand All @@ -721,13 +696,12 @@ sub defaultroot_allowchrootsymlinks_bug3852 {

$expected = 530;
$self->assert($expected == $resp_code,
test_msg("Expected response code $expected, got $resp_code"));
"Expected response code $expected, got $resp_code");

$expected = "Login incorrect.";
$self->assert($expected eq $resp_msg,
test_msg("Expected response message '$expected', got '$resp_msg'"));
"Expected response message '$expected', got '$resp_msg'");
};

if ($@) {
$ex = $@;
}
Expand All @@ -736,7 +710,7 @@ sub defaultroot_allowchrootsymlinks_bug3852 {
$wfh->flush();

} else {
eval { server_wait($config_file, $rfh) };
eval { server_wait($setup->{config_file}, $rfh) };
if ($@) {
warn($@);
exit 1;
Expand All @@ -746,18 +720,10 @@ sub defaultroot_allowchrootsymlinks_bug3852 {
}

# Stop server
server_stop($pid_file);

server_stop($setup->{pid_file});
$self->assert_child_ok($pid);

if ($ex) {
test_append_logfile($log_file, $ex);
unlink($log_file);

die($ex);
}

unlink($log_file);
test_cleanup($setup->{log_file}, $ex);
}

1;