Fix signature verification on RedirectBinding, add security measures for DSig signature validation

Author: sgrampone

gamsaml20/src/main/java/com/genexus/saml20/RedirectBinding.java

@@ -6,6 +6,7 @@
 import org.bouncycastle.crypto.Signer;
 import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
 import org.bouncycastle.crypto.signers.RSADigestSigner;
+import org.bouncycastle.jcajce.provider.asymmetric.RSA;
 import org.bouncycastle.util.encoders.Base64;
 import org.w3c.dom.Document;
 
@@ -14,6 +15,7 @@
 import java.net.URLDecoder;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
+import java.security.cert.X509Certificate;
 import java.text.MessageFormat;
 import java.util.HashMap;
 import java.util.Map;
@@ -100,6 +102,36 @@ public String getLoginAttribute(String name) {
 
 	// EXTERNAL OBJECT PUBLIC METHODS  - END
 
+	private boolean VerifySignature_internal(String certPath, String certPass, String certAlias) {
+		logger.trace("VerifySignature_internal");
+
+		byte[] signature = Encoding.decodeParameter(this.redirectMessage.get("Signature"));
+
+		String signedMessage;
+		if (this.redirectMessage.containsKey("RelayState")) {
+			signedMessage = MessageFormat.format("SAMLResponse={0}", this.redirectMessage.get("SAMLResponse"));
+			signedMessage += MessageFormat.format("&RelayState={0}", this.redirectMessage.get("RelayState"));
+			signedMessage += MessageFormat.format("&SigAlg={0}", this.redirectMessage.get("SigAlg"));
+		} else {
+			signedMessage = MessageFormat.format("SAMLResponse={0}", this.redirectMessage.get("SAMLResponse"));
+			signedMessage += MessageFormat.format("&SigAlg={0}", this.redirectMessage.get("SigAlg"));
+		}
+
+		byte[] query = signedMessage.getBytes(StandardCharsets.UTF_8);
+
+		X509Certificate cert = Keys.loadCertificate(certPath, certPass, certAlias);
+
+		try (InputStream inputStream = new ByteArrayInputStream(query)) {
+			String sigalg = URLDecoder.decode(this.redirectMessage.get("SigAlg"), StandardCharsets.UTF_8.name());
+			RSADigestSigner signer = new RSADigestSigner(Hash.getDigest(Hash.getHashFromSigAlg(sigalg)));
+			setUpSigner(signer, inputStream, Keys.getAsymmetricKeyParameter(cert), false);
+			return signer.verifySignature(signature);
+		} catch (Exception e) {
+			logger.error("VerifySignature_internal", e);
+			return false;
+		}
+	}
+
 	private static Map<String, String> parseRedirect(String request) {
 		logger.trace("parseRedirect");
 		Map<String, String> result = new HashMap<>();

gamsaml20/src/main/java/com/genexus/saml20/utils/DSig.java

@@ -14,6 +14,9 @@
 import javax.xml.xpath.XPathFactory;
 import java.security.cert.X509Certificate;
 import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 
 public class DSig {
 
@@ -26,7 +29,10 @@ public static boolean validateSignatures(Document xmlDoc, String certPath, Strin
 		NodeList nodes = findElementsByPath(xmlDoc, "//*[@ID]");
 
 		NodeList signatures = xmlDoc.getElementsByTagNameNS(Constants.SignatureSpecNS, Constants._TAG_SIGNATURE);
-
+		//check the message is signed - security measure
+		if(signatures.getLength() == 0){
+			return false;
+		}
 		for (int i = 0; i < signatures.getLength(); i++) {
 			Element signedElement = findNodeById(nodes, getSignatureID((Element) signatures.item(i)));
 			if (signedElement == null) {
@@ -35,6 +41,10 @@ public static boolean validateSignatures(Document xmlDoc, String certPath, Strin
 			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;
+				}
 				if (!signature.checkSignatureValue(cert)) {
 					return false;
 				}
@@ -46,6 +56,21 @@ public static boolean validateSignatures(Document xmlDoc, String certPath, Strin
 		return true;
 	}
 
+	private static boolean verifySignatureAlgorithm(Element elem) {
+		logger.trace("verifySignatureAlgorithm");
+		NodeList signatureMethod = elem.getElementsByTagNameNS(Constants.SignatureSpecNS, Constants._TAG_SIGNATUREMETHOD);
+		String signatureAlgorithm = signatureMethod.item(0).getAttributes().getNamedItem(Constants._ATT_ALGORITHM).getNodeValue();
+		logger.debug(MessageFormat.format("verifySignatureAlgorithm - algorithm: {0}", signatureAlgorithm));
+		String[] algorithm = signatureAlgorithm.split("#");
+		List<String> validAlgorithms = Arrays.asList("rsa-sha1", "rsa-sha256", "rsa-sha512");
+		for (String alg : validAlgorithms) {
+			if (algorithm[1].trim().equals(alg)) {
+				return true;
+			}
+		}
+		logger.error(MessageFormat.format("verifySignatureAlgorithm - Invalid Signature algorithm {0}", algorithm[1]));
+		return false;
+	}
 
 	private static String getSignatureID(Element signatureElement) {
 		return signatureElement.getElementsByTagNameNS(Constants.SignatureSpecNS, Constants._TAG_REFERENCE).item(0).getAttributes().getNamedItem(Constants._ATT_URI).getNodeValue();

gamsaml20/src/main/java/com/genexus/saml20/utils/Encoding.java

@@ -73,4 +73,16 @@ public static String documentToString(Document doc) {
 			return null;
 		}
 	}
+
+	public static byte[] decodeParameter(String parm) {
+		logger.trace("decodeParameter");
+		try {
+			String base64 = URLDecoder.decode(parm, StandardCharsets.UTF_8.name());
+			return Base64.decode(base64);
+		} catch (Exception e) {
+			logger.error("decodeParameter", e);
+			return null;
+		}
+
+	}
 }

gamsaml20/src/main/java/com/genexus/saml20/utils/Hash.java

@@ -72,4 +72,19 @@ public static Digest getDigest(Hash hash) {
 				return null;
 		}
 	}
+
+	public static Hash getHashFromSigAlg(String sigAlg) {
+		logger.trace("getHashFromSigAlg");
+		switch (sigAlg.trim()) {
+			case "http://www.w3.org/2001/04/xmldsig-more#rsa-sha1":
+				return SHA1;
+			case "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256":
+				return SHA256;
+			case "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512":
+				return SHA512;
+			default:
+				logger.error(MessageFormat.format("getHashFromSigAlg - not implemented signature algorithm: {0}", sigAlg));
+				return null;
+		}
+	}
 }

gamsaml20/src/main/java/com/genexus/saml20/utils/Keys.java

@@ -5,8 +5,10 @@
 import org.bouncycastle.asn1.ASN1InputStream;
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.pkcs.PrivateKeyInfo;
+import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
 import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
 import org.bouncycastle.crypto.util.PrivateKeyFactory;
+import org.bouncycastle.crypto.util.PublicKeyFactory;
 import org.bouncycastle.jcajce.provider.asymmetric.x509.CertificateFactory;
 import org.bouncycastle.util.encoders.Base64;
 
@@ -18,6 +20,7 @@
 import java.nio.file.Path;
 import java.security.KeyStore;
 import java.security.PrivateKey;
+import java.security.PublicKey;
 import java.security.cert.X509Certificate;
 import java.text.MessageFormat;
 
@@ -33,6 +36,18 @@ public static X509Certificate loadCertificate(String path, String alias, String
 		return isBase64(path) ? loadCertificateFromBase64(path) : loadCertificateFromJKS(path, alias, password);
 	}
 
+	public static AsymmetricKeyParameter getAsymmetricKeyParameter(X509Certificate cert) {
+		logger.trace("getAsymmetricKeyParameter");
+		try {
+			PublicKey publicKey = cert.getPublicKey();
+			SubjectPublicKeyInfo subjectPublicKeyInfo = SubjectPublicKeyInfo.getInstance(publicKey.getEncoded());
+			return PublicKeyFactory.createKey(subjectPublicKeyInfo);
+		} catch (Exception e) {
+			logger.error("getAsymmetricKeyParameter", e);
+			return null;
+		}
+	}
+
 	private static String getCertPath(String path) {
 		//boolean isAbsolute = new File(path).isAbsolute();