Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

See #294 Support Alg Deprecated rejection. #295

Merged
merged 2 commits into from
Nov 23, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ onelogin.saml2.idp.x509cert =
# let the toolkit know which Algorithm was used. Possible values: sha1, sha256, sha384 or sha512
# 'sha1' is the default value.
# onelogin.saml2.idp.certfingerprint =
# onelogin.saml2.idp.certfingerprint_algorithm = sha1
# onelogin.saml2.idp.certfingerprint_algorithm = sha256

# Security settings
#
Expand Down Expand Up @@ -342,7 +342,18 @@ onelogin.saml2.security.want_xml_validation = true
# 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'
# 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha384'
# 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha512'
onelogin.saml2.security.signature_algorithm = http://www.w3.org/2000/09/xmldsig#rsa-sha1
onelogin.saml2.security.signature_algorithm = http://www.w3.org/2001/04/xmldsig-more#rsa-sha256

# Algorithm that the toolkit will use on digest process. Options:
# 'http://www.w3.org/2000/09/xmldsig#sha1'
# 'http://www.w3.org/2001/04/xmlenc#sha256'
# 'http://www.w3.org/2001/04/xmldsig-more#sha384'
# 'http://www.w3.org/2001/04/xmlenc#sha512'
onelogin.saml2.security.digest_algorithm = http://www.w3.org/2001/04/xmlenc#sha256


# Reject Signatures with deprecated algorithms (sha1)
onelogin.saml2.security.reject_deprecated_alg = true

