Merge pull request #85 from waterkip/GH-redirect-verify_fixes

BREAKING CHANGE: Verify the SAMLResponse based on the raw query string

Author: timlegge

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",

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";

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.
@@ -220,7 +215,7 @@ sub sign {
     return $u->as_string;
 }
 
-sub _verified {
+sub _verify {
     my ($self, $sigalg, $signed, $sig) = @_;
 
     foreach my $crt (@{$self->cert}) {
@@ -237,84 +232,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<URI> 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);
 }
 

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();