From 84779ce0414b319327a194a0d7f92be15a899686 Mon Sep 17 00:00:00 2001 From: sgrampone Date: Wed, 20 Aug 2025 19:07:51 -0300 Subject: [PATCH 1/3] Asserition manipulation attack mitigation --- .../java/com/genexus/saml20/PostBinding.java | 22 ++++++++---- .../java/com/genexus/saml20/utils/DSig.java | 34 +++++++++++++++---- .../com/genexus/saml20/utils/Encoding.java | 20 +++++++++++ .../java/com/genexus/saml20/utils/Keys.java | 1 - 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java b/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java index 885f354bf..81edb67b3 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java @@ -15,10 +15,10 @@ public class PostBinding extends Binding { private static final Logger logger = LogManager.getLogger(PostBinding.class); private Document xmlDoc; + private Document verifiedDoc; public PostBinding() { logger.trace("PostBinding constructor"); - xmlDoc = null; } // EXTERNAL OBJECT PUBLIC METHODS - BEGIN @@ -42,31 +42,39 @@ public static String logout(SamlParms parms, String relayState) { } public boolean verifySignatures(SamlParms parms) { - return DSig.validateSignatures(this.xmlDoc, parms.getTrustCertPath(), parms.getTrustCertAlias(), parms.getTrustCertPass()); + String verified = DSig.validateSignatures(this.xmlDoc, parms.getTrustCertPath(), parms.getTrustCertAlias(), parms.getTrustCertPass()); + if(verified.isEmpty()){ + return false; + }else { + this.verifiedDoc = SamlAssertionUtils.loadDocument(verified); + this.xmlDoc = null; + logger.debug(MessageFormat.format("verifySignatures sanitized xmlDoc {0}", Encoding.documentToString(this.xmlDoc))); + return true; + } } public String getLoginAssertions() { logger.trace("getLoginAssertions"); - return SamlAssertionUtils.getLoginInfo(this.xmlDoc); + return SamlAssertionUtils.getLoginInfo(this.verifiedDoc); } public String getLogoutAssertions() { logger.trace("getLogoutAssertions"); - return SamlAssertionUtils.getLogoutInfo(this.xmlDoc); + return SamlAssertionUtils.getLogoutInfo(this.verifiedDoc); } public String getLoginAttribute(String name) { logger.trace("getLoginAttribute"); - return SamlAssertionUtils.getLoginAttribute(this.xmlDoc, name).trim(); + return SamlAssertionUtils.getLoginAttribute(this.verifiedDoc, name).trim(); } public String getRoles(String name) { logger.debug("getRoles"); - return SamlAssertionUtils.getRoles(this.xmlDoc, name); + return SamlAssertionUtils.getRoles(this.verifiedDoc, name); } public boolean isLogout(){ - return SamlAssertionUtils.isLogout(this.xmlDoc); + return SamlAssertionUtils.isLogout(this.verifiedDoc); } // EXTERNAL OBJECT PUBLIC METHODS - END diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java index e3aa3a623..6f72c3088 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java @@ -6,6 +6,7 @@ import org.apache.xml.security.utils.Constants; import org.w3c.dom.Document; import org.w3c.dom.Element; +import org.w3c.dom.Node; import org.w3c.dom.NodeList; import javax.xml.xpath.XPath; @@ -22,8 +23,9 @@ public class DSig { private static final Logger logger = LogManager.getLogger(DSig.class); - public static boolean validateSignatures(Document xmlDoc, String certPath, String certAlias, String certPassword) { + public static String validateSignatures(Document xmlDoc, String certPath, String certAlias, String certPassword) { logger.trace("validateSignatures"); + List assertions = new ArrayList(); X509Certificate cert = Keys.loadCertificate(certPath, certAlias, certPassword); NodeList nodes = findElementsByPath(xmlDoc, "//*[@ID]"); @@ -31,29 +33,47 @@ public static boolean validateSignatures(Document xmlDoc, String certPath, Strin NodeList signatures = xmlDoc.getElementsByTagNameNS(Constants.SignatureSpecNS, Constants._TAG_SIGNATURE); //check the message is signed - security measure if(signatures.getLength() == 0){ - return false; + return ""; } for (int i = 0; i < signatures.getLength(); i++) { Element signedElement = findNodeById(nodes, getSignatureID((Element) signatures.item(i))); + assertions.add(signedElement); if (signedElement == null) { - return false; + return ""; } signedElement.setIdAttribute("ID", true); try { XMLSignature signature = new XMLSignature((Element) signatures.item(i), ""); //verifies the signature algorithm is one expected - security meassure if (!verifySignatureAlgorithm((Element) signatures.item(i))) { - return false; + return ""; } if (!signature.checkSignatureValue(cert)) { - return false; + return ""; } } catch (Exception e) { logger.error("validateSignatures", e); - return false; + return ""; } } - return true; + return buildXml(assertions, xmlDoc); + } + + public static String buildXml(List assertions, Document xmlDoc){ + //security meassure against assertion manipulation, it assures that every assertion to be used on the app has been signed and verified + Element element = xmlDoc.getDocumentElement(); + Node response = element.cloneNode(false); + + NodeList status = element.getElementsByTagNameNS("urn:oasis:names:tc:SAML:2.0:protocol", "Status"); + response.appendChild(status.item(0)); + + for(Element elem: assertions){ + if(!elem.getLocalName().equals("Response")){ + Node node = elem.cloneNode(true); + response.appendChild(node); + } + } + return Encoding.elementToString((Element) response); } private static boolean verifySignatureAlgorithm(Element elem) { diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/Encoding.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/Encoding.java index 4b9bfa523..f60a872d0 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/Encoding.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/Encoding.java @@ -4,7 +4,9 @@ import org.apache.logging.log4j.Logger; import org.bouncycastle.util.encoders.Base64; import org.w3c.dom.Document; +import org.w3c.dom.Element; +import javax.xml.transform.OutputKeys; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; @@ -74,6 +76,24 @@ public static String documentToString(Document doc) { } } + public static String elementToString(Element element) { + try { + TransformerFactory tf = TransformerFactory.newInstance(); + Transformer transformer = tf.newTransformer(); + + transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes"); + transformer.setOutputProperty(OutputKeys.INDENT, "yes"); + + StringWriter writer = new StringWriter(); + transformer.transform(new DOMSource(element), new StreamResult(writer)); + + return writer.toString(); + } catch (Exception e) { + logger.error("elementToString", e); + return null; + } + } + public static byte[] decodeParameter(String parm) { logger.trace("decodeParameter"); try { diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/Keys.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/Keys.java index 9547bae3f..d1641284d 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/Keys.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/Keys.java @@ -112,7 +112,6 @@ private static X509Certificate loadCertificateFromJKS(String path, String alias, logger.debug(MessageFormat.format("path: {0}, alias: {1}", path, alias)); Path p = new File(path).toPath(); logger.debug("Res path: " + p.toAbsolutePath()); - System.out.println("Res path: " + p.toAbsolutePath()); try (InputStream in = new DataInputStream(Files.newInputStream(p))) { KeyStore ks = KeyStore.getInstance("JKS"); ks.load(in, password.toCharArray()); From 3fde14b72a837fc14d6ab2740f0881a7190193df Mon Sep 17 00:00:00 2001 From: sgrampone Date: Thu, 21 Aug 2025 17:27:18 -0300 Subject: [PATCH 2/3] Fix Logout errors --- .../java/com/genexus/saml20/utils/DSig.java | 17 +--------- .../saml20/utils/SamlAssertionUtils.java | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java index 6f72c3088..a6fcc30f2 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java @@ -56,25 +56,10 @@ public static String validateSignatures(Document xmlDoc, String certPath, String return ""; } } - return buildXml(assertions, xmlDoc); + return SamlAssertionUtils.isLogout(xmlDoc) ? SamlAssertionUtils.buildXmlLogout(assertions) : SamlAssertionUtils.buildXmlLogin(assertions, xmlDoc); } - public static String buildXml(List assertions, Document xmlDoc){ - //security meassure against assertion manipulation, it assures that every assertion to be used on the app has been signed and verified - Element element = xmlDoc.getDocumentElement(); - Node response = element.cloneNode(false); - NodeList status = element.getElementsByTagNameNS("urn:oasis:names:tc:SAML:2.0:protocol", "Status"); - response.appendChild(status.item(0)); - - for(Element elem: assertions){ - if(!elem.getLocalName().equals("Response")){ - Node node = elem.cloneNode(true); - response.appendChild(node); - } - } - return Encoding.elementToString((Element) response); - } private static boolean verifySignatureAlgorithm(Element elem) { logger.trace("verifySignatureAlgorithm"); diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java index b8b91ee5c..02bd528d6 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java @@ -49,6 +49,37 @@ public static Document loadDocument(String xml) { } } + public static String buildXmlLogin(List assertions, Document xmlDoc){ + //security meassure against assertion manipulation, it assures that every assertion to be used on the app has been signed and verified + org.w3c.dom.Element element = xmlDoc.getDocumentElement(); + Node response = element.cloneNode(false); + + NodeList status = element.getElementsByTagNameNS(_saml_protocolNS, "Status"); + response.appendChild(status.item(0)); + + for(org.w3c.dom.Element elem: assertions){ + if(!elem.getLocalName().equals("Response")){ + Node node = elem.cloneNode(true); + response.appendChild(node); + } + } + return Encoding.elementToString((org.w3c.dom.Element) response); + } + + public static String buildXmlLogout(List assertions){ + if(assertions.isEmpty()) + { + return ""; + } + org.w3c.dom.Element element = assertions.get(0); + Node logoutResponse = element.cloneNode(false); + NodeList status = element.getElementsByTagNameNS(_saml_protocolNS, "Status"); + logoutResponse.appendChild(status.item(0)); + NodeList issuer = element.getElementsByTagNameNS(_saml_assertionNS, "Issuer"); + logoutResponse.appendChild(issuer.item(0)); + return Encoding.elementToString((org.w3c.dom.Element) logoutResponse); + } + public static boolean isLogout(Document xmlDoc){ logger.trace("isLogout"); try { From 694f58651e682b017165e0a8bb003f77f50bda39 Mon Sep 17 00:00:00 2001 From: sgrampone Date: Fri, 22 Aug 2025 18:02:58 -0300 Subject: [PATCH 3/3] Add log --- gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java | 5 ++--- .../java/com/genexus/saml20/utils/SamlAssertionUtils.java | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java b/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java index 81edb67b3..b8d1e50bd 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/PostBinding.java @@ -47,8 +47,7 @@ public boolean verifySignatures(SamlParms parms) { return false; }else { this.verifiedDoc = SamlAssertionUtils.loadDocument(verified); - this.xmlDoc = null; - logger.debug(MessageFormat.format("verifySignatures sanitized xmlDoc {0}", Encoding.documentToString(this.xmlDoc))); + logger.debug(MessageFormat.format("verifySignatures - sanitized xmlDoc {0}", Encoding.documentToString(this.xmlDoc))); return true; } } @@ -74,7 +73,7 @@ public String getRoles(String name) { } public boolean isLogout(){ - return SamlAssertionUtils.isLogout(this.verifiedDoc); + return SamlAssertionUtils.isLogout(this.xmlDoc); } // EXTERNAL OBJECT PUBLIC METHODS - END diff --git a/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java b/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java index 02bd528d6..88755c1dd 100644 --- a/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java +++ b/gamsaml20/src/main/java/com/genexus/saml20/utils/SamlAssertionUtils.java @@ -51,6 +51,7 @@ public static Document loadDocument(String xml) { public static String buildXmlLogin(List assertions, Document xmlDoc){ //security meassure against assertion manipulation, it assures that every assertion to be used on the app has been signed and verified + logger.trace("buildXmlLogin"); org.w3c.dom.Element element = xmlDoc.getDocumentElement(); Node response = element.cloneNode(false); @@ -67,8 +68,10 @@ public static String buildXmlLogin(List assertions, Documen } public static String buildXmlLogout(List assertions){ + logger.trace("buildXmlLogout"); if(assertions.isEmpty()) { + logger.error("buildXmlLogout - There are 0 signed assertions on LogoutResponse"); return ""; } org.w3c.dom.Element element = assertions.get(0);