Improve security of redirect URIs (fixes #166)
Change-Id: Ic9b771d71c14f43eca83b1f2ad39d51e9a37cb53
diff --git a/Changes b/Changes
index f41203a..ad2ee66 100755
--- a/Changes
+++ b/Changes
@@ -22,6 +22,7 @@
- Fix tour localization to be overwritable. (diewald)
- Make scope a requirement for OAuth authorizations.
(diewald)
+ - Improve security of OAuth redirects. (diewald)
WARNING: Mojolicious 9.31 is a security update -
updating is highly recommended.
diff --git a/lib/Kalamar/Plugin/Auth.pm b/lib/Kalamar/Plugin/Auth.pm
index bd3c388..58896d9 100644
--- a/lib/Kalamar/Plugin/Auth.pm
+++ b/lib/Kalamar/Plugin/Auth.pm
@@ -131,6 +131,7 @@
short => 'Zugriffsrechte erteilen'
},
oauthGrantPublicWarn => 'Achtung - dies ist ein öffentlicher Client!',
+ oauthGrantRedirectWarn => 'Die Weiterleitung findet an eine unbekannte Adresse statt',
createdAt => 'Erstellt am <time datetime="<%= stash("date") %>"><%= stash("date") %></date>.',
expiresIn => 'Läuft in <%= stash("seconds") %> Sekunden ab.',
fileSizeExceeded => 'Dateigröße überschritten'
@@ -189,6 +190,7 @@
short => 'Grant access'
},
oauthGrantPublicWarn => 'Warning - this is a public client!',
+ oauthGrantRedirectWarn => 'The redirect points to an unknown location',
createdAt => 'Created at <time datetime="<%= stash("date") %>"><%= stash("date") %></date>.',
expiresIn => 'Expires in <%= stash("seconds") %> seconds.',
fileSizeExceeded => 'File size exceeded',
@@ -435,10 +437,10 @@
$app->helper(
'auth.client_info_p' => sub {
my $c = shift;
- my $client_id = shift;
+ my $req_client_id = shift;
# Get list of registered clients
- my $r_url = Mojo::URL->new($c->korap->api)->path('oauth2/client/')->path($client_id);
+ my $r_url = Mojo::URL->new($c->korap->api)->path('oauth2/client/')->path($req_client_id);
my $form = {
super_client_id => $client_id,
@@ -1133,10 +1135,10 @@
_set_no_cache($c->res->headers);
my $v = $c->validation;
- $v->required('client_id');
- $v->required('scope');
- $v->optional('state');
- $v->optional('redirect_uri');
+ $v->required('client_id', 'trim');
+ $v->required('scope', 'trim');
+ $v->optional('state', 'trim');
+ $v->optional('redirect_uri', 'trim', 'not_empty')->like(qr/^(http|$)/i);
# Redirect with error
if ($v->has_error) {
@@ -1174,6 +1176,61 @@
)->then(
sub {
+ # Check, if the client redirect_uri is valid
+ my $redirect_uri_server = $c->stash('redirect_uri_server');
+ my $redirect_uri = $v->param('redirect_uri');
+ my ($server_o, $client_o);
+
+ # Both exist
+ if ($redirect_uri_server && $redirect_uri) {
+ $server_o = Mojo::URL->new($redirect_uri_server);
+ $client_o = Mojo::URL->new($redirect_uri);
+
+ # Host not valid - take registered URI
+ if ($server_o->host ne $client_o->host) {
+ $c->notify(warn => 'redirect_uri host differs from registered host');
+ $client_o = $server_o;
+ }
+
+ # Port not valid - take registered URI
+ elsif (($server_o->port // '') ne ($client_o->port // '')) {
+ $c->notify(warn => 'redirect_uri port differs from registered port');
+ $client_o = $server_o;
+ };
+ }
+
+ # Client sent exists
+ elsif ($redirect_uri) {
+ $client_o = Mojo::URL->new($redirect_uri);
+ $c->stash(redirect_warning => $c->loc('oauthGrantRedirectWarn'))
+ }
+
+ # Server registered exists
+ elsif ($redirect_uri_server) {
+ $client_o = Mojo::URL->new($redirect_uri_server);
+ }
+
+ # Redirect unknown
+ else {
+ $c->notify(error => 'redirect_uri not set');
+ return $c->redirect_to('oauth-settings');
+ };
+
+ # No userinfo allowed
+ if ($client_o->userinfo) {
+ $c->notify(warn => 'redirect_uri contains userinfo');
+ $client_o->userinfo('');
+ };
+
+ # HTTPS warning
+ # if ($client_o->scheme ne 'https') {
+ # $c->notify(warn => 'redirect_uri is not HTTPS');
+ # };
+
+ # Sign redirection URL
+ $c->stash(redirect_uri => $client_o->to_string);
+ $c->stash(close_redirect_uri => '' . $c->close_redirect_to($client_o));
+
# Get auth token
my $auth_token = $c->auth->token;
@@ -1208,17 +1265,17 @@
# KorAP and not by the client!
my $v = $c->validation;
$v->csrf_protect;
- $v->required('client_id');
- $v->required('scope');
- $v->optional('state');
- $v->optional('redirect_uri');
+ $v->required('client_id', 'trim');
+ $v->required('scope', 'trim');
+ $v->optional('state', 'trim');
- # WARN! SIGN THIS TO PREVENT OPEN REDIRECT ATTACKS!
- $v->required('redirect_uri_server');
+ # Only signed redirects are allowed
+ $v->required('redirect_uri', 'trim', 'not_empty')->closed_redirect;
# Render with error
if ($v->has_error) {
- my $url = Mojo::URL->new($v->param('redirect_uri_server') || $c->url_for('index'));
+
+ my $url = $c->url_for('oauth-settings');
if ($v->has_error('client_id')) {
$url->query([error_description => $c->loc('Auth_clientIDFail')]);
@@ -1237,12 +1294,12 @@
};
state $r_url = Mojo::URL->new($c->korap->api)->path('oauth2/authorize');
- $c->stash(redirect_uri_server => Mojo::URL->new($v->param('redirect_uri_server')));
+ $c->stash(redirect_uri => Mojo::URL->new($v->param('redirect_uri')));
return $c->korap_request(post => $r_url, {} => form => {
response_type => 'code',
client_id => $v->param('client_id'),
- redirect_uri => $v->param('redirect_uri'),
+ redirect_uri => $c->stash('redirect_uri'),
state => $v->param('state'),
scope => $v->param('scope'),
})->then(
@@ -1279,7 +1336,7 @@
)->catch(
sub {
my $err_msg = shift;
- my $url = $c->stash('redirect_uri_server');
+ my $url = $c->stash('redirect_uri');
if ($err_msg) {
$url = $url->query([error_description => $err_msg]);
};
diff --git a/lib/Kalamar/Plugin/Auth/templates/auth/grant_scope.html.ep b/lib/Kalamar/Plugin/Auth/templates/auth/grant_scope.html.ep
index c2c4a2f..f28d8d0 100644
--- a/lib/Kalamar/Plugin/Auth/templates/auth/grant_scope.html.ep
+++ b/lib/Kalamar/Plugin/Auth/templates/auth/grant_scope.html.ep
@@ -15,6 +15,12 @@
% if (stash('client_type') eq 'PUBLIC') {
<blockquote class="warning"><%= loc 'oauthGrantPublicWarn' %></blockquote>
% };
+ % if (stash('redirect_warning')) {
+ <blockquote class="warning">
+ <p><%= stash 'redirect_warning' %>:</p>
+ <pre><%= stash('redirect_uri') %></pre>
+ </blockquote>
+ % };
</li>
</ul>
@@ -22,8 +28,7 @@
%= csrf_field
%= hidden_field 'client_id' => stash('client_id')
%= hidden_field 'state' => stash('state')
- %= hidden_field 'redirect_uri' => stash('redirect_uri')
- %= hidden_field 'redirect_uri_server' => stash('redirect_uri_server')
+ %= hidden_field 'redirect_uri' => stash('close_redirect_uri')
% if (stash('scope')) {
<ul id="scopes">
% foreach (split(/\s+/, stash('scope'))) {
@@ -34,5 +39,5 @@
% };
<input type="submit" class="form-submit" value="<%= loc 'Auth_oauthGrantScope_short' %>" />
- %= link_to loc('abort') => (stash('redirect_uri_server') // stash('redirect_uri')) => {} => (class => 'form-button button-abort form-submit')
+ %= link_to loc('abort') => (stash('redirect_uri')) => {} => (class => 'form-button button-abort form-submit')
% end
diff --git a/lib/Kalamar/Plugin/Auth/templates/partial/auth/login.html.ep b/lib/Kalamar/Plugin/Auth/templates/partial/auth/login.html.ep
index b80d7ad..9d6d535 100644
--- a/lib/Kalamar/Plugin/Auth/templates/partial/auth/login.html.ep
+++ b/lib/Kalamar/Plugin/Auth/templates/partial/auth/login.html.ep
@@ -17,7 +17,7 @@
%= hidden_field 'client_name' => stash('client_name')
%= hidden_field 'state' => stash('state')
%= hidden_field 'scope' => stash('scope')
- %= hidden_field 'redirect_uri' => stash('redirect_uri')
+ %= hidden_field 'redirect_uri' => stash('close_redirect_uri')
% };
<div>
%= password_field 'pwd', placeholder => loc('pwd')
diff --git a/t/plugin/auth-oauth.t b/t/plugin/auth-oauth.t
index ba1f95d..a2cc73c 100644
--- a/t/plugin/auth-oauth.t
+++ b/t/plugin/auth-oauth.t
@@ -829,6 +829,41 @@
# "client_redirect_uri" => $redirect_uri
});
+$fake_backend_app->add_client({
+ "client_id" => 'xyz2',
+ "client_name" => 'New added client',
+ "client_description" => 'This is a new client',
+ "client_url" => 'http://example.com',
+ "client_type" => 'CONFIDENTIAL',
+ "client_redirect_uri" => 'http://redirect.url/'
+});
+
+$fwd = $t->get_ok(Mojo::URL->new('/settings/oauth/authorize')->query({
+ client_id => 'xyz2',
+ scope => 'search match',
+ redirect_uri => 'http://test.com/',
+}))
+ ->status_is(200)
+ ->text_is('div.notify-warn', 'redirect_uri host differs from registered host')
+ ->element_exists_not('div.notify-error')
+ ->element_exists_not('div.notify-success')
+ ->element_exists("input[name=handle_or_email]")
+ ->tx->res->dom->at('input[name=csrf_token]')->attr('value')
+ ;
+
+$fwd = $t->get_ok(Mojo::URL->new('/settings/oauth/authorize')->query({
+ client_id => 'xyz2',
+ scope => 'search match',
+ redirect_uri => 'http://redirect.url:9000/',
+}))
+ ->status_is(200)
+ ->text_is('div.notify-warn', 'redirect_uri port differs from registered port')
+ ->element_exists_not('div.notify-error')
+ ->element_exists_not('div.notify-success')
+ ->element_exists("input[name=handle_or_email]")
+ ->tx->res->dom->at('input[name=csrf_token]')->attr('value')
+ ;
+
$fwd = $t->get_ok(Mojo::URL->new('/settings/oauth/authorize')->query({
client_id => 'xyz',
state => 'abcde',
@@ -863,11 +898,15 @@
->status_is(200)
->attr_is('input[name=client_id]','value','xyz')
->attr_is('input[name=state]','value','abcde')
+ ->attr_like('input[name=redirect_uri]','value', qr!^http://test\.com\/\?crto=.{3,}!)
->text_is('ul#scopes li:nth-child(1)','search')
->text_is('ul#scopes li:nth-child(2)','match')
->text_is('span.client-name','New added client')
->attr_is('a.form-button','href','http://test.com/')
->attr_is('a.embedded-link', 'href', '/doc/korap/kalamar')
+ ->element_exists_not('div.notify-error')
+ ->element_exists_not('div.notify-warn')
+ ->element_exists_not('blockquote.warning')
;
$t->get_ok(Mojo::URL->new('/settings/oauth/authorize')->query({
@@ -893,7 +932,7 @@
redirect_uri => 'http://test.com/'
}))
->status_is(302)
- ->header_is('location', '/?error_description=Bad+CSRF+token')
+ ->header_is('location', '/settings/oauth?error_description=Bad+CSRF+token')
;
$fwd = $t->post_ok(Mojo::URL->new('/settings/oauth/authorize')->query({
@@ -918,12 +957,12 @@
client_id => 'xyz',
state => 'fail',
scope => 'search match',
- redirect_uri_server => 'http://example.com/',
+# redirect_uri_server => 'http://example.com/',
redirect_uri => $fake_backend_app->url_for('return_uri')->to_abs,
csrf_token => $csrf,
}))
->status_is(302)
- ->header_is('location', 'http://example.com/?error_description=FAIL')
+ ->header_is('location', '/realapi/fakeclient/return?error_description=FAIL')
;
my $json_post = {