Skip to content

Commit

Permalink
improve url rewriting during authentication
Browse files Browse the repository at this point in the history 
apache won't handle encoded ? (%3f) during rewrites. So use a & instead.
  • Loading branch information
@sni
sni committed Jul 10, 2024
1 parent a6a9d4e commit c027ea1
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 15 deletions.
9 changes: 9 additions & 0 deletions lib/Thruk/Controller/login.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ sub index {
$keywords = 'logout';
$logoutref = $1;
}
# replace first & with ? (so we don't have to use encoded %3f which breaks apache rewrites)
if($logoutref && $logoutref !~ m/\?/mx) {
$logoutref =~ s|&|?|mx;
}

my $login = $c->req->parameters->{'login'} || '';
my $pass = $c->req->parameters->{'password'} || '';
Expand All @@ -76,6 +80,11 @@ sub index {
$referer =~ s/^(logout|expired|invalid|problem|locked|setsession|nocookie)\&//gmx;
$keywords =~ s/^(logout|expired|invalid|problem|locked|setsession|nocookie)\&.*$/$1/gmx if $keywords;

# replace first & with ? (so we don't have to use encoded %3f which breaks apache rewrites)
if($referer && $referer !~ m/\?/mx) {
$referer =~ s|&|?|mx;
}

$c->stash->{'referer'} = $referer;
$c->stash->{'clean_cookies'} = 0;

Expand Down
35 changes: 25 additions & 10 deletions script/thruk_auth
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ sub process {

# remove last &
$path =~ s|/____/|?|mxo;
$path =~ s|\?$||mxo;
$path =~ s|\?$||mxo; # remove trailing ?

# some urls must pass
if($path =~ $pass_regex) {
Expand Down Expand Up @@ -194,7 +194,7 @@ sub process {
if(defined $cached && ($cache_timeout == 0 || ($cache_timeout > 0 && $sessioncache->{$auth}->{'time'} >= $now - $cache_timeout))) {
if($sessioncache->{$auth}->{'failed'}) {
_debug("basic auth login failed by cache hit: ".substr($auth, 0, 6)."...") if $verbose > 1;
return $loginurl."?invalid&".$path;
return $loginurl."?invalid&"._encode_path($path);
}
_debug("basic auth login by cache hit: ".$sessioncache->{$auth}->{'login'}) if $verbose > 1;
return "/loginok/".$sessioncache->{$auth}->{'login'}."/".$path;
Expand All @@ -217,14 +217,14 @@ sub process {
'failed' => 1,
'time' => $now,
};
return $loginurl."?invalid&".$path;
return $loginurl."?invalid&"._encode_path($path);
}

# did we get a cookie
if($cookies eq '' || $cookies !~ $cookie_regex) {
_debug("no cookie: $path") if $verbose;
return "/loginok/".$guest."/".$path if $guest;
return $loginurl."?nocookie&".$path;
return $loginurl."?nocookie&"._encode_path($path);
}
my $auth = $1;

Expand All @@ -233,7 +233,7 @@ sub process {
if(defined $sessioncache->{$auth} && ($cache_timeout == 0 || ($cache_timeout > 0 && $sessioncache->{$auth}->{'time'} >= $now - $cache_timeout))) {
if($sessioncache->{$auth}->{'failed'}) {
_debug("login failed by cache hit") if $verbose > 1;
return $loginurl."?invalid&".$path;
return $loginurl."?invalid&"._encode_path($path);
}
_debug("login by cache hit: ".$sessioncache->{$auth}->{'login'}) if $verbose > 1;
return "/loginok/".$sessioncache->{$auth}->{'login'}."/".$path;
Expand All @@ -246,7 +246,7 @@ sub process {
_debug("session expired: ".($sessioncache->{$auth}->{'login'} || '?')) if $verbose;
delete $sessioncache->{$auth} if defined $sessioncache->{$auth};
return "/loginok/".$guest."/".$path if $guest;
return $loginurl."?expired&".$path;
return $loginurl."?expired&"._encode_path($path);
}

# fill sessioncache, so we don't have to revalidate new sessions immediately
Expand All @@ -265,7 +265,7 @@ sub process {
unlink($session->{'file'});
delete $sessioncache->{$auth};
return "/loginok/".$guest."/".$path if $guest;
return $loginurl."?expired&".$path;
return $loginurl."?expired&"._encode_path($path);
}

# revalidation disabled
Expand All @@ -274,7 +274,7 @@ sub process {
# no validation enabled
if($sessioncache->{$auth}->{'failed'}) {
_debug("session not ok by cache hit") if $verbose > 1;
return $loginurl."?invalid&".$path;
return $loginurl."?invalid&"._encode_path($path);
}
_debug("session ok (not revalidated)") if $verbose > 1;
}
Expand All @@ -298,14 +298,14 @@ sub process {
_audit_log("session", "technical issue during session revalidation, removing session file", $session->{'username'}//'?', $session->{'file'}, 0);
unlink($session->{'file'});
delete $sessioncache->{$auth};
return $loginurl."?problem&".$path;
return $loginurl."?problem&"._encode_path($path);
}
elsif($rc == 0) {
_debug("basic auth verify failed for ".$user) if $verbose;
_audit_log("session", "session revalidation failed, removing session file", $session->{'username'}//'?', $session->{'file'}, 0);
unlink($session->{'file'});
delete $sessioncache->{$auth};
return $loginurl."?invalid&".$path;
return $loginurl."?invalid&"._encode_path($path);
}
}

Expand Down Expand Up @@ -343,3 +343,18 @@ sub _clean_credentials {
}
return($str);
}

##########################################################
sub _encode_path {
my($path) = @_;

# replace first ? by &
$path =~ s|\?|&|mx;

# replace first encoded ? by &
$path =~ s|%3f|&|mx;

return($path);
}

##########################################################
18 changes: 15 additions & 3 deletions t/300-controller_cookie_auth.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ BEGIN {
eval "use Test::Cmd";
plan skip_all => 'Test::Cmd required' if $@;
plan skip_all => 'backends required' if(!-s 'thruk_local.conf' and !defined $ENV{'PLACK_TEST_EXTERNALSERVER_URI'});
plan tests => 59;
plan tests => 67;

use lib('t');
require TestUtils;
Expand All @@ -24,8 +24,8 @@ SKIP: {
my $pages = [
{ url => '/thruk/cgi-bin/login.cgi', like => ['Thruk Monitoring Webinterface', 'loginuser' ], code => 401 },
{ url => '/thruk/cgi-bin/login.cgi?logout/thruk/cgi-bin/tac.cgi', 'redirect' => 1, location => 'tac.cgi', like => 'This item has moved' },
{ url => '/thruk/cgi-bin/login.cgi?logout/thruk/cgi-bin/tac.cgi%3ftest=blah', 'redirect' => 1, location => 'tac.cgi\?test=blah', like => 'This item has moved' },
{ url => '/thruk/cgi-bin/login.cgi?logout/thruk/cgi-bin/tac.cgi%3ftest=blah&test2=blub', 'redirect' => 1, location => 'tac.cgi\?test=blah&test2=blub', like => 'This item has moved' },
{ url => '/thruk/cgi-bin/login.cgi?logout/thruk/cgi-bin/tac.cgi&test=blah', 'redirect' => 1, location => 'tac.cgi\?test=blah', like => 'This item has moved' },
{ url => '/thruk/cgi-bin/login.cgi?logout/thruk/cgi-bin/tac.cgi&test=blah&test2=blub', 'redirect' => 1, location => 'tac.cgi\?test=blah&test2=blub', like => 'This item has moved' },
];

for my $url (@{$pages}) {
Expand All @@ -44,3 +44,15 @@ TestUtils::test_command({
stdin => '///____/thruk/cgi-bin/tac.cgi',
like => ['/^\/redirect\/thruk\/cgi\-bin\/login\.cgi\?nocookie&thruk\/cgi\-bin\/tac\.cgi$/'],
});

TestUtils::test_command({
cmd => './script/thruk_auth',
stdin => '///____/thruk/cgi-bin/tac.cgi%3ftest=blah',
like => ['/^\/redirect\/thruk\/cgi\-bin\/login\.cgi\?nocookie&thruk\/cgi\-bin\/tac\.cgi&test=blah$/'],
});

TestUtils::test_command({
cmd => './script/thruk_auth',
stdin => '///____/thruk/cgi-bin/tac.cgi?test=blah',
like => ['/^\/redirect\/thruk\/cgi\-bin\/login\.cgi\?nocookie&thruk\/cgi\-bin\/tac\.cgi&test=blah$/'],
});
1 change: 0 additions & 1 deletion t/TestUtils.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,6 @@ sub _external_request {
my $login_page = $req->{'_headers'}->{'location'};
my $r = _external_request($login_page, undef, undef, $agent);
$login_page =~ s/nocookie&//gmx;
$login_page =~ s/%3f/?/gmx; # fix AH: Unsafe URL with %3f URL rewritten without UnsafeAllow3F
$r = _external_request($login_page, undef, { password => $pass, login => $user, submit => 'login', referer => $referer }, $agent, undef, undef, 0);
$req = _external_request($r->{'_headers'}->{'location'}, $start_to, $post, $agent, undef, undef, 0);
}
Expand Down
12 changes: 11 additions & 1 deletion t/scenarios/auth/t/100-auth.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use strict;
use Test::More;

BEGIN {
plan tests => 308;
plan tests => 317;

use lib('t');
require TestUtils;
Expand Down Expand Up @@ -202,3 +202,13 @@ BEGIN {
'code' => 401,
);
};

# check redirect when there is no cookie
{
local $ENV{'THRUK_TEST_AUTH'} = undef;
TestUtils::test_page(
'url' => '/thruk/cgi-bin/extinfo.cgi?type=2&host=localhost',
'redirect' => 1,
'location' => 'login.cgi\?nocookie&demo/thruk/cgi-bin/extinfo.cgi&type=2&host=localhost',
);
};

0 comments on commit c027ea1

Please sign in to comment.