Skip to content

Commit 54b0ddd

Browse files
author
mexmarv
committed
4 Security Checks + Indentation Tabs.
3 Security Checks in isValid(). 1 in loadXml() for XEE/XXE attacks.
1 parent 3d03ce8 commit 54b0ddd

1 file changed

Lines changed: 35 additions & 16 deletions

File tree

com/onelogin/saml/Response.java

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@
88
import java.security.cert.X509Certificate;
99
import java.util.ArrayList;
1010
import java.util.HashMap;
11-
import java.util.Iterator;
12-
import java.util.Map;
1311
import javax.xml.XMLConstants;
1412
import javax.xml.crypto.dsig.XMLSignature;
1513
import javax.xml.crypto.dsig.XMLSignatureFactory;
@@ -28,6 +26,8 @@
2826
public class Response {
2927

3028
private Document xmlDoc;
29+
private Integer Assertions;
30+
private Element rootElement;
3131
private final AccountSettings accountSettings;
3232
private final Certificate certificate;
3333

@@ -40,6 +40,7 @@ public Response(AccountSettings accountSettings) throws CertificateException {
4040
public void loadXml(String xml) throws ParserConfigurationException, SAXException, IOException {
4141
DocumentBuilderFactory fty = DocumentBuilderFactory.newInstance();
4242
fty.setNamespaceAware(true);
43+
// XMLConstants with FEATURE_SECURE_PROCESSING prevents external document access. (XXE/XEE Possible Attacks).
4344
fty.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
4445
DocumentBuilder builder = fty.newDocumentBuilder();
4546
ByteArrayInputStream bais = new ByteArrayInputStream(xml.getBytes());
@@ -48,16 +49,36 @@ public void loadXml(String xml) throws ParserConfigurationException, SAXExceptio
4849

4950
public void loadXmlFromBase64(String response) throws ParserConfigurationException, SAXException, IOException {
5051
Base64 base64 = new Base64();
51-
byte [] decodedB = base64.decode(response);
52+
byte[] decodedB = base64.decode(response);
5253
String decodedS = new String(decodedB);
5354
loadXml(decodedS);
5455
}
5556

57+
// isValid() function should be called to make basic security checks to responses.
5658
public boolean isValid() throws Exception {
59+
// Security Checks
60+
rootElement = xmlDoc.getDocumentElement();
61+
Assertions = xmlDoc.getElementsByTagNameNS("urn:oasis:names:tc:SAML:2.0:assertion", "Assertion").getLength();
62+
xmlDoc.getDocumentElement().normalize();
63+
64+
if (Assertions > 1) {
65+
throw new Exception("SAML Response must contain 1 Assertion.");
66+
}
67+
68+
String attName = rootElement.getAttribute("ID");
69+
if (attName.equals("")) {
70+
throw new Exception("Missing ID attribute on SAML Response.");
71+
}
72+
73+
attName = rootElement.getAttribute("Version");
74+
if (!attName.equals("2.0")) {
75+
throw new Exception("Unsupported SAML Version.");
76+
}
77+
5778
NodeList nodes = xmlDoc.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature");
5879

5980
if (nodes == null || nodes.getLength() == 0) {
60-
throw new Exception("Can't find signature in document.");
81+
throw new Exception("Can't find signature in Document.");
6182
}
6283

6384
if (setIdAttributeExists()) {
@@ -74,27 +95,25 @@ public boolean isValid() throws Exception {
7495

7596
public String getNameId() throws Exception {
7697
NodeList nodes = xmlDoc.getElementsByTagNameNS("urn:oasis:names:tc:SAML:2.0:assertion", "NameID");
77-
78-
if(nodes.getLength()==0){
79-
throw new Exception("No name id found in document");
98+
if (nodes.getLength() == 0) {
99+
throw new Exception("No name id found in Document.");
80100
}
81-
82101
return nodes.item(0).getTextContent();
83-
}
84-
102+
}
103+
85104
public String getAttribute(String name) {
86105
HashMap attributes = getAttributes();
87106
if (!attributes.isEmpty()) {
88107
return attributes.get(name).toString();
89108
}
90109
return null;
91110
}
92-
111+
93112
public HashMap getAttributes() {
94-
HashMap<String,ArrayList> attributes = new HashMap<>();
113+
HashMap<String, ArrayList> attributes = new HashMap<>();
95114
NodeList nodes = xmlDoc.getElementsByTagNameNS("urn:oasis:names:tc:SAML:2.0:assertion", "Attribute");
96115

97-
if(nodes.getLength()!=0){
116+
if (nodes.getLength() != 0) {
98117
for (int i = 0; i < nodes.getLength(); i++) {
99118
NamedNodeMap attrName = nodes.item(i).getAttributes();
100119
String attName = attrName.getNamedItem("Name").getNodeValue();
@@ -107,10 +126,10 @@ public HashMap getAttributes() {
107126
attributes.put(attName, attrValues);
108127
}
109128
} else {
110-
return null;
129+
return null;
111130
}
112131
return attributes;
113-
}
132+
}
114133

115134
private boolean setIdAttributeExists() {
116135
for (Method method : Element.class.getDeclaredMethods()) {
@@ -124,5 +143,5 @@ private boolean setIdAttributeExists() {
124143
private void tagIdAttributes(Document xmlDoc) {
125144
throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
126145
}
127-
}
128146

147+
}

0 commit comments

Comments
 (0)