Skip to content

Commit 70aa5c7

Browse files
pitbulkpitbulk
authored
Merge pull request #66 from miszobi/issue/63-reject-incomplete-SubjectConfirmationData
#63: Reject SubjectConfirmationData with no Recipient or NotOnOrAfter
2 parents 28613de + 599f4ba commit 70aa5c7

8 files changed

Lines changed: 395 additions & 280 deletions

File tree

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

Lines changed: 67 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package com.onelogin.saml2.authn;
22

3-
import java.io.IOException;
43
import java.security.PrivateKey;
54
import java.security.cert.X509Certificate;
65
import java.util.ArrayList;
@@ -9,12 +8,9 @@
98
import java.util.Map;
109

1110
import javax.servlet.http.HttpServletRequest;
12-
import javax.xml.parsers.ParserConfigurationException;
13-
import javax.xml.xpath.XPath;
14-
import javax.xml.xpath.XPathConstants;
1511
import javax.xml.xpath.XPathExpressionException;
16-
import javax.xml.xpath.XPathFactory;
1712

13+
import com.onelogin.saml2.model.SubjectConfirmationIssue;
1814
import org.joda.time.DateTime;
1915
import org.slf4j.Logger;
2016
import org.slf4j.LoggerFactory;
@@ -23,7 +19,6 @@
2319
import org.w3c.dom.NamedNodeMap;
2420
import org.w3c.dom.Node;
2521
import org.w3c.dom.NodeList;
26-
import org.xml.sax.SAXException;
2722

2823
import com.onelogin.saml2.settings.Saml2Settings;
2924
import com.onelogin.saml2.model.SamlResponseStatus;
@@ -242,55 +237,7 @@ public boolean isValid(String requestId) {
242237
}
243238
}
244239

245-
// Check SubjectConfirmation, at least one SubjectConfirmation must be valid
246-
boolean validSubjectConfirmation = false;
247-
NodeList subjectConfirmationNodes = this.queryAssertion("/saml:Subject/saml:SubjectConfirmation");
248-
for (int i = 0; i < subjectConfirmationNodes.getLength(); i++) {
249-
Node scn = subjectConfirmationNodes.item(i);
250-
251-
Node method = scn.getAttributes().getNamedItem("Method");
252-
if (method != null && !method.getNodeValue().equals(Constants.CM_BEARER)) {
253-
continue;
254-
}
255-
256-
NodeList subjectConfirmationDataNodes = scn.getChildNodes();
257-
for (int c = 0; c < subjectConfirmationDataNodes.getLength(); c++) {
258-
if (subjectConfirmationDataNodes.item(c).getLocalName() != null && subjectConfirmationDataNodes.item(c).getLocalName().equals("SubjectConfirmationData")) {
259-
260-
Node recipient = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("Recipient");
261-
if (recipient != null && !recipient.getNodeValue().isEmpty()
262-
&& !recipient.getNodeValue().equals(currentUrl)) {
263-
continue;
264-
}
265-
266-
Node inResponseTo = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("InResponseTo");
267-
if (inResponseTo != null && !inResponseTo.getNodeValue().equals(responseInResponseTo)) {
268-
continue;
269-
}
270-
271-
272-
Node notOnOrAfter = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotOnOrAfter");
273-
if (notOnOrAfter != null) {
274-
DateTime noa = Util.parseDateTime(notOnOrAfter.getNodeValue());
275-
if (noa.isEqualNow() || noa.isBeforeNow()) {
276-
continue;
277-
}
278-
}
279-
280-
Node notBefore = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotBefore");
281-
if (notBefore != null) {
282-
DateTime nb = Util.parseDateTime(notBefore.getNodeValue());
283-
if (nb.isAfterNow()) {
284-
continue;
285-
}
286-
}
287-
validSubjectConfirmation = true;
288-
}
289-
}
290-
}
291-
if (!validSubjectConfirmation) {
292-
throw new Exception("A valid SubjectConfirmation was not found on this Response");
293-
}
240+
validateSubjectConfirmation(responseInResponseTo);
294241

295242
if (settings.getWantAssertionsSigned() && !signedElements.contains("Assertion")) {
296243
throw new Exception("The Assertion of the Response is not signed and the SP requires it");
@@ -334,6 +281,71 @@ public boolean isValid(String requestId) {
334281
}
335282
}
336283

