diff --git a/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Attributes.pm b/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Attributes.pm index bccb38b6c..64a1f5a16 100644 --- a/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Attributes.pm +++ b/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Attributes.pm @@ -2148,6 +2148,7 @@ qr/^(?:\*\.)?(?:(?:(?:(?:[a-zA-Z0-9][-a-zA-Z0-9]*)?[a-zA-Z0-9])[.])*(?:[a-zA-Z][ }, 'oidcRPMetaDataOptionsExtraClaims' => { 'default' => {}, + 'keyTest' => qr/^[\x21\x23-\x5B\x5D-\x7E]+$/, 'type' => 'keyTextContainer' }, 'oidcRPMetaDataOptionsIcon' => { @@ -2274,7 +2275,8 @@ qr/^(?:\*\.)?(?:(?:(?:(?:[a-zA-Z0-9][-a-zA-Z0-9]*)?[a-zA-Z0-9])[.])*(?:[a-zA-Z][ 'type' => 'keyTextContainer' }, 'oidcServiceDynamicRegistrationExtraClaims' => { - 'type' => 'keyTextContainer' + 'keyTest' => qr/^[\x21\x23-\x5B\x5D-\x7E]+$/, + 'type' => 'keyTextContainer' }, 'oidcServiceIDTokenExpiration' => { 'default' => 3600, diff --git a/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Build/Attributes.pm b/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Build/Attributes.pm index fd92f63c8..86db33bdd 100644 --- a/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Build/Attributes.pm +++ b/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Build/Attributes.pm @@ -3840,7 +3840,8 @@ m{^(?:ldapi://[^/]*/?|\w[\w\-\.]*(?::\d{1,5})?|ldap(?:s|\+tls)?://\w[\w\-\.]*(?: 'OpenID Connect exported variables for dynamic registration', }, oidcServiceDynamicRegistrationExtraClaims => { - type => 'keyTextContainer', + type => 'keyTextContainer', + keyTest => qr/^[\x21\x23-\x5B\x5D-\x7E]+$/, documentation => 'OpenID Connect extra claims for dynamic registration', }, @@ -3960,8 +3961,11 @@ m{^(?:ldapi://[^/]*/?|\w[\w\-\.]*(?::\d{1,5})?|ldap(?:s|\+tls)?://\w[\w\-\.]*(?: oidcRPMetaDataOptionsAuthorizationCodeExpiration => { type => 'int' }, oidcRPMetaDataOptionsOfflineSessionExpiration => { type => 'int' }, oidcRPMetaDataOptionsRedirectUris => { type => 'text', }, - oidcRPMetaDataOptionsExtraClaims => - { type => 'keyTextContainer', default => {} }, + oidcRPMetaDataOptionsExtraClaims => { + type => 'keyTextContainer', + keyTest => qr/^[\x21\x23-\x5B\x5D-\x7E]+$/, + default => {} + }, oidcRPMetaDataOptionsBypassConsent => { type => 'bool', help => 'openidconnectclaims.html', diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Issuer/OpenIDConnect.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Issuer/OpenIDConnect.pm index 457f69cd0..464216861 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Issuer/OpenIDConnect.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Issuer/OpenIDConnect.pm @@ -346,14 +346,20 @@ sub run { } # Check scope validity - unless ( $oidc_request->{'scope'} =~ /^[a-zA-Z_\-\s]+$/ ) { - $self->logger->error( "Submitted scope is not valid: " - . $oidc_request->{'scope'} ); + # We use a slightly more relaxed version of + # https://tools.ietf.org/html/rfc6749#appendix-A.4 + # To be tolerant of user error (trailing spaces, etc.) + # Scope names are restricted to printable ASCII characters, + # excluding double quote and backslash + unless ( + $oidc_request->{'scope'} =~ /^[\x20\x21\x23-\x5B\x5D-\x7E]*$/ ) + { + $self->logger->error("Submitted scope is not valid"); return PE_ERROR; } # Check openid scope - unless ( $oidc_request->{'scope'} =~ /\bopenid\b/ ) { + unless ( $self->_hasScope( 'openid', $oidc_request->{'scope'} ) ) { $self->logger->debug("No openid scope found"); #TODO manage standard OAuth request @@ -467,7 +473,12 @@ sub run { foreach my $requested_scope ( split( /\s+/, $oidc_request->{'scope'} ) ) { - if ( $consent_scope =~ /\b$requested_scope\b/ ) { + if ( + $self->_hasScope( + $requested_scope, $consent_scope + ) + ) + { $self->logger->debug( "Scope $requested_scope already accepted"); } @@ -543,7 +554,8 @@ sub run { my $display_name = $self->conf->{oidcRPMetaDataOptions}->{$rp} ->{oidcRPMetaDataOptionsDisplayName}; - my $icon = $self->conf->{oidcRPMetaDataOptions}->{$rp} + my $icon = + $self->conf->{oidcRPMetaDataOptions}->{$rp} ->{oidcRPMetaDataOptionsIcon}; my $imgSrc; @@ -564,7 +576,7 @@ sub run { }; my @list; foreach my $requested_scope ( - split( /\s/, $oidc_request->{'scope'} ) ) + split( /\s+/, $oidc_request->{'scope'} ) ) { if ( my $message = $scope_messages->{$requested_scope} ) @@ -620,7 +632,9 @@ sub run { # WIP: Offline access my $offline = 0; - if ( $oidc_request->{'scope'} =~ /\boffline_access\b/ ) { + if ( + $self->_hasScope( 'offline_access', $oidc_request->{'scope'} ) ) + { $offline = 1; # MUST ensure that the prompt parameter contains consent unless @@ -655,8 +669,10 @@ sub run { } # Strip offline_access from scopes from now on - $oidc_request->{'scope'} = join " ", grep !/^offline_access$/, - split /\s+/, $oidc_request->{'scope'}; + $oidc_request->{'scope'} = join " ", + grep !/^offline_access$/, + split /\s+/, + $oidc_request->{'scope'}; } # Authorization Code Flow @@ -730,7 +746,8 @@ sub run { "Generated access token: $access_token"); # Compute hash to store in at_hash - my $alg = $self->conf->{oidcRPMetaDataOptions}->{$rp} + my $alg = + $self->conf->{oidcRPMetaDataOptions}->{$rp} ->{oidcRPMetaDataOptionsIDTokenSignAlg}; my ($hash_level) = ( $alg =~ /(?:\w{2})(\d{3})/ ); $at_hash = $self->createHash( $access_token, $hash_level ) @@ -1510,8 +1527,8 @@ sub userInfo { my $userinfo_response = $self->buildUserInfoResponse( $req, $scope, $rp, $session ); unless ($userinfo_response) { - return $self->returnBearerError( 'invalid_request', 'Invalid request', - 401 ); + return $self->returnBearerError( 'invalid_request', + 'Invalid request', 401 ); } my $userinfo_sign_alg = $self->conf->{oidcRPMetaDataOptions}->{$rp} @@ -2020,6 +2037,11 @@ sub exportRequestParameters { return PE_OK; } +sub _hasScope { + my ( $self, $scope, $scopelist ) = @_; + return scalar grep { $_ eq $scope } ( split /\s+/, $scopelist ); +} + sub _convertOldFormatConsents { my ( $self, $req ) = @_; my @oidcConsents = (); diff --git a/lemonldap-ng-portal/site/templates/bootstrap/oidcConsents.tpl b/lemonldap-ng-portal/site/templates/bootstrap/oidcConsents.tpl index 7365cee74..4a8d21c92 100644 --- a/lemonldap-ng-portal/site/templates/bootstrap/oidcConsents.tpl +++ b/lemonldap-ng-portal/site/templates/bootstrap/oidcConsents.tpl @@ -12,7 +12,7 @@ "> "> - + " title="delete" class="oidcConsent"> diff --git a/lemonldap-ng-portal/site/templates/bootstrap/oidcGiveConsent.tpl b/lemonldap-ng-portal/site/templates/bootstrap/oidcGiveConsent.tpl index c93c347e5..235588b32 100644 --- a/lemonldap-ng-portal/site/templates/bootstrap/oidcGiveConsent.tpl +++ b/lemonldap-ng-portal/site/templates/bootstrap/oidcGiveConsent.tpl @@ -8,7 +8,7 @@
  • "> - () + ()
  • diff --git a/lemonldap-ng-portal/t/32-OIDC-Offline-Session.t b/lemonldap-ng-portal/t/32-OIDC-Offline-Session.t index c591e40a1..8c4d166f1 100644 --- a/lemonldap-ng-portal/t/32-OIDC-Offline-Session.t +++ b/lemonldap-ng-portal/t/32-OIDC-Offline-Session.t @@ -82,8 +82,10 @@ $res = $op->_post( ); my $idpId = expectCookie($res); +# Include a weird scope name, to make sure they work (#2168) $query = - "response_type=code&scope=openid%20profile%20email%20offline_access&" + "response_type=code&scope=openid%20profile%20email%20" + . "offline_access%20%21weird%3Ascope.name~&" . "client_id=rpid&state=af0ifjsldkj&redirect_uri=http%3A%2F%2Ftest%2F"; $res = $op->_get( "/oauth2/authorize", @@ -265,7 +267,10 @@ is( $json->{active}, 1 ); is( $json->{client_id}, 'rpid' ); is( $json->{sub}, 'french' ); -count(3); +# #2168 +ok( grep { $_ eq "!weird:scope.name~" } ( split /\s+/, $json->{scope} ), + "Scope contains weird scope name" ); +count(4); clean_sessions(); done_testing( count() ); diff --git a/lemonldap-ng-portal/t/32-OIDC-Token-Security.t b/lemonldap-ng-portal/t/32-OIDC-Token-Security.t index 9c604038a..9b8e90b09 100644 --- a/lemonldap-ng-portal/t/32-OIDC-Token-Security.t +++ b/lemonldap-ng-portal/t/32-OIDC-Token-Security.t @@ -102,6 +102,21 @@ ok( count(1); my $idpId = expectCookie($res); +# Try to get code for RP1 with invalide scope name +$query = +"response_type=code&scope=openid%20profile%20email%22&client_id=rpid&state=af0ifjsldkj&redirect_uri=http%3A%2F%2Frp2.com%2F"; +ok( + $res = $op->_get( + "/oauth2/authorize", + query => "$query", + accept => 'text/html', + cookie => "lemonldap=$idpId", + ), + "Get authorization code for rp1" +); +count(1); +expectPortalError( $res, 24, "Invalid scope" ); +# # Get code for RP1 $query = "response_type=code&scope=openid%20profile%20email&client_id=rpid&state=af0ifjsldkj&redirect_uri=http%3A%2F%2Frp2.com%2F";