fixes #78: make sure the correct Signature elements are verified

- checks that the found Signature elements match their expected schema
  locations, and that there's only one of each
- make sure the expected Signatures are always verified
- a few other small hardenings

Author: miszobi

core/src/main/java/com/onelogin/saml2/authn/SamlResponse.java

@@ -271,7 +271,7 @@ public boolean isValid(String requestId) {
 					}
 				}
 
-				if (!Util.validateSign(documentToValidate, cert, fingerprint, alg)) {
+				if (!Util.validateSign(documentToValidate, cert, fingerprint, alg, settings.getWantMessagesSigned(), settings.getWantAssertionsSigned())) {
 					throw new Exception("Signature validation failed. SAML Response rejected");
 				}
 			}
@@ -676,6 +676,22 @@ public ArrayList<String> processSignedElements() throws Exception {
 			if (!signedElement.equals("Response") && !signedElement.equals("Assertion")) {
 				throw new Exception("Invalid Signature Element " + signedElement + " SAML Response rejected");
 			}
+
+			// check that the signed elements found here, are the ones that will be verified
+			// by com.onelogin.saml2.util.Util.validateSign()
+			if (signedElement.equals("Response")) {
+				final NodeList expectedSignatureNode = query(Util.RESPONSE_SIGNATURE_XPATH, null);
+				if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
+					throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
+				}
+			}
+
+			if (signedElement.equals("Assertion")) {
+				final NodeList expectedSignatureNode = query(Util.ASSERTION_SIGNATURE_XPATH, null);
+				if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
+					throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
+				}
+			}
 
 			// Check that reference URI matches the parent ID and no duplicate References or IDs
 			Node idNode = signNode.getParentNode().getAttributes().getNamedItem("ID");
@@ -689,8 +705,8 @@ public ArrayList<String> processSignedElements() throws Exception {
 			}
 			verifiedIds.add(idValue);
 
