Skip to content

Commit fb9b104

Browse files
giuseppe-gangemigiuseppe-gangemi
committed
fix: add security suggestions
1 parent c4fb3b4 commit fb9b104

File tree

3 files changed

+69
-25
lines changed

3 files changed

+69
-25
lines changed

‎src/oneid/oneid-ecs-core/src/main/java/it/pagopa/oneid/service/config/SAMLNamespaceContext.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package it.pagopa.oneid.service.config;
2+
3+
import java.util.Iterator;
4+
import javax.xml.XMLConstants;
5+
import javax.xml.namespace.NamespaceContext;
6+
7+
public class SAMLNamespaceContext implements NamespaceContext {
8+
9+
@Override
10+
public String getNamespaceURI(String prefix) {
11+
if (prefix == null) {
12+
throw new NullPointerException("Null prefix is not allowed.");
13+
}
14+
switch (prefix) {
15+
case "samlp":
16+
return "urn:oasis:names:tc:SAML:2.0:protocol";
17+
case "saml":
18+
return "urn:oasis:names:tc:SAML:2.0:assertion";
19+
case "ds":
20+
return "http://www.w3.org/2000/09/xmldsig#";
21+
default:
22+
// Return null for unmapped prefixes
23+
return XMLConstants.NULL_NS_URI;
24+
}
25+
}
26+
27+
@Override
28+
public String getPrefix(String namespaceURI) {
29+
throw new UnsupportedOperationException();
30+
}
31+
32+
@Override
33+
public Iterator<String> getPrefixes(String namespaceURI) {
34+
throw new UnsupportedOperationException();
35+
}
36+
}

‎src/oneid/oneid-ecs-core/src/main/java/it/pagopa/oneid/service/utils/SAMLUtilsExtendedCore.java‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import it.pagopa.oneid.exception.GenericAuthnRequestCreationException;
1212
import it.pagopa.oneid.exception.SAMLValidationException;
1313
import it.pagopa.oneid.model.dto.AttributeDTO;
14+
import it.pagopa.oneid.service.config.SAMLNamespaceContext;
1415
import it.pagopa.oneid.web.controller.interceptors.CurrentAuthDTO;
1516
import jakarta.enterprise.context.ApplicationScoped;
1617
import jakarta.inject.Inject;
@@ -32,6 +33,10 @@
3233
import javax.xml.parsers.DocumentBuilder;
3334
import javax.xml.parsers.DocumentBuilderFactory;
3435
import javax.xml.parsers.ParserConfigurationException;
36+
import javax.xml.xpath.XPath;
37+
import javax.xml.xpath.XPathConstants;
38+
import javax.xml.xpath.XPathExpressionException;
39+
import javax.xml.xpath.XPathFactory;
3540
import net.shibboleth.utilities.java.support.xml.BasicParserPool;
3641
import net.shibboleth.utilities.java.support.xml.XMLParserException;
3742
import org.eclipse.microprofile.config.inject.ConfigProperty;
@@ -60,9 +65,6 @@
6065
import org.opensaml.xmlsec.signature.support.SignatureValidator;
6166
import org.opensaml.xmlsec.signature.support.Signer;
6267
import org.w3c.dom.Document;
63-
import org.w3c.dom.Element;
64-
import org.w3c.dom.Node;
65-
import org.w3c.dom.NodeList;
6668
import org.xml.sax.SAXException;
6769

6870
@ApplicationScoped
@@ -188,47 +190,44 @@ private byte[] decodeBase64(String SAMLResponse) throws OneIdentityException {
188190
}
189191

190192
private Response unmarshallResponse(byte[] decodedSamlResponse) throws OneIdentityException {
193+
// log size of SAMLResponse for possible future check on max size
194+
Log.info("SAMLResponse size: " + decodedSamlResponse.length);
195+
196+
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
191197
try {
192198

193199
// Parse XML
194-
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
200+
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
201+
dbf.setXIncludeAware(false);
195202
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
196203
dbf.setNamespaceAware(true);
204+
197205
DocumentBuilder db = dbf.newDocumentBuilder();
198206
Document doc = db.parse(new ByteArrayInputStream(decodedSamlResponse));
199207

200-
// Check Response doesn't have multiple Signatures
201-
Element responseElem = doc.getDocumentElement();
202-
checkNoMultipleSignatures(responseElem);
203-
// Check each Assertion doesn't have multiple Signatures
204-
NodeList assertionNodes = responseElem.getElementsByTagNameNS(
205-
"urn:oasis:names:tc:SAML:2.0:assertion", "Assertion");
206-
for (int i = 0; i < assertionNodes.getLength(); i++) {
207-
checkNoMultipleSignatures((Element) assertionNodes.item(i));
208-
}
208+
checkNoMultipleSignatures(doc);
209209

210210
// return response even if it has multiple signatures, the error will be handled inside SAMLController
211211
return (Response) XMLObjectSupport.unmarshallFromInputStream(basicParserPool,
212212
new ByteArrayInputStream(decodedSamlResponse));
213213

214214
} catch (UnmarshallingException | XMLParserException | SAXException | IOException |
215-
ParserConfigurationException e) {
215+
ParserConfigurationException | XPathExpressionException e) {
216216
Log.error("Unmarshalling error: " + e.getMessage());
217217
throw new OneIdentityException(e);
218218
}
219219
}
220220

