Skip to content

Commit dcc0b23

Browse files
pitbulkpitbulk
committed
Improve Signature validation process
1 parent 19c3b25 commit dcc0b23

2 files changed

Lines changed: 69 additions & 54 deletions

File tree

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

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,12 @@ 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");
163+
String responseTag = "{" + Constants.NS_SAMLP + "}Response";
164+
String assertionTag = "{" + Constants.NS_SAML + "}Assertion";
165+
166+
final boolean hasSignedResponse = signedElements.contains(responseTag);
167+
final boolean hasSignedAssertion = signedElements.contains(assertionTag);
168+
165169
if (settings.isStrict()) {
166170
if (settings.getWantXMLValidation()) {
167171
if (!Util.validateXML(samlResponseDocument, SchemaFactory.SAML_SCHEMA_PROTOCOL_2_0)) {
@@ -267,7 +271,7 @@ public boolean isValid(String requestId) {
267271
}
268272
}
269273

270-
if (signedElements.isEmpty()) {
274+
if (signedElements.isEmpty() || (!hasSignedAssertion && !hasSignedResponse)) {
271275
throw new Exception("No Signature found. SAML Response rejected");
272276
} else {
273277
X509Certificate cert = settings.getIdpx509cert();
@@ -403,7 +407,7 @@ public HashMap<String,String> getNameIdData() throws Exception {
403407

404408
if (nameIdElem != null) {
405409
String value = nameIdElem.getTextContent();
406-
if (value.isEmpty()) {
410+
if (settings.isStrict() && value.isEmpty()) {
407411
throw new Exception("An empty NameID value found");
408412
}
409413

@@ -415,7 +419,7 @@ public HashMap<String,String> getNameIdData() throws Exception {
415419
if (nameIdElem.hasAttribute("SPNameQualifier")) {
416420
String spId = settings.getSpEntityId();
417421
String spNameQualifier = nameIdElem.getAttribute("SPNameQualifier");
418-
if (!spNameQualifier.equals(settings.getSpEntityId())) {
422+
if (settings.isStrict() && !spNameQualifier.equals(settings.getSpEntityId())) {
419423
throw new Exception("The SPNameQualifier value mistmatch the SP entityID value.");
420424
} else {
421425
nameIdData.put("SPNameQualifier", spNameQualifier);
@@ -461,16 +465,13 @@ public HashMap<String, List<String>> getAttributes() throws Exception {
461465
HashMap<String, List<String>> attributes = new HashMap<String, List<String>>();
462466

463467
NodeList nodes = this.queryAssertion("/saml:AttributeStatement/saml:Attribute");
464-
List<String> processedNames = new ArrayList<String>();
465468

466469
if (nodes.getLength() != 0) {
467470
for (int i = 0; i < nodes.getLength(); i++) {
468471
NamedNodeMap attrName = nodes.item(i).getAttributes();
469472
String attName = attrName.getNamedItem("Name").getNodeValue();
470-
if (processedNames.contains(attName)) {
473+
if (attributes.containsKey(attName)) {
471474
throw new Exception("Found an Attribute element with duplicated Name");
472-
} else {
473-
processedNames.add(attName);
474475
}
475476

476477
NodeList childrens = nodes.item(i).getChildNodes();
@@ -525,14 +526,14 @@ public static SamlResponseStatus getStatus(Document dom) throws IllegalArgumentE
525526

526527
NodeList statusEntry = Util.query(dom, statusExpr, null);
527528
if (statusEntry.getLength() != 1) {
528-
throw new IllegalArgumentException("Missing Status on response");
529+
throw new IllegalArgumentException("Missing valid Status on response");
529530
}
530531
NodeList codeEntry;
531532

532533
codeEntry = Util.query(dom, statusExpr + "/samlp:StatusCode", (Element) statusEntry.item(0));
533534

534535
if (codeEntry.getLength() != 1) {
535-
throw new IllegalArgumentException("Missing Status Code on response");
536+
throw new IllegalArgumentException("Missing valid Status Code on response");
536537
}
537538

538539
String stausCode = codeEntry.item(0).getAttributes().getNamedItem("Value").getNodeValue();
@@ -621,7 +622,7 @@ public List<String> getIssuers() throws Exception {
621622
issuers.add(value);
622623
}
623624
} else {
624-
throw new Exception("Issuer of the Response not found.");
625+
throw new Exception("Issuer of the Response not found or multiple.");
625626
}
626627

627628
NodeList assertionIssuer = this.queryAssertion("/saml:Issuer");
@@ -631,7 +632,7 @@ public List<String> getIssuers() throws Exception {
631632
issuers.add(value);
632633
}
633634
} else {
634-
throw new Exception("Issuer of the Assertion not found.");
635+
throw new Exception("Issuer of the Assertion not found or multiple.");
635636
}
636637

637638
return issuers;
@@ -739,28 +740,15 @@ public ArrayList<String> processSignedElements() throws Exception {
739740
NodeList signNodes = query("//ds:Signature", null);
740741
for (int i = 0; i < signNodes.getLength(); i++) {
741742
Node signNode = signNodes.item(i);
742-
String signedElement = signNode.getParentNode().getLocalName();
743+
String signedElement = "{" + signNode.getParentNode().getNamespaceURI() + "}" + signNode.getParentNode().getLocalName();
744+
745+
String responseTag = "{" + Constants.NS_SAMLP + "}Response";
746+
String assertionTag = "{" + Constants.NS_SAML + "}Assertion";
743747

744-
if (!signedElement.equals("Response") && !signedElement.equals("Assertion")) {
748+
if (!signedElement.equals(responseTag) && !signedElement.equals(assertionTag)) {
745749
throw new Exception("Invalid Signature Element " + signedElement + " SAML Response rejected");
746750
}
747751

748-
// check that the signed elements found here, are the ones that will be verified
749-
// by com.onelogin.saml2.util.Util.validateSign()
750-
if (signedElement.equals("Response")) {
751-
final NodeList expectedSignatureNode = query(Util.RESPONSE_SIGNATURE_XPATH, null);
752-
if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
753-
throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
754-
}
755-
}
756-
757-
if (signedElement.equals("Assertion")) {
758-
final NodeList expectedSignatureNode = query(Util.ASSERTION_SIGNATURE_XPATH, null);
759-
if (expectedSignatureNode.getLength() != 1 || signedElements.contains(signedElement)) {
760-
throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
761-
}
762-
}
763-
764752
// Check that reference URI matches the parent ID and no duplicate References or IDs
765753
Node idNode = signNode.getParentNode().getAttributes().getNamedItem("ID");
766754
if (idNode == null || idNode.getNodeValue() == null || idNode.getNodeValue().isEmpty()) {
@@ -810,8 +798,10 @@ public ArrayList<String> processSignedElements() throws Exception {
810798
* @param signedElements
811799
* the elements to be validated
812800
* @return true if is valid
801+
*
802+
* @throws Exception
813803
*/
814-
public static boolean validateSignedElements(ArrayList<String> signedElements) {
804+
public boolean validateSignedElements(ArrayList<String> signedElements) throws Exception {
815805
if (signedElements.size() > 2) {
816806
return false;
817807
}
@@ -825,11 +815,31 @@ public static boolean validateSignedElements(ArrayList<String> signedElements) {
825815
}
826816
}
827817

828-
if ((occurrences.containsKey("Response") && occurrences.get("Response") > 1)
829-
|| (occurrences.containsKey("Assertion") && occurrences.get("Assertion") > 1)
830-
|| !occurrences.containsKey("Response") && !occurrences.containsKey("Assertion")) {
818+
String responseTag = "{" + Constants.NS_SAMLP + "}Response";
819+
String assertionTag = "{" + Constants.NS_SAML + "}Assertion";
820+
821+
if ((occurrences.containsKey(responseTag) && occurrences.get(responseTag) > 1)
822+
|| (occurrences.containsKey(assertionTag) && occurrences.get(assertionTag) > 1)
823+
|| !occurrences.containsKey(responseTag) && !occurrences.containsKey(assertionTag)) {
831824
return false;
832825
}
826+
827+
// check that the signed elements found here, are the ones that will be verified
828+
// by com.onelogin.saml2.util.Util.validateSign()
829+
if (occurrences.containsKey(responseTag)) {
830+
final NodeList expectedSignatureNode = query(Util.RESPONSE_SIGNATURE_XPATH, null);
831+
if (expectedSignatureNode.getLength() != 1) {
832+
throw new Exception("Unexpected number of Response signatures found. SAML Response rejected.");
833+
}
834+
}
835+
836+
if (occurrences.containsKey(assertionTag)) {
837+
final NodeList expectedSignatureNode = query(Util.ASSERTION_SIGNATURE_XPATH, null);
838+
if (expectedSignatureNode.getLength() != 1) {
839+
throw new Exception("Unexpected number of Assertion signatures found. SAML Response rejected.");
840+
}
841+
}
842+
833843
return true;
834844
}
835845

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

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import static org.junit.Assert.assertThat;
1010
import static org.junit.Assert.assertTrue;
1111

12-
import java.io.IOException;
1312
import java.util.ArrayList;
1413
import java.util.HashMap;
1514
import java.util.List;
@@ -777,7 +776,7 @@ public void testValidatesTheExpectedSignatures() throws Exception {
777776

778777
// then
779778
assertFalse(samlResponse.isValid());
780-
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
779+
assertEquals("Invalid Signature Element {urn:oasis:names:tc:SAML:2.0:assertion}Response SAML Response rejected", samlResponse.getError());
781780
}
782781

783782
/**
@@ -1040,10 +1039,9 @@ public void testIsValidWrongEncryptedID() throws Exception {
10401039
public void testIsValidWrongSPNameQualifier() throws Exception {
10411040
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
10421041
String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_spnamequalifier.xml.base64");
1043-
settings.setStrict(false);
1042+
settings.setStrict(true);
10441043
settings.setWantAssertionsSigned(false);
10451044
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1046-
assertTrue(samlResponse.isValid());
10471045
String nameId = samlResponse.getNameId();
10481046
}
10491047

@@ -1885,27 +1883,27 @@ public void testIsInValidSign() throws Exception {
18851883
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/triple_signed_response.xml.base64");
18861884
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
18871885
assertFalse(samlResponse.isValid());
1888-
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
1886+
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
18891887

18901888
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_assertion_response_with_2signatures.xml.base64");
18911889
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
18921890
assertFalse(samlResponse.isValid());
1893-
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
1891+
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
18941892

18951893
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_message_response_with_2signatures.xml.base64");
18961894
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
18971895
assertFalse(samlResponse.isValid());
1898-
assertEquals("Unexpected number of Response signatures found. SAML Response rejected.", samlResponse.getError());
1896+
assertEquals("Duplicated ID. SAML Response rejected", samlResponse.getError());
18991897

19001898
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_signed_element.xml.base64");
19011899
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19021900
assertFalse(samlResponse.isValid());
1903-
assertEquals("Invalid Signature Element Subject SAML Response rejected", samlResponse.getError());
1901+
assertEquals("Invalid Signature Element {urn:oasis:names:tc:SAML:2.0:assertion}Subject SAML Response rejected", samlResponse.getError());
19041902

19051903
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_signed_element2.xml.base64");
19061904
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19071905
assertFalse(samlResponse.isValid());
1908-
assertEquals("Invalid Signature Element Subject SAML Response rejected", samlResponse.getError());
1906+
assertEquals("Invalid Signature Element {urn:oasis:names:tc:SAML:2.0:assertion}Subject SAML Response rejected", samlResponse.getError());
19091907

19101908
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/duplicate_reference_uri.xml.base64");
19111909
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
@@ -1933,63 +1931,69 @@ public void testIsInValidSign() throws Exception {
19331931
*/
19341932
@Test
19351933
public void testValidateSignedElements() throws Exception {
1934+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.min.properties").build();
19361935
ArrayList<String> signedElements = new ArrayList<String>();
19371936
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_response.xml.base64");
19381937
Document samlResponseDoc = Util.loadXML(new String(Util.base64decoder(samlResponseEncoded)));
1938+
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19391939
NodeList signNodes = Util.query(samlResponseDoc, "//ds:Signature");
19401940

1941-
assertFalse(SamlResponse.validateSignedElements(signedElements));
1941+
assertFalse(samlResponse.validateSignedElements(signedElements));
19421942

19431943
signedElements = new ArrayList<String>();
19441944
for (int i = 0; i < signNodes.getLength(); i++) {
19451945
Node signNode = signNodes.item(i);
1946-
signedElements.add(signNode.getParentNode().getLocalName());
1946+
signedElements.add("{" + signNode.getParentNode().getNamespaceURI() + "}" + signNode.getParentNode().getLocalName());
19471947
}
1948-
assertTrue(SamlResponse.validateSignedElements(signedElements));
1948+
assertTrue(samlResponse.validateSignedElements(signedElements));
19491949

19501950
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/triple_signed_response.xml.base64");
19511951
samlResponseDoc = Util.loadXML(new String(Util.base64decoder(samlResponseEncoded)));
1952+
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19521953
signNodes = Util.query(samlResponseDoc, "//ds:Signature");
19531954

19541955
signedElements = new ArrayList<String>();
19551956
for (int i = 0; i < signNodes.getLength(); i++) {
19561957
Node signNode = signNodes.item(i);
19571958
signedElements.add(signNode.getParentNode().getLocalName());
19581959
}
1959-
assertFalse(SamlResponse.validateSignedElements(signedElements));
1960+
assertFalse(samlResponse.validateSignedElements(signedElements));
19601961

19611962
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_assertion_response_with_2signatures.xml.base64");
19621963
samlResponseDoc = Util.loadXML(new String(Util.base64decoder(samlResponseEncoded)));
1964+
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19631965
signNodes = Util.query(samlResponseDoc, "//ds:Signature");
19641966

19651967
signedElements = new ArrayList<String>();
19661968
for (int i = 0; i < signNodes.getLength(); i++) {
19671969
Node signNode = signNodes.item(i);
1968-
signedElements.add(signNode.getParentNode().getLocalName());
1970+
signedElements.add("{" + signNode.getParentNode().getNamespaceURI() + "}" + signNode.getParentNode().getLocalName());
19691971
}
1970-
assertFalse(SamlResponse.validateSignedElements(signedElements));
1972+
assertFalse(samlResponse.validateSignedElements(signedElements));
19711973

19721974
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/signed_message_response_with_2signatures.xml.base64");
19731975
samlResponseDoc = Util.loadXML(new String(Util.base64decoder(samlResponseEncoded)));
1976+
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19741977
signNodes = Util.query(samlResponseDoc, "//ds:Signature");
19751978

19761979
signedElements = new ArrayList<String>();
19771980
for (int i = 0; i < signNodes.getLength(); i++) {
19781981
Node signNode = signNodes.item(i);
1979-
signedElements.add(signNode.getParentNode().getLocalName());
1982+
signedElements.add("{" + signNode.getParentNode().getNamespaceURI() + "}" + signNode.getParentNode().getLocalName());
19801983
}
1981-
assertFalse(SamlResponse.validateSignedElements(signedElements));
1984+
assertFalse(samlResponse.validateSignedElements(signedElements));
19821985

19831986
samlResponseEncoded = Util.getFileAsString("data/responses/invalids/wrong_signed_element.xml.base64");
19841987
samlResponseDoc = Util.loadXML(new String(Util.base64decoder(samlResponseEncoded)));
1988+
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
19851989
signNodes = Util.query(samlResponseDoc, "//ds:Signature");
19861990

19871991
signedElements = new ArrayList<String>();
19881992
for (int i = 0; i < signNodes.getLength(); i++) {
19891993
Node signNode = signNodes.item(i);
1990-
signedElements.add(signNode.getParentNode().getLocalName());
1994+
signedElements.add("{" + signNode.getParentNode().getNamespaceURI() + "}" + signNode.getParentNode().getLocalName());
19911995
}
1992-
assertFalse(SamlResponse.validateSignedElements(signedElements));
1996+
assertFalse(samlResponse.validateSignedElements(signedElements));
19931997
}
19941998

19951999
/**
@@ -2088,3 +2092,4 @@ private static HttpRequest newHttpRequest(String requestURL, String samlResponse
20882092
return new HttpRequest(requestURL).addParameter("SAMLResponse", samlResponseEncoded);
20892093
}
20902094
}
2095+

0 commit comments

Comments
 (0)