diff --git a/Makefile.PL b/Makefile.PL index c2b98342..a558e618 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -45,6 +45,7 @@ my %WriteMakefileArgs = ( "Types::Serialiser" => 0, "URI" => 0, "URI::Encode" => 0, + "URI::Escape" => 0, "URI::QueryParam" => 0, "URN::OASIS::SAML2" => "0.002", "XML::Enc" => "0.05", @@ -124,6 +125,7 @@ my %FallbackPrereqs = ( "Types::Serialiser" => 0, "URI" => 0, "URI::Encode" => 0, + "URI::Escape" => 0, "URI::QueryParam" => 0, "URI::URL" => 0, "URN::OASIS::SAML2" => "0.002", diff --git a/cpanfile b/cpanfile index f8cc8772..a427f6a0 100644 --- a/cpanfile +++ b/cpanfile @@ -29,6 +29,7 @@ requires "MooseX::Types::URI" => "0"; requires "Types::Serialiser" => "0"; requires "URI" => "0"; requires "URI::Encode" => "0"; +requires "URI::Escape" => "0"; requires "URI::QueryParam" => "0"; requires "URN::OASIS::SAML2" => "0.002"; requires "XML::Enc" => "0.05"; diff --git a/lib/Net/SAML2/Binding/Redirect.pm b/lib/Net/SAML2/Binding/Redirect.pm index b67bfb1f..caf06a4b 100644 --- a/lib/Net/SAML2/Binding/Redirect.pm +++ b/lib/Net/SAML2/Binding/Redirect.pm @@ -1,19 +1,24 @@ -use strict; -use warnings; package Net::SAML2::Binding::Redirect; +use Moose; + # VERSION -use Moose; +use Carp qw(croak); +use Crypt::OpenSSL::RSA; +use Crypt::OpenSSL::X509; +use File::Slurper qw/ read_text /; +use IO::Compress::RawDeflate qw/ rawdeflate /; +use IO::Uncompress::RawInflate qw/ rawinflate /; +use MIME::Base64 qw/ encode_base64 decode_base64 /; use MooseX::Types::URI qw/ Uri /; use Net::SAML2::Types qw(signingAlgorithm SAMLRequestType); -use Carp qw(croak); +use URI::Encode qw/uri_decode/; +use URI::Escape qw(uri_unescape); +use URI::QueryParam; +use URI; # ABSTRACT: Net::SAML2::Binding::Redirect - HTTP Redirect binding for SAML -=head1 NAME - -Net::SAML2::Binding::Redirect - =head1 SYNOPSIS my $redirect = Net::SAML2::Binding::Redirect->new( @@ -32,16 +37,6 @@ Net::SAML2::Binding::Redirect =cut -use MIME::Base64 qw/ encode_base64 decode_base64 /; -use IO::Compress::RawDeflate qw/ rawdeflate /; -use IO::Uncompress::RawInflate qw/ rawinflate /; -use URI; -use URI::QueryParam; -use Crypt::OpenSSL::RSA; -use Crypt::OpenSSL::X509; -use File::Slurper qw/ read_text /; -use URI::Encode qw/uri_decode/; - =head2 new( ... ) Constructor. Creates an instance of the Redirect binding. @@ -218,7 +213,7 @@ sub sign { return $u->as_string; } -sub _verified { +sub _verify { my ($self, $sigalg, $signed, $sig) = @_; foreach my $crt (@{$self->cert}) { @@ -235,84 +230,60 @@ sub _verified { $rsa_pub->use_sha512_hash; } elsif ($sigalg eq 'http://www.w3.org/2000/09/xmldsig#rsa-sha1') { $rsa_pub->use_sha1_hash; - } else { - warn "Unsupported Signature Algorithim: $sigalg" if ($self->debug); } - - if ($rsa_pub->verify($signed, $sig)) { - return 1; + else { + warn "Unsupported Signature Algorithim: $sigalg, defaulting to sha256" if $self->debug; } - warn "Unable to verify with " . $cert->subject if ($self->debug); + return 1 if $rsa_pub->verify($signed, $sig); + + warn "Unable to verify with " . $cert->subject if $self->debug; } - die "bad sig"; + croak("Unable to verify the XML signature"); } -=head2 verify( $url ) +=head2 verify( $query_string ) Decode a Redirect binding URL. Verifies the signature on the response. +Requires the *raw* query string to be passed, because L parses and +re-encodes URI-escapes in uppercase (C<%3f> becomes C<%3F>, for instance), +which leads to signature verification failures if the other party uses lower +case (or mixed case). + =cut sub verify { my ($self, $url) = @_; - my $u = URI->new($url); - # verify the response - my $sigalg = $u->query_param('SigAlg'); + # This now becomes the query string + $url =~ s#^https?://.+\?##; - my $signed; - my $saml_request; - my $sig = $u->query_param_delete('Signature'); + my %params = map { split(/=/, $_, 2) } split(/&/, $url); - # During the verify the only query parameters that should be in the query are - # 'SAMLRequest', 'RelayState', 'Sig', 'SigAlg' the other parameter values are - # deleted from the URI query that was created from the URL that was passed - # to the verify function - my @signed_params = ('SAMLRequest', 'SAMLResponse', 'RelayState', 'Sig', 'SigAlg'); - - for my $key ($u->query_param) { - if (grep /$key/, @signed_params ) { - next; - } - $u->query_param_delete($key); - } + my $sigalg = uri_unescape($params{SigAlg}); - # Some IdPs (PingIdentity) seem to double encode the LogoutResponse URL - if ($self->sls_double_encoded_response) { - #if ($sigalg =~ m/%/) { - $signed = uri_decode($u->query); - $sig = uri_decode($sig); - $sigalg = uri_decode($sigalg); - $saml_request = uri_decode($u->query_param($self->param)); - } else { - $signed = $u->query; - $saml_request = $u->query_param($self->param); - } + my $encoded_sig = uri_unescape($params{Signature}); + my $sig = decode_base64($encoded_sig); - # What can we say about this one Microsoft Azure uses lower case in the - # URL encoding %2f not %2F. As it is signed as %2f the resulting signed - # needs to change it to lowercase if the application layer reencoded the URL. - if ($self->sls_force_lcase_url_encoding) { - # TODO: This is a hack. - $signed =~ s/(%..)/lc($1)/ge; + my @signed_parts; + for my $p ($self->param, qw(RelayState SigAlg)) { + push @signed_parts, join('=', $p, $params{$p}) if exists $params{$p}; } + my $signed = join('&', @signed_parts); - $sig = decode_base64($sig); - - $self->_verified($sigalg, $signed, $sig); + $self->_verify($sigalg, $signed, $sig); # unpack the SAML request - my $deflated = decode_base64($saml_request); + my $deflated = decode_base64(uri_unescape($params{$self->param})); my $request = ''; rawinflate \$deflated => \$request; # unpack the relaystate - my $relaystate = $u->query_param('RelayState'); - + my $relaystate = uri_unescape($params{'RelayState'}); return ($request, $relaystate); } diff --git a/t/17-lowercase-url-escaping.t b/t/17-lowercase-url-escaping.t index 14a36a68..cf750477 100644 --- a/t/17-lowercase-url-escaping.t +++ b/t/17-lowercase-url-escaping.t @@ -1,6 +1,5 @@ use Test::Lib; use Test::Net::SAML2; -use Test::More tests => 2; use Net::SAML2::Binding::Redirect; use File::Slurper qw/read_text/; @@ -15,13 +14,11 @@ my $redirect = Net::SAML2::Binding::Redirect->new( param => 'SAMLRequest', cert => read_text('t/net-saml2-cert.pem'), sig_hash => 'sha256', - sls_force_lcase_url_encoding => 1, ); my ($request, $relaystate) = $redirect->verify($url); -like($request, qr/NETSAML/, 'Good Signature if sls_force_lcase_url_encoding = 1'); +like($request, qr/NETSAML/, + "Good Signature because we now don't alter the input with URI anymore"); -$redirect->{sls_force_lcase_url_encoding} = 0; - -throws_ok( sub { $redirect->verify($url) }, qr/bad sig/, "Bad Signature if sls_force_lcase_url_encoding = 0"); +done_testing();