221-
private void checkNoMultipleSignatures(Element elem) {
222-
int count = 0;
223-
NodeList children = elem.getChildNodes();
224-
for (int i = 0; i < children.getLength(); i++) {
225-
Node n = children.item(i);
226-
if (n.getNodeType() == Node.ELEMENT_NODE && "Signature".equals(n.getLocalName())
227-
&& "http://www.w3.org/2000/09/xmldsig#".equals(n.getNamespaceURI())) {
228-
count++;
229-
}
230-
}
231-
if (count > 1) {
221+
private void checkNoMultipleSignatures(Document doc) throws XPathExpressionException {
222+
XPath xPath = XPathFactory.newInstance().newXPath();
223+
// Set the namespace context to handle prefixes like 'samlp', 'saml', and 'ds'
224+
xPath.setNamespaceContext(new SAMLNamespaceContext());
225+
226+
String expression = "count(.//ds:Signature)";
227+
Double responseSignatureCount = (Double) xPath.compile(expression)
228+
.evaluate(doc, XPathConstants.NUMBER);
229+
230+
if (responseSignatureCount > 2) {
232231
// set a flag in currentAuthDTO to handle the error in SAMLController
233232
Log.error(ErrorCode.IDP_ERROR_MULTIPLE_SAMLRESPONSE_SIGNATURES_PRESENT.getErrorMessage());
234233
currentAuthDTO.setResponseWithMultipleSignatures(true);

‎src/oneid/oneid-ecs-core/src/test/java/it/pagopa/oneid/service/utils/SAMLUtilsExtendedCoreTest.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static it.pagopa.oneid.model.Base64SAMLResponses.UNSIGNED_SAML_RESPONSE_02;
1010
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
1111
import static org.junit.jupiter.api.Assertions.assertEquals;
12+
import static org.junit.jupiter.api.Assertions.assertFalse;
1213
import static org.junit.jupiter.api.Assertions.assertNotNull;
1314
import static org.junit.jupiter.api.Assertions.assertNull;
1415
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -22,6 +23,7 @@
2223
import it.pagopa.oneid.common.model.exception.OneIdentityException;
2324
import it.pagopa.oneid.exception.SAMLValidationException;
2425
import it.pagopa.oneid.service.mock.X509CredentialTestProfile;
26+
import it.pagopa.oneid.web.controller.interceptors.CurrentAuthDTO;
2527
import jakarta.inject.Inject;
2628
import java.util.Map;
2729
import java.util.Set;
@@ -57,6 +59,8 @@ public class SAMLUtilsExtendedCoreTest {
5759
@Inject
5860
MarshallerFactory marshallerFactory;
5961

62+
@Inject
63+
CurrentAuthDTO currentAuthDTO;
6064

6165
@Test
6266
void buildIssuer() {
@@ -131,6 +135,7 @@ void getSAMLResponseFromString_success() {
131135
samlUtilsExtendedCore.getSAMLResponseFromString(
132136
samlResponseString);
133137
});
138+
assertFalse(currentAuthDTO.isResponseWithMultipleSignatures());
134139
}
135140

136141
@Test
@@ -142,6 +147,7 @@ void getSAMLResponseFromString_unMarshallingException() {
142147
OneIdentityException exception = assertThrows(OneIdentityException.class,
143148
() -> samlUtilsExtendedCore.getSAMLResponseFromString(samlResponseString));
144149
assertEquals(UnmarshallingException.class, exception.getCause().getClass());
150+
assertFalse(currentAuthDTO.isResponseWithMultipleSignatures());
145151
}
146152

147153
@Test
@@ -153,6 +159,7 @@ void getSAMLResponseFromString_IllegalArgumentException() {
153159
OneIdentityException exception = assertThrows(OneIdentityException.class,
154160
() -> samlUtilsExtendedCore.getSAMLResponseFromString(response));
155161
assertEquals(IllegalArgumentException.class, exception.getCause().getClass());
162+
assertFalse(currentAuthDTO.isResponseWithMultipleSignatures());
156163
}
157164

158165
@Test
@@ -170,6 +177,7 @@ void getSAMLResponseFromString_ResponseWithMultipleSignatures() {
170177
samlUtilsExtendedCore.getSAMLResponseFromString(
171178
samlResponseString);
172179
});
180+
assertTrue(currentAuthDTO.isResponseWithMultipleSignatures());
173181
}
174182

175183
@Test
@@ -187,6 +195,7 @@ void getSAMLResponseFromString_AssertionWithMultipleSignatures() {
187195
samlUtilsExtendedCore.getSAMLResponseFromString(
188196
samlResponseString);
189197
});
198+
assertTrue(currentAuthDTO.isResponseWithMultipleSignatures());
190199
}
191200

192201
@Test

0 commit comments

Comments
 (0)