284+
// Check SubjectConfirmation, at least one SubjectConfirmation must be valid
285+
private void validateSubjectConfirmation(String responseInResponseTo) throws Exception {
286+
final List<SubjectConfirmationIssue> validationIssues = new ArrayList<>();
287+
boolean validSubjectConfirmation = false;
288+
NodeList subjectConfirmationNodes = this.queryAssertion("/saml:Subject/saml:SubjectConfirmation");
289+
for (int i = 0; i < subjectConfirmationNodes.getLength(); i++) {
290+
Node scn = subjectConfirmationNodes.item(i);
291+
292+
Node method = scn.getAttributes().getNamedItem("Method");
293+
if (method != null && !method.getNodeValue().equals(Constants.CM_BEARER)) {
294+
continue;
295+
}
296+
297+
NodeList subjectConfirmationDataNodes = scn.getChildNodes();
298+
for (int c = 0; c < subjectConfirmationDataNodes.getLength(); c++) {
299+
if (subjectConfirmationDataNodes.item(c).getLocalName() != null && subjectConfirmationDataNodes.item(c).getLocalName().equals("SubjectConfirmationData")) {
300+
301+
Node recipient = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("Recipient");
302+
if (recipient == null) {
303+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't contain a Recipient"));
304+
continue;
305+
}
306+
307+
if (!recipient.getNodeValue().equals(currentUrl)) {
308+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't match a valid Recipient"));
309+
continue;
310+
}
311+
312+
Node inResponseTo = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("InResponseTo");
313+
if (inResponseTo != null && !inResponseTo.getNodeValue().equals(responseInResponseTo)) {
314+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData has an invalid InResponseTo value"));
315+
continue;
316+
}
317+
318+
319+
Node notOnOrAfter = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotOnOrAfter");
320+
if (notOnOrAfter == null) {
321+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't contain a NotOnOrAfter attribute"));
322+
continue;
323+
}
324+
325+
DateTime noa = Util.parseDateTime(notOnOrAfter.getNodeValue());
326+
if (noa.isEqualNow() || noa.isBeforeNow()) {
327+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is no longer valid"));
328+
continue;
329+
}
330+
331+
Node notBefore = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotBefore");
332+
if (notBefore != null) {
333+
DateTime nb = Util.parseDateTime(notBefore.getNodeValue());
334+
if (nb.isAfterNow()) {
335+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is not yet valid"));
336+
continue;
337+
}
338+
}
339+
validSubjectConfirmation = true;
340+
}
341+
}
342+
}
343+
344+
if (!validSubjectConfirmation) {
345+
throw new Exception(SubjectConfirmationIssue.prettyPrintIssues(validationIssues));
346+
}
347+
}
348+
337349
/**
338350
* Determines if the SAML Response is valid using the certificate.
339351
*

‎core/src/main/java/com/onelogin/saml2/model/SubjectConfirmationIssue.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package com.onelogin.saml2.model;
2+
3+
import java.util.List;
4+
5+
public class SubjectConfirmationIssue {
6+
private final int subjectConfirmationIndex;
7+
private final String message;
8+
9+
public SubjectConfirmationIssue(int subjectConfirmationIndex, String message) {
10+
this.subjectConfirmationIndex = subjectConfirmationIndex;
11+
this.message = message;
12+
}
13+
14+
public static String prettyPrintIssues(List<SubjectConfirmationIssue> subjectConfirmationDataIssues) {
15+
StringBuilder subjectConfirmationDataIssuesMsg = new StringBuilder("A valid SubjectConfirmation was not found on this Response");
16+
if (subjectConfirmationDataIssues.size() > 0) {
17+
subjectConfirmationDataIssuesMsg.append(": ");
18+
}
19+
for (int i = 0; i < subjectConfirmationDataIssues.size(); i++) {
20+
final SubjectConfirmationIssue issue = subjectConfirmationDataIssues.get(i);
21+
if (subjectConfirmationDataIssues.size() > 1) {
22+
subjectConfirmationDataIssuesMsg.append("\n[")
23+
.append(issue.subjectConfirmationIndex)
24+
.append("] ");
25+
}
26+
subjectConfirmationDataIssuesMsg.append(issue.message);
27+
if (i != subjectConfirmationDataIssues.size() - 1) {
28+
subjectConfirmationDataIssuesMsg.append(", ");
29+
}
30+
}
31+
32+
return subjectConfirmationDataIssuesMsg.toString();
33+
}
34+
}

0 commit comments

Comments
 (0)