-			NodeList refNodes = Util.query(null, ".//ds:Reference", signNode);
-			if (refNodes.getLength() > 0) {
+			NodeList refNodes = Util.query(null, "ds:SignedInfo/ds:Reference", signNode);
+			if (refNodes.getLength() == 1) {
 				Node refNode = refNodes.item(0);
 				Node seiNode = refNode.getAttributes().getNamedItem("URI");
 				if (seiNode != null && seiNode.getNodeValue() != null && !seiNode.getNodeValue().isEmpty()) {
@@ -704,6 +720,10 @@ public ArrayList<String> processSignedElements() throws Exception {
 					}
 					verifiedSeis.add(sei);
 				}
+			} else {
+				// Signatures MUST contain a single <ds:Reference> containing a same-document reference to the ID
+				// attribute value of the root element of the assertion or protocol message being signed
+				throw new Exception("Unexpected number of Reference nodes found for signature. SAML Response rejected.");
 			}
 
 			signedElements.add(signedElement);
@@ -809,13 +829,10 @@ public String getError() {
 	 *
 	 */
 	private NodeList queryAssertion(String assertionXpath) throws XPathExpressionException {
-        String assertionExpr;
-
-        assertionExpr = "/saml:Assertion";
-
-        String signatureExpr = "ds:Signature/ds:SignedInfo/ds:Reference";
+        final String assertionExpr = "/saml:Assertion";
+        final String signatureExpr = "ds:Signature/ds:SignedInfo/ds:Reference";
 
-        String nameQuery = "";
+        String nameQuery;
         String signedAssertionQuery = "/samlp:Response" + assertionExpr + "/" + signatureExpr;
         NodeList nodeList = query(signedAssertionQuery, null);
         if (nodeList.getLength() == 0 ) {
@@ -835,19 +852,6 @@ private NodeList queryAssertion(String assertionXpath) throws XPathExpressionExc
                 // On this case there is no element signed, the query will work but
                 // the response validation will throw and error.
             	nameQuery = "/samlp:Response";
-            	
-            	// If we want to change this behaviour, uncomment this block.
-            	// but test should be updated then.
-            	
-            	// Trick in order to return empty NodeList (not instanciable)
-            	/*
-            	XPath xpath = XPathFactory.newInstance().newXPath();
-            	NodeList noresult = null;
-				try {
-					noresult = (NodeList) xpath.evaluate("/noresult", Util.convertStringToDocument("<null></null>"), XPathConstants.NODESET);
-				} catch (ParserConfigurationException | SAXException | IOException e) {}
-            	return noresult;
-            	*/
             }
             nameQuery += assertionExpr;
         } else {  // there is a signed assertion

core/src/main/java/com/onelogin/saml2/util/Util.java

@@ -96,6 +96,10 @@ public final class Util {
     private static final DateTimeFormatter DATE_TIME_FORMAT = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss'Z'").withZone(DateTimeZone.UTC);
 	private static final DateTimeFormatter DATE_TIME_FORMAT_MILLS = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").withZone(DateTimeZone.UTC);
 	public static final String UNIQUE_ID_PREFIX = "ONELOGIN_";
+	public static final String RESPONSE_SIGNATURE_XPATH = "/samlp:Response/ds:Signature";
+	public static final String ASSERTION_SIGNATURE_XPATH = "/samlp:Response/saml:Assertion/ds:Signature";
+
+	private static final Logger log = LoggerFactory.getLogger(Util.class);
 
 	/**
 	 * This function load an XML string in a save way. Prevent XEE/XXE Attacks
@@ -220,9 +224,13 @@ public static boolean validateXML(Document xmlDocument, URL schemaUrl) throws Ex
 			Source xmlSource = new DOMSource(xmlDocument);
 			validator.validate(xmlSource);
 
-			return !errorAcumulator.hasError();
+			final boolean isValid = !errorAcumulator.hasError();
+			if (!isValid) {
+				LOGGER.warn("Errors found when validating SAML response with schema: " + errorAcumulator.getErrorXML());
+			}
+			return isValid;
 		} catch (Exception e) {
-			LOGGER.debug("Error executing validateXML: " + e.getMessage(), e);
+			LOGGER.warn("Error executing validateXML: " + e.getMessage(), e);
 			return false;
 		}
 	}
@@ -785,25 +793,44 @@ public static String signatureAlgConversion(String sign) {
      *               The fingerprint of the public certificate
      * @param alg
      *               The signature algorithm method
+     * @param wantAssertionSigned whether the caller requires assertions to be signed
+     * @param wantResponseSigned whether the caller requires responses to be signed
      *
-     * @return True if the sign is valid, false otherwise.
+     * @return True if the required signatures are present and the present signatures are valid, false otherwise.
      */
-    public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg) {
-        NodeList signNodesToValidate;
+    public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg,
+									   boolean wantResponseSigned, boolean wantAssertionSigned) {
 		try {
-			signNodesToValidate = query(doc, "/samlp:Response/ds:Signature");
-			if (signNodesToValidate.getLength() == 0) {
-				signNodesToValidate = query(doc, "/samlp:Response/saml:Assertion/ds:Signature");
-			}
+			boolean validResponseSignature = checkSignature(doc, cert, fingerprint, alg, wantResponseSigned, RESPONSE_SIGNATURE_XPATH);
+			boolean validAssertionSignature = checkSignature(doc, cert, fingerprint, alg, wantAssertionSigned, ASSERTION_SIGNATURE_XPATH);
 
-			if (signNodesToValidate.getLength() == 1) {
-				return validateSignNode(signNodesToValidate.item(0), cert, fingerprint, alg);
-			}
-		} catch (XPathExpressionException e) {}
+			return validResponseSignature && validAssertionSignature;
+		} catch (XPathExpressionException e) {
+			log.warn("Failed to find signature nodes", e);
+		}
 		return false;
     }
 
-    /**
+	private static boolean checkSignature(Document doc, X509Certificate cert, String fingerprint, String alg, boolean required, final String xpath) throws XPathExpressionException {
+		final NodeList responseSignature = query(doc, xpath);
+		final int signatureCount = responseSignature.getLength();
+		if (signatureCount > 1) {
+			log.warn("Unexpected number of signatures found matching " + xpath + ": " + signatureCount);
+			return false;
+		}
+
+		if (signatureCount == 0) {
+			if (required) {
+				log.warn("No signature matching " + xpath + ", found but was required");
+				return false;
+			}
+			return true;
+		}
+
+		return validateSignNode(responseSignature.item(0), cert, fingerprint, alg);
+	}
+
+	/**
      * Validate signature (Metadata).
      *
      * @param doc

core/src/test/java/com/onelogin/saml2/test/authn/AuthnResponseTest.java

@@ -609,6 +609,23 @@ public void testDoesNotAllowSignatureWrappingAttack4() throws Exception {
 		assertEquals("someone@example.org", samlResponse.getNameId());
 	}
 
+	@Test
+	public void testValidatesTheExpectedSignatures() throws Exception {
+		// having
+		Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
+		settings.setWantAssertionsSigned(true);
+		settings.setWantMessagesSigned(true);
+
+		String samlResponseEncoded = Util.base64encoder(Util.getFileAsString("data/responses/invalids/attacks/response_with_spoofed_response_signature.xml"));
+
+		// when
+		SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
+
+		// then
+		assertFalse(samlResponse.isValid());
+		assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
+	}
+
 	/**
 	 * Tests the getSessionNotOnOrAfter method of SamlResponse
 	 *
@@ -851,6 +868,7 @@ public void testIsValidWrongEncryptedID() throws Exception {
 		Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
 		String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/response_encrypted_subconfirm_as_nameid.xml.base64");
 		settings.setStrict(false);
+		settings.setWantAssertionsSigned(false);
 		SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
 		assertTrue(samlResponse.isValid());
 		String nameId = samlResponse.getNameId();
@@ -1630,17 +1648,17 @@ public void testIsInValidSign() throws Exception {
 		samlResponseEncoded = Util.getFileAsString("data/responses/invalids/triple_signed_response.xml.base64");
 		samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
 		assertFalse(samlResponse.isValid());
-		assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
+		assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
 
 		samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_assertion_response_with_2signatures.xml.base64");
 		samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
 		assertFalse(samlResponse.isValid());
-		assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
+		assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
 
 		samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_message_response_with_2signatures.xml.base64");
 		samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
 		assertFalse(samlResponse.isValid());
-		assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
+		assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
 
 		samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_signed_element.xml.base64");
 		samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));