Skip to content

Commit 6a36944

Browse files
pitbulkpitbulk
authored
Merge pull request #79 from miszobi/issue/78-avoid-response-signature-spoofing
fixes #78: make sure the correct Signature elements are verified
2 parents cc296ae + bb18cf2 commit 6a36944

7 files changed

Lines changed: 238 additions & 137 deletions

File tree

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

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,8 @@ public boolean isValid(String requestId) {
160160

161161
ArrayList<String> signedElements = processSignedElements();
162162

163+
final boolean hasSignedAssertion = signedElements.contains("Assertion");
164+
final boolean hasSignedResponse = signedElements.contains("Response");
163165
if (settings.isStrict()) {
164166
if (settings.getWantXMLValidation()) {
165167
if (!Util.validateXML(samlResponseDocument, SchemaFactory.SAML_SCHEMA_PROTOCOL_2_0)) {
@@ -244,11 +246,11 @@ public boolean isValid(String requestId) {
244246

245247
validateSubjectConfirmation(responseInResponseTo);
246248

247-
if (settings.getWantAssertionsSigned() && !signedElements.contains("Assertion")) {
249+
if (settings.getWantAssertionsSigned() && !hasSignedAssertion) {
248250
throw new Exception("The Assertion of the Response is not signed and the SP requires it");
249251
}
250252

251-
if (settings.getWantMessagesSigned() && !signedElements.contains("Response")) {
253+
if (settings.getWantMessagesSigned() && !hasSignedResponse) {
252254
throw new Exception("The Message of the Response is not signed and the SP requires it");
253255
}
254256
}
@@ -260,18 +262,12 @@ public boolean isValid(String requestId) {
260262
String fingerprint = settings.getIdpCertFingerprint();
261263
String alg = settings.getIdpCertFingerprintAlgorithm();
262264

263-
Document documentToValidate;
264-
if (signedElements.contains("Response")) {
265-
documentToValidate = samlResponseDocument;
266-
} else {
267-
if (encrypted) {
268-
documentToValidate = decryptedDocument;
269-
} else {
270-
documentToValidate = samlResponseDocument;
271-
}
265+
if (hasSignedResponse && !Util.validateSign(samlResponseDocument, cert, fingerprint, alg, Util.RESPONSE_SIGNATURE_XPATH)) {
266+
throw new Exception("Signature validation failed. SAML Response rejected");
272267
}
273268

274-
if (!Util.validateSign(documentToValidate, cert, fingerprint, alg)) {
269+
final Document documentToCheckAssertion = encrypted ? decryptedDocument : samlResponseDocument;
270+
if (hasSignedAssertion && !Util.validateSign(documentToCheckAssertion, cert, fingerprint, alg, Util.ASSERTION_SIGNATURE_XPATH)) {
275271
throw new Exception("Signature validation failed. SAML Response rejected");
276272
}
277273
}
@@ -676,6 +672,22 @@ public ArrayList<String> processSignedElements() throws Exception {
676672
if (!signedElement.equals("Response") && !signedElement.equals("Assertion")) {
677673
throw new Exception("Invalid Signature Element " + signedElement + " SAML Response rejected");
678674
}
675+
676+
// check that the signed elements found here, are the ones that will be verified
677+
// by com.onelogin.saml2.util.Util.validateSign()
678+
if (signedElement.equals("Response")) {
679+
final NodeList expectedSignatureNode = query(Util.RESPONSE_SIGNATURE_XPATH, null);
680+
if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
681+
throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
682+
}
683+
}
684+
685+
if (signedElement.equals("Assertion")) {
686+
final NodeList expectedSignatureNode = query(Util.ASSERTION_SIGNATURE_XPATH, null);
687+
if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
688+
throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
689+
}
690+
}
679691

680692
// Check that reference URI matches the parent ID and no duplicate References or IDs
681693
Node idNode = signNode.getParentNode().getAttributes().getNamedItem("ID");
@@ -689,8 +701,8 @@ public ArrayList<String> processSignedElements() throws Exception {
689701
}
690702
verifiedIds.add(idValue);
691703

692-
NodeList refNodes = Util.query(null, ".//ds:Reference", signNode);
693-
if (refNodes.getLength() > 0) {
704+
NodeList refNodes = Util.query(null, "ds:SignedInfo/ds:Reference", signNode);
705+
if (refNodes.getLength() == 1) {
694706
Node refNode = refNodes.item(0);
695707
Node seiNode = refNode.getAttributes().getNamedItem("URI");
696708
if (seiNode != null && seiNode.getNodeValue() != null && !seiNode.getNodeValue().isEmpty()) {
@@ -704,6 +716,10 @@ public ArrayList<String> processSignedElements() throws Exception {
704716
}
705717
verifiedSeis.add(sei);
706718
}
719+
} else {
720+
// Signatures MUST contain a single <ds:Reference> containing a same-document reference to the ID
721+
// attribute value of the root element of the assertion or protocol message being signed
722+
throw new Exception("Unexpected number of Reference nodes found for signature. SAML Response rejected.");
707723
}
708724

709725
signedElements.add(signedElement);
@@ -809,13 +825,10 @@ public String getError() {
809825
*
810826
*/
811827
private NodeList queryAssertion(String assertionXpath) throws XPathExpressionException {
812-
String assertionExpr;
813-
814-
assertionExpr = "/saml:Assertion";
815-
816-
String signatureExpr = "ds:Signature/ds:SignedInfo/ds:Reference";
828+
final String assertionExpr = "/saml:Assertion";
829+
final String signatureExpr = "ds:Signature/ds:SignedInfo/ds:Reference";
817830

818-
String nameQuery = "";
831+
String nameQuery;
819832
String signedAssertionQuery = "/samlp:Response" + assertionExpr + "/" + signatureExpr;
820833
NodeList nodeList = query(signedAssertionQuery, null);
821834
if (nodeList.getLength() == 0 ) {
@@ -835,19 +848,6 @@ private NodeList queryAssertion(String assertionXpath) throws XPathExpressionExc
835848
// On this case there is no element signed, the query will work but
836849
// the response validation will throw and error.
837850
nameQuery = "/samlp:Response";
838-
839-
// If we want to change this behaviour, uncomment this block.
840-
// but test should be updated then.
841-
842-
// Trick in order to return empty NodeList (not instanciable)
843-
/*
844-
XPath xpath = XPathFactory.newInstance().newXPath();
845-
NodeList noresult = null;
846-
try {
847-
noresult = (NodeList) xpath.evaluate("/noresult", Util.convertStringToDocument("<null></null>"), XPathConstants.NODESET);
848-
} catch (ParserConfigurationException | SAXException | IOException e) {}
849-
return noresult;
850-
*/
851851
}
852852
nameQuery += assertionExpr;
853853
} else { // there is a signed assertion

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

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,10 @@ public final class Util {
9696
private static final DateTimeFormatter DATE_TIME_FORMAT = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss'Z'").withZone(DateTimeZone.UTC);
9797
private static final DateTimeFormatter DATE_TIME_FORMAT_MILLS = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").withZone(DateTimeZone.UTC);
9898
public static final String UNIQUE_ID_PREFIX = "ONELOGIN_";
99+
public static final String RESPONSE_SIGNATURE_XPATH = "/samlp:Response/ds:Signature";
100+
public static final String ASSERTION_SIGNATURE_XPATH = "/samlp:Response/saml:Assertion/ds:Signature";
101+
102+
private static final Logger log = LoggerFactory.getLogger(Util.class);
99103

100104
/**
101105
* 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
220224
Source xmlSource = new DOMSource(xmlDocument);
221225
validator.validate(xmlSource);
222226

223-
return !errorAcumulator.hasError();
227+
final boolean isValid = !errorAcumulator.hasError();
228+
if (!isValid) {
229+
LOGGER.warn("Errors found when validating SAML response with schema: " + errorAcumulator.getErrorXML());
230+
}
231+
return isValid;
224232
} catch (Exception e) {
225-
LOGGER.debug("Error executing validateXML: " + e.getMessage(), e);
233+
LOGGER.warn("Error executing validateXML: " + e.getMessage(), e);
226234
return false;
227235
}
228236
}
@@ -774,36 +782,29 @@ public static String signatureAlgConversion(String sign) {
774782
return convertedSignatureAlg;
775783
}
776784

777-
/**
778-
* Validate signature (Message or Assertion).
779-
*
780-
* @param doc
781-
* The document we should validate
782-
* @param cert
783-
* The public certificate
784-
* @param fingerprint
785-
* The fingerprint of the public certificate
786-
* @param alg
787-
* The signature algorithm method
788-
*
789-
* @return True if the sign is valid, false otherwise.
790-
*/
791-
public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg) {
792-
NodeList signNodesToValidate;
785+
/**
786+
* Validate the signature pointed to by the xpath
787+
*
788+
* @param doc The document we should validate
789+
* @param cert The public certificate
790+
* @param fingerprint The fingerprint of the public certificate
791+
* @param alg The signature algorithm method
792+
* @param xpath the xpath of the ds:Signture node to validate
793+
*
794+
* @return True if the signature exists and is valid, false otherwise.
795+
*/
796+
public static boolean validateSign(final Document doc, final X509Certificate cert, final String fingerprint,
797+
final String alg, final String xpath) {
793798
try {
794-
signNodesToValidate = query(doc, "/samlp:Response/ds:Signature");
795-
if (signNodesToValidate.getLength() == 0) {
796-
signNodesToValidate = query(doc, "/samlp:Response/saml:Assertion/ds:Signature");
797-
}
798-
799-
if (signNodesToValidate.getLength() == 1) {
800-
return validateSignNode(signNodesToValidate.item(0), cert, fingerprint, alg);
801-
}
802-
} catch (XPathExpressionException e) {}
803-
return false;
804-
}
799+
final NodeList signatures = query(doc, xpath);
800+
return signatures.getLength() == 1 && validateSignNode(signatures.item(0), cert, fingerprint, alg);
801+
} catch (XPathExpressionException e) {
802+
log.warn("Failed to find signature nodes", e);
803+
return false;
804+
}
805+
}
805806

806-
/**
807+
/**
807808
* Validate signature (Metadata).
808809
*
809810
* @param doc

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

Lines changed: 50 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,23 @@ public void testDoesNotAllowSignatureWrappingAttack4() throws Exception {
609609
assertEquals("someone@example.org", samlResponse.getNameId());
610610
}
611611

612+
@Test
613+
public void testValidatesTheExpectedSignatures() throws Exception {
614+
// having
615+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
616+
settings.setWantAssertionsSigned(true);
617+
settings.setWantMessagesSigned(true);
618+
619+
String samlResponseEncoded = Util.base64encoder(Util.getFileAsString("data/responses/invalids/attacks/response_with_spoofed_response_signature.xml"));
620+
621+
// when
622+
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
623+
624+
// then
625+
assertFalse(samlResponse.isValid());
626+
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
627+
}
628+
612629
/**
613630
* Tests the getSessionNotOnOrAfter method of SamlResponse
614631
*
@@ -851,6 +868,7 @@ public void testIsValidWrongEncryptedID() throws Exception {
851868
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
852869
String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/response_encrypted_subconfirm_as_nameid.xml.base64");
853870
settings.setStrict(false);
871+
settings.setWantAssertionsSigned(false);
854872
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
855873
assertTrue(samlResponse.isValid());
856874
String nameId = samlResponse.getNameId();
@@ -1431,39 +1449,44 @@ public void testIsValid2() throws Exception {
14311449
* @see com.onelogin.saml2.authn.SamlResponse#isValid
14321450
*/
14331451
@Test
1434-
public void testIsValidEnc() throws Exception {
1452+
public void testIsValid_doubleSignedEncrypted() throws Exception {
14351453
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1436-
settings.setWantAssertionsSigned(false);
1437-
settings.setWantMessagesSigned(false);
1454+
settings.setWantAssertionsSigned(true);
1455+
settings.setWantMessagesSigned(true);
14381456
String samlResponseEncoded = Util.getFileAsString("data/responses/double_signed_encrypted_assertion.xml.base64");
14391457

1440-
settings.setStrict(false);
1441-
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1442-
assertTrue(samlResponse.isValid());
1443-
1444-
settings.setStrict(true);
1445-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1446-
assertTrue(samlResponse.isValid());
1458+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1459+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1460+
}
14471461

1448-
samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_encrypted_assertion.xml.base64");
1462+
@Test
1463+
public void testIsValid_signedResponseEncryptedAssertion() throws Exception {
1464+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1465+
settings.setWantAssertionsSigned(false);
1466+
settings.setWantMessagesSigned(true);
14491467

1450-
settings.setStrict(false);
1451-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1452-
assertTrue(samlResponse.isValid());
1468+
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_encrypted_assertion.xml.base64");
14531469

1454-
settings.setStrict(true);
1455-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1456-
assertTrue(samlResponse.isValid());
1470+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1471+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1472+
settings.setWantAssertionsSigned(true);
1473+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1474+
assertResponseValid(settings, samlResponseEncoded, true, false, "The Assertion of the Response is not signed and the SP requires it");
1475+
}
14571476

1458-
samlResponseEncoded = Util.getFileAsString("data/responses/signed_encrypted_assertion.xml.base64");
1477+
@Test
1478+
public void testIsValid_signedEncryptedAssertion() throws Exception {
1479+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1480+
settings.setWantAssertionsSigned(true);
1481+
settings.setWantMessagesSigned(false);
14591482

1460-
settings.setStrict(false);
1461-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1462-
assertTrue(samlResponse.isValid());
1483+
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_encrypted_assertion.xml.base64");
14631484

1464-
settings.setStrict(true);
1465-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1466-
assertTrue(samlResponse.isValid());
1485+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1486+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1487+
settings.setWantMessagesSigned(true);
1488+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1489+
assertResponseValid(settings, samlResponseEncoded, true, false, "The Message of the Response is not signed and the SP requires it");
14671490
}
14681491

14691492
/**
@@ -1630,17 +1653,17 @@ public void testIsInValidSign() throws Exception {
16301653
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/triple_signed_response.xml.base64");
16311654
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
16321655
assertFalse(samlResponse.isValid());
1633-
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
1656+
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
16341657

16351658
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_assertion_response_with_2signatures.xml.base64");
16361659
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
16371660
assertFalse(samlResponse.isValid());
1638-
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
1661+
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
16391662

16401663
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_message_response_with_2signatures.xml.base64");
16411664
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
16421665
assertFalse(samlResponse.isValid());
1643-
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
1666+
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
16441667

16451668
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_signed_element.xml.base64");
16461669
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));

0 commit comments

Comments
 (0)