Merge pull request #65 from ziluvatar/sec-fix-wrapping-attack

Security Fix on XPath searches + extra validations to avoid wrapping attacks
auth0 · Nov 21, 2017 · 2ec5bd2 · 2ec5bd2
2 parents 749f3f3 + 1bdbe32
commit 2ec5bd2
Show file tree

Hide file tree

Showing 10 changed files with 194 additions and 49 deletions.
diff --git a/SECURITY-NOTICE.md b/SECURITY-NOTICE.md
@@ -0,0 +1,23 @@
+Security vulnerability details for passport-wsfed-saml2 < 3.0.5
+===============================================================
+
+A vulnerability has been discovered in the passport-wsfed-saml2 library affecting versions < 3.0.5. This vulnerability allows an attacker to impersonate another user and potentially elevate their privileges if the SAML identity provider:
+
+* signs SAML response and signs assertion
+* does not sign SAML response and signs assertion
+
+Developers using the passport-wsfed-saml2 Passport Strategy need to upgrade to the latest version: 3.0.5.
+
+Updated packages are available on npm. To ensure delivery of additional bug fixes moving forward, please make sure your `package.json` file is updated to take patch and minor level updates of our libraries. See below:
+
+```
+{
+  "dependencies": {
+    "passport-wsfed-saml2": "^3.0.5"
+  }
+}
+```
+
+## Upgrade Notes
+
+This fix patches the library that your application runs, but will not impact your users, their current state, or any existing sessions.
diff --git a/examples/login/app.js b/examples/login/app.js
@@ -53,7 +53,7 @@ passport.use(new wsfedsaml2(
     homeRealm: '', // specify an identity provider to avoid showing the idp selector
     identityProviderUrl: 'https://auth10-dev.accesscontrol.windows.net/v2/wsfederation',
     // setup either a certificate base64 encoded (cer) or just the thumbprint of the certificate if public key is embedded in the signature
-    
+
     //cert: 'MIIDFjCCAf6gAwIBAgIQDRRprj9lv5RBvaQdlFltDzANBgkqhkiG9w0BAQUFADAvMS0wKwYDVQQDEyRhdXRoMTAtZGV2LmFjY2Vzc2NvbnRyb2wud2luZG93cy5uZXQwHhcNMTEwOTIxMDMzMjMyWhcNMTIwOTIwMDkzMjMyWjAvMS0wKwYDVQQDEyRhdXRoMTAtZGV2LmFjY2Vzc2NvbnRyb2wud2luZG93cy5uZXQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCEIAEB/KKT3ehNMy2MQEyJIQ14CnZ8DC2FZgL5Gw3UBSdRb9JinK/gw7yOQtwKfJUqeoZaUSAAdcdbgqwVxOnMBfWiYX7DGlEznSfqYVnjOWjqqjpoe0h6RaOkdWovDtoidmqVV1tWRJFjkj895clPxkLpnqqcycfXtSdZen0SroGyirD2mhMc9ccLbJ3zRnBNjlvpo5zow1zYows09tNC2EhGROL/OS4JNRQnJRICZC+WkA7Igf3xb4btJOzIPYhFiqCGrd/81CHmAyEuNzyc60I5yomDQfZ91Eb5Uk3F7mlfAlYB2aZwDwldLSOlVE8G1E5xFexF/5KyPC4ShNodAgMBAAGjLjAsMAsGA1UdDwQEAwIE8DAdBgNVHQ4EFgQUyYfx/r0czsPgTzitqey+fGMQpkcwDQYJKoZIhvcNAQEFBQADggEBAB5dgQlM3tKS+/cjlvMCPjZH0Iqo/Wxecri3YWi2iVziZ/TQ3dSV+J/iTyduN7rJmFQzTsNERcsgyAwblwnEKXXvlWo8G/+VDIMh3zVPNQFKns5WPkfkhoSVlnZPTQ8zdXAcWgDXbCgvdqIPozdgL+4l0W0XVL1ugA4/hmMXh4TyNd9Qj7MWvlmwVjevpSqN4wG735jAZFHb/L/vvc91uKqP+JvLNj8tPFVxatzi56X1V8jBM61Hx1Z9D0RCDjtmcQVysVEylW9O6mNy6ZrhLm0q5yecWudfBbTKDqRoCHQRjrMU2c5q/ZFDtgjLim7FaNxFbgTyjeRCPclEhfemYVg='
     thumbprints: ['a3cff17cbf7e793a97861390eb698d00e9598537']
   },
@@ -91,7 +91,7 @@ app.use(bodyParser.urlencoded({
   extended: true
 }));
 app.use(methodOverride());
-app.use(session({ 
+app.use(session({
   secret: 'keyboard cat',
   resave: false,
  saveUninitialized: true

diff --git a/lib/passport-wsfed-saml2/saml.js b/lib/passport-wsfed-saml2/saml.js
@@ -40,9 +40,13 @@ SAML.prototype.validateSignature = function (xml, options, callback) {
   xml = utils.parseSamlResponse(xml);
 
   var signaturePath = this.options.signaturePath || options.signaturePath;
-  var signature = xpath.select(signaturePath, xml)[0];
-  if (!signature)
+  var signatures = xpath.select(signaturePath, xml);
+  if (signatures.length === 0) {
     return callback(new Error('Signature is missing (xpath: ' + signaturePath + ')'));
+  } else if (signatures.length > 1) {
+    return callback(new Error('Signature was found more than one time (xpath: ' + signaturePath + ')'));
+  }
+  var signature = signatures[0];
 
   var sig = new xmlCrypto.SignedXml(null, { idAttribute: 'AssertionID' });
   sig.keyInfoProvider = {
@@ -112,7 +116,7 @@ SAML.prototype.validateSignature = function (xml, options, callback) {
 };
 
 SAML.prototype.validateExpiration = function (samlAssertion, version) {
-  var conditions = xpath.select("//*[local-name(.)='Conditions']", samlAssertion);
+  var conditions = xpath.select(".//*[local-name(.)='Conditions']", samlAssertion);
   if (!conditions || conditions.length === 0) return true;
 
   var notBefore = new Date(conditions[0].getAttribute('NotBefore'));
@@ -131,9 +135,9 @@ SAML.prototype.validateExpiration = function (samlAssertion, version) {
 SAML.prototype.validateAudience = function (samlAssertion, realm, version) {
   var audience;
   if (version === '2.0') {
-    audience = xpath.select("//*[local-name(.)='Conditions']/*[local-name(.)='AudienceRestriction']/*[local-name(.)='Audience']", samlAssertion);
+    audience = xpath.select(".//*[local-name(.)='Conditions']/*[local-name(.)='AudienceRestriction']/*[local-name(.)='Audience']", samlAssertion);
   } else {
-    audience = xpath.select("//*[local-name(.)='Conditions']/*[local-name(.)='AudienceRestrictionCondition']/*[local-name(.)='Audience']", samlAssertion);
+    audience = xpath.select(".//*[local-name(.)='Conditions']/*[local-name(.)='AudienceRestrictionCondition']/*[local-name(.)='Audience']", samlAssertion);
   }
 
   if (!audience || audience.length === 0) return false;
@@ -152,7 +156,7 @@ SAML.prototype.validateNameQualifier = function (samlAssertion, issuer) {
       // Ignore validation if the format is not persistent or transient
       return true;
   }
-  
+
   return nameID.NameQualifier === issuer
 };
 
@@ -168,8 +172,8 @@ SAML.prototype.validateSPNameQualifier = function (samlAssertion, audience) {
       // Ignore validation if the format is not persistent or transient
       return true;
   }
-  
-  return nameID.SPNameQualifier === audience  
+
+  return nameID.SPNameQualifier === audience
 };
 
 // https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf
@@ -179,17 +183,17 @@ SAML.prototype.validateSPNameQualifier = function (samlAssertion, audience) {
 // example, this attribute might indicate that the assertion must be delivered to a particular network
 // endpoint in order to prevent an intermediary from redirecting it someplace else.
 SAML.prototype.validateRecipient = function(samlAssertion, recipientUrl){
-  var subjectConfirmationData = xpath.select("//*[local-name(.)='Subject']/*[local-name(.)='SubjectConfirmation']/*[local-name(.)='SubjectConfirmationData']", samlAssertion);
-  
+  var subjectConfirmationData = xpath.select(".//*[local-name(.)='Subject']/*[local-name(.)='SubjectConfirmation']/*[local-name(.)='SubjectConfirmationData']", samlAssertion);
+
   // subjectConfirmationData is optional in the spec. Only validate if the assertion contains a recipient
-  if (!subjectConfirmationData || subjectConfirmationData.length === 0){ 
+  if (!subjectConfirmationData || subjectConfirmationData.length === 0){
     return true;
   }
 
   var recipient = subjectConfirmationData[0].getAttribute('Recipient');
 
   var valid = !recipient || recipient === recipientUrl;
-  
+
   if (!valid){
     this.eventEmitter.emit('recipientValidationFailed', {
        configuredRecipient: recipientUrl,
@@ -202,26 +206,26 @@ SAML.prototype.validateRecipient = function(samlAssertion, recipientUrl){
 
 SAML.prototype.parseAttributes = function (samlAssertion, version) {
   function getAttributes(samlAssertion) {
-    var attributes = xpath.select("//*[local-name(.)='AttributeStatement']/*[local-name(.)='Attribute']", samlAssertion);
+    var attributes = xpath.select(".//*[local-name(.)='AttributeStatement']/*[local-name(.)='Attribute']", samlAssertion);
     return attributes;
   }
 
   function getSessionIndex(samlAssertion) {
-    var authnStatement = xpath.select("//*[local-name(.)='AuthnStatement']", samlAssertion);
+    var authnStatement = xpath.select(".//*[local-name(.)='AuthnStatement']", samlAssertion);
     var sessionIndex = authnStatement.length > 0 && authnStatement[0].attributes.length > 0 ?
                     authnStatement[0].getAttribute('SessionIndex') : undefined;
     return sessionIndex || undefined;
   }
 
   function getNameID11(samlAssertion) {
-    var nameId = xpath.select("//*[local-name(.)='AuthenticationStatement']/*[local-name(.)='Subject']/*[local-name(.)='NameIdentifier']", samlAssertion);
-    
+    var nameId = xpath.select(".//*[local-name(.)='AuthenticationStatement']/*[local-name(.)='Subject']/*[local-name(.)='NameIdentifier']", samlAssertion);
+
     if (nameId.length === 0) {
       // only for backward compatibility with adfs
-      nameId = xpath.select("//*[local-name(.)='AttributeStatement']/*[local-name(.)='Subject']/*[local-name(.)='NameIdentifier']", samlAssertion);
+      nameId = xpath.select(".//*[local-name(.)='AttributeStatement']/*[local-name(.)='Subject']/*[local-name(.)='NameIdentifier']", samlAssertion);
       if (nameId.length === 0) return;
     }
-    
+
     return nameId[0].textContent;
   }
 
@@ -239,7 +243,7 @@ SAML.prototype.parseAttributes = function (samlAssertion, version) {
   }
 
   function getAuthContext20(samlAssertion) {
-    var authnContext = xpath.select("//*[local-name(.)='AuthnStatement']/*[local-name(.)='AuthnContext']/*[local-name(.)='AuthnContextClassRef']", samlAssertion);
+    var authnContext = xpath.select(".//*[local-name(.)='AuthnStatement']/*[local-name(.)='AuthnContext']/*[local-name(.)='AuthnContextClassRef']", samlAssertion);
     if (authnContext.length === 0) return;
     return authnContext[0].textContent;
   }
@@ -296,7 +300,7 @@ SAML.prototype.validateSamlAssertion = function (samlAssertion, callback) {
   self.validateSignature(samlAssertion.toString(), {
     cert: self.options.cert,
     thumbprints: self.options.thumbprints,
-    signaturePath: "//*[local-name(.)='Assertion']/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']" }, function(err) {
+    signaturePath: ".//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']" }, function(err) {
     if (err) return callback(err);
 
     self.parseAssertion(samlAssertion, callback);
@@ -308,7 +312,7 @@ SAML.prototype.parseAssertion = function(samlAssertion, callback) {
   if (self.options.extractSAMLAssertion){
     samlAssertion = self.options.extractSAMLAssertion(samlAssertion);
   }
-  
+
   samlAssertion = utils.parseSamlResponse(samlAssertion);
 
   if (!samlAssertion.getAttribute)
@@ -341,13 +345,13 @@ SAML.prototype.parseAssertion = function(samlAssertion, callback) {
 
   var issuer;
   if (version === '2.0') {
-    var issuerNode = xpath.select("//*[local-name(.)='Issuer']", samlAssertion);
+    var issuerNode = xpath.select(".//*[local-name(.)='Issuer']", samlAssertion);
     if (issuerNode.length > 0) issuer = issuerNode[0].textContent;
   } else {
     issuer = samlAssertion.getAttribute('Issuer');
   }
 
-  
+
   this.eventEmitter.emit('parseAssertion', {
       issuer: issuer,
       version: version,
@@ -363,7 +367,7 @@ SAML.prototype.parseAssertion = function(samlAssertion, callback) {
   // Validate the SP name qualifier in the NameID element if found with the issuer
   if (self.options.checkSPNameQualifier && !self.validateSPNameQualifier(samlAssertion, self.options.realm)){
     return callback(new Error('SPNameQualifier attribute in the NameID element does not match ' + self.options.realm), null);
-  }  
+  }
 
   if (!profile.email && profile['http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress']) {
     profile.email = profile['http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress'];
@@ -373,7 +377,7 @@ SAML.prototype.parseAssertion = function(samlAssertion, callback) {
 };
 
 function getNameID20(samlAssertion) {
-  var nameId = xpath.select("//*[local-name(.)='Subject']/*[local-name(.)='NameID']", samlAssertion);
+  var nameId = xpath.select(".//*[local-name(.)='Subject']/*[local-name(.)='NameID']", samlAssertion);
   if (nameId.length === 0) return;
   var element = nameId[0];
   var result = {

diff --git a/lib/passport-wsfed-saml2/samlp.js b/lib/passport-wsfed-saml2/samlp.js
@@ -248,17 +248,21 @@ Samlp.prototype = {
       callback(null, assertion);
     };
 
-    var assertions = samlpResponse.getElementsByTagNameNS(saml2Namespace, 'Assertion');
-    if (assertions.length > 1) {
-      return done(new Error('A SAMLResponse can contains only one Assertion element.'));
+    var foundAssertions = xpath.select("//*[local-name(.)='Assertion']", samlpResponse);
+    if (foundAssertions.length > 1) {
+      return done(new Error('A SAMLResponse can contain only one Assertion element.'));
     }
 
+    // After being sure no more "Assertion" elements are found, we extract it from the expected place
+    var assertions = xpath.select("/*[local-name(.)='Response']/*[local-name(.)='Assertion' and namespace-uri(.)='" + saml2Namespace + "']", samlpResponse);
     var token = assertions[0];
+
     if (!token) {
       // check for encrypted assertion
-      var encryptedAssertion = samlpResponse.getElementsByTagNameNS(saml2Namespace, 'EncryptedAssertion');
+      var encryptedAssertionPath = "/*[local-name(.)='Response']/*[local-name(.)='EncryptedAssertion' and namespace-uri(.)='" + saml2Namespace + "']";
+      var encryptedAssertion = xpath.select(encryptedAssertionPath, samlpResponse);
       if (encryptedAssertion.length > 1) {
-        return done(new Error('A SAMLResponse can contains only one EncryptedAssertion element.'));
+        return done(new Error('A SAMLResponse can contain only one EncryptedAssertion element.'));
       }
 
       var encryptedToken = encryptedAssertion[0];
@@ -316,10 +320,14 @@ Samlp.prototype = {
     samlResponse = utils.parseSamlResponse(samlResponse);
 
     // Check that the saml Resopnse actually has a Response object
-    var responseXML = xpath.select("//*[local-name(.)='Response']", samlResponse)[0];
-    if (!responseXML){
+    var responseXMLs = xpath.select("//*[local-name(.)='Response']", samlResponse);
+    if (responseXMLs.length === 0) {
       return callback(new Error('XML is not a valid saml response'));
     }
+    if (responseXMLs.length > 1) {
+      return callback(new Error('SAMLResponse should be unique'));
+    }
+    var responseXML = responseXMLs[0];
 
     self.isValidResponseID(responseXML.getAttribute('ID'), function(err){
       if (err && self.options.checkResponseID) { return callback(err); }
@@ -349,7 +357,7 @@ Samlp.prototype = {
             return callback(new Error('Destination endpoint ' + destination + ' did not match ' + self.options.destinationUrl));
           }
         }
-        
+
         // check status
         var samlStatus = self.getSamlStatus(samlResponse);
 
@@ -369,11 +377,16 @@ Samlp.prototype = {
             return callback(new Error('saml response does not contain an Assertion element (Status: ' + samlStatus.code + ')'));
           }
 
-          var samlResponseSignaturePath = "//*[local-name(.)='Response']/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']";
+          var samlResponseSignaturePath = "/*[local-name(.)='Response']/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']";
           var isResponseSigned = xpath.select(samlResponseSignaturePath, samlResponse).length > 0;
-          var samlAssertionSignaturePath = "//*[local-name(.)='Assertion']/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']";
+          var samlAssertionSignaturePath = ".//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']";
           var isAssertionSigned =  xpath.select(samlAssertionSignaturePath, assertion).length > 0;
 
+          self.eventEmitter.emit('SAMLResponse:signatures', {
+            isResponseSigned: isResponseSigned,
+            isAssertionSigned: isAssertionSigned
+          });
+
           if (!isResponseSigned && !isAssertionSigned) {
             return callback(new Error('neither the response nor the assertion are signed'));
           }

diff --git a/lib/passport-wsfed-saml2/strategy.js b/lib/passport-wsfed-saml2/strategy.js
@@ -65,7 +65,7 @@ WsFedSaml2Strategy.prototype._authenticate_saml = function (req, state) {
 
   self._wsfed.retrieveToken(req, function(err, token) {
     if (err) return self.fail(err, err.status || 400);
-    
+
     self.options.recipientUrl = self.options.recipientUrl || getReqUrl(req);
 
     self._saml.validateSamlAssertion(token, function (err, profile) {
@@ -214,7 +214,7 @@ WsFedSaml2Strategy.prototype.authenticate = function (req, opts) {
         }
 
         var samlResponseDom = utils.parseSamlResponse(samlResponse);
-       
+
         // If options are not set, we set the expected value from the request object
         var req_full_url = getReqUrl(req);
         self.options.destinationUrl = self.options.destinationUrl || req_full_url;

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "passport-wsfed-saml2",
-  "version": "3.0.4",
+  "version": "3.0.5",
   "description": "SAML2 Protocol and WS-Fed library",
   "scripts": {
     "test": "mocha --reporter spec --recursive"