# Organization
onelogin.saml2.organization.name = SP Java
Expand All @@ -362,6 +373,7 @@ onelogin.saml2.contacts.support.email_address = [email protected]
# onelogin.saml2.unique_id_prefix = _
```


##### KeyStores

The Auth constructor supports the ability to read SP public cert/private key from a KeyStore. A KeyStoreSettings object must be provided with the KeyStore, the Alias and the KeyEntry password.
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/com/onelogin/saml2/authn/SamlResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -323,12 +323,14 @@ public boolean isValid(String requestId) {
String fingerprint = settings.getIdpCertFingerprint();
String alg = settings.getIdpCertFingerprintAlgorithm();

if (hasSignedResponse && !Util.validateSign(samlResponseDocument, certList, fingerprint, alg, Util.RESPONSE_SIGNATURE_XPATH)) {
Boolean rejectDeprecatedAlg = settings.getRejectDeprecatedAlg();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect an unboxed, non-nullable (primitive) bool here


if (hasSignedResponse && !Util.validateSign(samlResponseDocument, certList, fingerprint, alg, Util.RESPONSE_SIGNATURE_XPATH, rejectDeprecatedAlg)) {
throw new ValidationError("Signature validation failed. SAML Response rejected", ValidationError.INVALID_SIGNATURE);
}

final Document documentToCheckAssertion = encrypted ? decryptedDocument : samlResponseDocument;
if (hasSignedAssertion && !Util.validateSign(documentToCheckAssertion, certList, fingerprint, alg, Util.ASSERTION_SIGNATURE_XPATH)) {
if (hasSignedAssertion && !Util.validateSign(documentToCheckAssertion, certList, fingerprint, alg, Util.ASSERTION_SIGNATURE_XPATH, rejectDeprecatedAlg)) {
throw new ValidationError("Signature validation failed. SAML Response rejected", ValidationError.INVALID_SIGNATURE);
}
}
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/com/onelogin/saml2/logout/LogoutRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,17 @@ public Boolean isValid() throws Exception {
if (signAlg == null || signAlg.isEmpty()) {
signAlg = Constants.RSA_SHA1;
}

if (signAlg.equals(Constants.RSA_SHA1)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should DSA_SHA1 be rejected too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Boolean rejectDeprecatedAlg = settings.getRejectDeprecatedAlg();
if (rejectDeprecatedAlg) {
LOGGER.error("A deprecated algorithm (RSA_SHA1) found in the Signature element, rejecting it");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it can be confusing that there are 2 kinds of rejections, the other being:

			if (!isAlgorithmWhitelisted(sigMethodAlg)){
				throw new Exception(sigMethodAlg + " is not a valid supported algorithm");
			}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supported by the toolkit vs Acceptable

return false;
} else {
LOGGER.info("RSA_SHA1 alg found in a Signature element, consider request a more robust alg");
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe hide/share the common logic inside an utility method:

if (Util.mustRejectDeprecatedSignatureAlgo(signAlg, settings.getRejectDeprecatedAlg())
    return false;
private static final Set<String> DEPRECATED_ALGOS = new HashSet<>(Arrays.asList(
       Constants.RSA_SHA1, Constants.DSA_SHA1));

public boolean mustRejectDeprecatedSignatureAlgo(String signAlg, boolean rejectDeprecatedAlg) {
			if (DEPRECATED_ALGOS.contains(signAlg) {
					boolean rejectDeprecatedAlg = settings.getRejectDeprecatedAlg();
					if (rejectDeprecatedAlg) {
						LOGGER.error("A deprecated algorithm (xxx_SHA1) found in the Signature element, rejecting it");
						return true; // must reject
					} else {
						LOGGER.info("xxx_SHA1 alg found in a Signature element, consider request a more robust alg");
 						return false; // can continue       
					}
				}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good


String relayState = request.getEncodedParameter("RelayState");

String signedQuery = "SAMLRequest=" + request.getEncodedParameter("SAMLRequest");
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/com/onelogin/saml2/logout/LogoutResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ public Boolean isValid(String requestId) {
signAlg = Constants.RSA_SHA1;
}

if (signAlg.equals(Constants.RSA_SHA1)) {
Boolean rejectDeprecatedAlg = settings.getRejectDeprecatedAlg();
if (rejectDeprecatedAlg) {
LOGGER.error("A deprecated algorithm (RSA_SHA1) found in the Signature element, rejecting it");
return false;
} else {
LOGGER.info("RSA_SHA1 alg found in a Signature element, consider request a more robust alg");
}
}

String signedQuery = "SAMLResponse=" + request.getEncodedParameter("SAMLResponse");

String relayState = request.getEncodedParameter("RelayState");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public class Saml2Settings {
private String digestAlgorithm = Constants.SHA1;
private boolean rejectUnsolicitedResponsesWithInResponseTo = false;
private boolean allowRepeatAttributeName = false;
private boolean rejectDeprecatedAlg = false;
private String uniqueIDPrefix = null;

// Compress
Expand Down Expand Up @@ -140,10 +141,17 @@ public final String getSpNameIDFormat() {
/**
* @return the allowRepeatAttributeName setting value
*/
public boolean isAllowRepeatAttributeName () {
public boolean isAllowRepeatAttributeName() {
return allowRepeatAttributeName;
}

/**
* @return the rejectDeprecatedAlg setting value
*/
public boolean getRejectDeprecatedAlg() {
return rejectDeprecatedAlg;
}

/**
* @return the spX509cert setting value
*/
Expand Down Expand Up @@ -478,6 +486,16 @@ public void setAllowRepeatAttributeName (boolean allowRepeatAttributeName) {
this.allowRepeatAttributeName = allowRepeatAttributeName;
}

/**
* Set the rejectDeprecatedAlg setting value
*
* @param rejectDeprecatedAlg
* the rejectDeprecatedAlg value to be set
*/
public void setRejectDeprecatedAlg (boolean rejectDeprecatedAlg) {
this.rejectDeprecatedAlg = rejectDeprecatedAlg;
}

/**
* Set the spX509cert setting value provided as X509Certificate object
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ public class SettingsBuilder {
public final static String SECURITY_DIGEST_ALGORITHM = "onelogin.saml2.security.digest_algorithm";
public final static String SECURITY_REJECT_UNSOLICITED_RESPONSES_WITH_INRESPONSETO = "onelogin.saml2.security.reject_unsolicited_responses_with_inresponseto";
public final static String SECURITY_ALLOW_REPEAT_ATTRIBUTE_NAME_PROPERTY_KEY = "onelogin.saml2.security.allow_duplicated_attribute_name";
public final static String SECURITY_REJECT_DEPRECATED_ALGORITHM = "onelogin.saml2.security.reject_deprecated_alg";

// Compress
public final static String COMPRESS_REQUEST = "onelogin.saml2.compress.request";
Expand Down Expand Up @@ -376,8 +377,14 @@ private void loadSecuritySetting() {
}

Boolean allowRepeatAttributeName = loadBooleanProperty(SECURITY_ALLOW_REPEAT_ATTRIBUTE_NAME_PROPERTY_KEY);
if (allowRepeatAttributeName != null)
if (allowRepeatAttributeName != null) {
saml2Setting.setAllowRepeatAttributeName(allowRepeatAttributeName);
}

Boolean rejectDeprecatedAlg = loadBooleanProperty(SECURITY_REJECT_DEPRECATED_ALGORITHM);
if (rejectDeprecatedAlg != null) {
saml2Setting.setRejectDeprecatedAlg(rejectDeprecatedAlg);
}
}

/**
Expand Down
94 changes: 91 additions & 3 deletions core/src/main/java/com/onelogin/saml2/util/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -923,13 +923,30 @@ public static boolean validateSign(final Document doc, final X509Certificate cer
*/
public static boolean validateSign(final Document doc, final List<X509Certificate> certList, final String fingerprint,
final String alg, final String xpath) {
return validateSign(doc, certList, fingerprint,alg, xpath, false);
}

/**
* Validate the signature pointed to by the xpath
*
* @param doc The document we should validate
* @param certList The public certificates
* @param fingerprint The fingerprint of the public certificate
* @param alg The signature algorithm method
* @param xpath the xpath of the ds:Signture node to validate
* @param rejectDeprecatedAlg Flag to invalidate or not Signatures with deprecated alg
*
* @return True if the signature exists and is valid, false otherwise.
*/
public static boolean validateSign(final Document doc, final List<X509Certificate> certList, final String fingerprint,
final String alg, final String xpath, final Boolean rejectDeprecatedAlg) {
try {
final NodeList signatures = query(doc, xpath);

if (signatures.getLength() == 1) {
final Node signNode = signatures.item(0);

Map<String,Object> signatureData = getSignatureData(signNode, alg);
Map<String,Object> signatureData = getSignatureData(signNode, alg, rejectDeprecatedAlg);
if (signatureData.isEmpty()) {
return false;
}
Expand Down Expand Up @@ -984,6 +1001,26 @@ public static boolean validateSign(final Document doc, final List<X509Certificat
* @return True if the sign is valid, false otherwise.
*/
public static Boolean validateMetadataSign(Document doc, X509Certificate cert, String fingerprint, String alg) {
return validateMetadataSign(doc, cert, fingerprint, alg, false);
}

/**
* Validate signature (Metadata).
*
* @param doc
* The document we should validate
* @param cert
* The public certificate
* @param fingerprint
* The fingerprint of the public certificate
* @param alg
* The signature algorithm method
* @param rejectDeprecatedAlg
* Flag to invalidate or not Signatures with deprecated alg
*
* @return True if the sign is valid, false otherwise.
*/
public static Boolean validateMetadataSign(Document doc, X509Certificate cert, String fingerprint, String alg, Boolean rejectDeprecatedAlg) {
NodeList signNodesToValidate;
try {
signNodesToValidate = query(doc, "/md:EntitiesDescriptor/ds:Signature");
Expand All @@ -999,7 +1036,7 @@ public static Boolean validateMetadataSign(Document doc, X509Certificate cert, S
if (signNodesToValidate.getLength() > 0) {
for (int i = 0; i < signNodesToValidate.getLength(); i++) {
Node signNode = signNodesToValidate.item(i);
if (!validateSignNode(signNode, cert, fingerprint, alg)) {
if (!validateSignNode(signNode, cert, fingerprint, alg, rejectDeprecatedAlg)) {
return false;
}
}
Expand All @@ -1026,6 +1063,26 @@ public static Boolean validateMetadataSign(Document doc, X509Certificate cert, S
* @return True if the sign is valid, false otherwise.
*/
private static Map<String,Object> getSignatureData(Node signNode, String alg) {
return getSignatureData(signNode, alg, false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a bit risky to have an implicit rejectDeprecatedAlg=false no? (the safer flag true could be accidentally ignored)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't set it true, I need to maintain backward compatibility. But notice that in the settings file I added the rejection of deprecated alg as default.

}

/**
* Validate signature (Metadata).
*
* @param doc
* The document we should validate
* @param cert
* The public certificate
* @param fingerprint
* The fingerprint of the public certificate
* @param alg
* The signature algorithm method
* @param rejectDeprecatedAlg
* Flag to invalidate or not Signatures with deprecated alg
*
* @return True if the sign is valid, false otherwise.
*/
private static Map<String,Object> getSignatureData(Node signNode, String alg, Boolean rejectDeprecatedAlg) {
Map<String,Object> signatureData = new HashMap<>();
try {
Element sigElement = (Element) signNode;
Expand All @@ -1036,6 +1093,15 @@ private static Map<String,Object> getSignatureData(Node signNode, String alg) {
throw new Exception(sigMethodAlg + " is not a valid supported algorithm");
}

if (sigMethodAlg.equals(Constants.RSA_SHA1)) {
if (rejectDeprecatedAlg) {
LOGGER.error("A deprecated algorithm (RSA_SHA1) found in the Signature element, rejecting it");
return signatureData;
} else {
LOGGER.info("RSA_SHA1 alg found in a Signature element, consider request a more robust alg");
}
}

signatureData.put("signature", signature);

String extractedFingerprint = null;
Expand Down Expand Up @@ -1073,7 +1139,29 @@ private static Map<String,Object> getSignatureData(Node signNode, String alg) {
* @throws Exception
*/
public static Boolean validateSignNode(Node signNode, X509Certificate cert, String fingerprint, String alg) {
Map<String,Object> signatureData = getSignatureData(signNode, alg);
return validateSignNode(signNode, cert, fingerprint, alg, false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other comment about forced flag

}

/**
* Validate signature of the Node.
*
* @param signNode
* The document we should validate
* @param cert
* The public certificate
* @param fingerprint
* The fingerprint of the public certificate
* @param alg
* The signature algorithm method
* @param rejectDeprecatedAlg
* Flag to invalidate or not Signatures with deprecated alg
*
* @return True if the sign is valid, false otherwise.
*
* @throws Exception
*/
public static Boolean validateSignNode(Node signNode, X509Certificate cert, String fingerprint, String alg, Boolean rejectDeprecatedAlg) {
Map<String,Object> signatureData = getSignatureData(signNode, alg, rejectDeprecatedAlg);
if (signatureData.isEmpty()) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.onelogin.saml2.exception.SettingsException;
import com.onelogin.saml2.exception.ValidationError;
import com.onelogin.saml2.http.HttpRequest;
import com.onelogin.saml2.logout.LogoutRequest;
import com.onelogin.saml2.model.SamlResponseStatus;
import com.onelogin.saml2.settings.Saml2Settings;
import com.onelogin.saml2.settings.SettingsBuilder;
Expand Down Expand Up @@ -2409,6 +2410,54 @@ public void testIsValid_signedEncryptedAssertion() throws Exception {
assertResponseValid(settings, samlResponseEncoded, true, false, "The Message of the Response is not signed and the SP requires it");
}

/**
* Tests the isValid method of SamlResponse
* Case: Signed with deprecated method and flag enabled
*
* @throws ValidationError
* @throws SettingsException
* @throws IOException
* @throws SAXException
* @throws ParserConfigurationException
* @throws XPathExpressionException
* @throws Error
*
* @see com.onelogin.saml2.authn.SamlResponse#isValid
*/
@Test
public void testIsInValidSignWithDeprecatedAlg() throws IOException, Error, XPathExpressionException, ParserConfigurationException, SAXException, SettingsException, ValidationError {
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
settings.setWantAssertionsSigned(false);
settings.setWantMessagesSigned(false);
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_response.xml.base64");

settings.setStrict(true);
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertTrue(samlResponse.isValid());

settings.setRejectDeprecatedAlg(true);
SamlResponse samlResponse2 = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertFalse(samlResponse2.isValid());

settings.setRejectDeprecatedAlg(false);
samlResponseEncoded = Util.getFileAsString("data/responses/signed_assertion_response.xml.base64");
SamlResponse samlResponse3 = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertTrue(samlResponse3.isValid());

settings.setRejectDeprecatedAlg(true);
SamlResponse samlResponse4 = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertFalse(samlResponse4.isValid());

settings.setRejectDeprecatedAlg(false);
samlResponseEncoded = Util.getFileAsString("data/responses/double_signed_response.xml.base64");
SamlResponse samlResponse5 = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertTrue(samlResponse5.isValid());

settings.setRejectDeprecatedAlg(true);
SamlResponse samlResponse6 = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
assertFalse(samlResponse6.isValid());
}

/**
* Tests the isValid method of SamlResponse
* Case: valid sign response / sign assertion / both signed
Expand Down
Loading