CVE-2020-5268: Improper Authentication (CWE-287)
value: In Saml2 Authentication Services for ASP.NET versions before 1.0.2, and between 2.0.0 and 2.6.0, there is a vulnerability in how tokens are validated in some cases. Saml2 tokens are usually used as bearer tokens - a caller that presents a token is assumed to be the subject of the token. There is also support in the Saml2 protocol for issuing tokens that is tied to a subject through other means, e.g. holder-of-key where possession of a private key must be proved. The Sustainsys.Saml2 library incorrectly treats all incoming tokens as bearer tokens, even though they have another subject confirmation method specified. This could be used by an attacker that could get access to Saml2 tokens with another subject confirmation method than bearer. The attacker could then use such a token to create a log in session. This vulnerability is patched in versions 1.0.2 and 2.7.0.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
CVSS v3 base score: 7.3
Published: 2020-04-21T17:15Z
Repository: https://github.com/Sustainsys/Saml2
Language: C#
Files changed: Sustainsys.Saml2/Configuration/SPOptions.cs, Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs, Sustainsys.Saml2/SAML2P/Saml2Response.cs, Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs, Tests/Tests.Shared/WebSSO/Saml2ArtifactBindingTests.cs
Commit date: 2020-03-24 11:56:25 +0100
Commit message: Fix token replay detection
- Fixes #1107
- Fixes #711

Diff: diff --git a/Sustainsys.Saml2/Configuration/SPOptions.cs b/Sustainsys.Saml2/Configuration/SPOptions.cs
index 738fdf5..16eec9f 100644
--- a/Sustainsys.Saml2/Configuration/SPOptions.cs
+++ b/Sustainsys.Saml2/Configuration/SPOptions.cs
@@ -1,7 +1,9 @@
-﻿using Sustainsys.Saml2.Metadata;
+﻿using Microsoft.IdentityModel.Tokens;
+using Sustainsys.Saml2.Metadata;
 using Sustainsys.Saml2.Saml2P;
+using Sustainsys.Saml2.Tokens;
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.Linq;
@@ -20,11 +22,11 @@ namespace Sustainsys.Saml2.Configuration
         /// <summary>
         /// Ctor
         /// </summary>
         public SPOptions()
         {
-			MetadataCacheDuration = new XsdDuration(hours: 1);
+            MetadataCacheDuration = new XsdDuration(hours: 1);
             Compatibility = new Compatibility();
             OutboundSigningAlgorithm = XmlHelpers.GetDefaultSigningAlgorithmName();
             MinIncomingSigningAlgorithm = XmlHelpers.GetDefaultSigningAlgorithmName();
         }
 
@@ -100,21 +102,21 @@ namespace Sustainsys.Saml2.Configuration
             get
             {
                 // Capture in a local variable to prevent race conditions. Reads and writes
                 // of references are atomic so there is no need for a lock.
                 var value = saml2PSecurityTokenHandler;
-                if(value == null)
+                if (value == null)
                 {
                     // Set the saved value, but don't trust it - still use a local var for the return.
                     saml2PSecurityTokenHandler = value = new Saml2PSecurityTokenHandler(this);
                 }
 
                 return value;
             }
             set
             {
-                saml2PSecurityTokenHandler = value; 
+                saml2PSecurityTokenHandler = value;
             }
         }
 
         /// <summary>
         /// Url to discovery service to use if no idp is specified in the sign in call.
@@ -133,11 +135,11 @@ namespace Sustainsys.Saml2.Configuration
             {
                 return entityId;
             }
             set
             {
-                if(saml2PSecurityTokenHandler != null)
+                if (saml2PSecurityTokenHandler != null)
                 {
                     throw new InvalidOperationException("Can't change entity id when a token handler has been instantiated.");
                 }
                 entityId = value;
             }
@@ -155,11 +157,11 @@ namespace Sustainsys.Saml2.Configuration
             {
                 return modulePath;
             }
             set
             {
-                if(value == null)
+                if (value == null)
                 {
                     throw new ArgumentNullException(nameof(value));
                 }
 
                 value = value.TrimEnd('/');
@@ -210,11 +212,11 @@ namespace Sustainsys.Saml2.Configuration
                 return contacts;
             }
         }
 
         readonly ICollection<AttributeConsumingService> attributeConsumingServices
-			= new List<AttributeConsumingService>();
+            = new List<AttributeConsumingService>();
 
         /// <summary>
         /// Collection of attribute consuming services for the service provider.
         /// </summary>
         public ICollection<AttributeConsumingService> AttributeConsumingServices
@@ -290,11 +292,11 @@ namespace Sustainsys.Saml2.Configuration
                     ).ToList();
 
                 var futureBothCertExists = metaDataCertificates
                     .Any(c => c.Status == CertificateStatus.Future && c.Use == CertificateUse.Both);
 
-                foreach(var cert in metaDataCertificates)
+                foreach (var cert in metaDataCertificates)
                 {
                     // Just like we stop publishing Encryption cert immediately when a Future one is added,
                     // in the case of a "Both" cert we should switch the current use to Signing so that Idp's stop sending
                     // us certs encrypted with the old key
                     if (cert.Use == CertificateUse.Both && cert.Status == CertificateStatus.Current && futureBothCertExists)
@@ -356,11 +358,11 @@ namespace Sustainsys.Saml2.Configuration
         /// <summary>
         /// Signing algorithm for metadata and outbound messages. Can be 
         /// overriden for each <see cref="IdentityProvider"/>.
         /// </summary>
         public string OutboundSigningAlgorithm { get; set; }
-        
+
         /// <summary>
         /// Metadata flag that we want assertions to be signed.
         /// </summary>
         public bool WantAssertionsSigned { get; set; }
 
@@ -377,11 +379,11 @@ namespace Sustainsys.Saml2.Configuration
         /// certain non-standard behaviour.
         /// </summary>
         public Compatibility Compatibility { get; set; }
 
         private string minIncomingSigningAlgorithm;
-        
+
         /// <summary>
         /// Minimum accepted signature algorithm for any incoming messages.
         /// </summary>
         public string MinIncomingSigningAlgorithm
         {
@@ -389,11 +391,11 @@ namespace Sustainsys.Saml2.Configuration
             {
                 return minIncomingSigningAlgorithm;
             }
             set
             {
-                if(!XmlHelpers.KnownSigningAlgorithms.Contains(value))
+                if (!XmlHelpers.KnownSigningAlgorithms.Contains(value))
                 {
                     throw new ArgumentException("The signing algorithm " + value +
                         " is unknown or not supported by the current .NET Framework.");
                 }
                 minIncomingSigningAlgorithm = value;
@@ -402,7 +404,24 @@ namespace Sustainsys.Saml2.Configuration
 
         /// <summary>
         /// Adapter to logging framework of hosting application.
         /// </summary>
         public ILoggerAdapter Logger { get; set; }
-    }
+
+        private ITokenReplayCache tokenReplayCache;
+        public ITokenReplayCache TokenReplayCache 
+        { 
+            get
+            {
+                if(tokenReplayCache == null)
+                {
+                    tokenReplayCache = new TokenReplayCache();
+                }
+                return tokenReplayCache;
+            }
+            set
+            {
+                tokenReplayCache = value;
+            }
+        }
+}
 }
diff --git a/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs b/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs
index 24a9b2f..976fae2 100644
--- a/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs
+++ b/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs
@@ -26,23 +26,10 @@ namespace Sustainsys.Saml2.Saml2P
 		public Saml2PSecurityTokenHandler(SPOptions spOptions)
 		{
 			Serializer = new Saml2PSerializer(spOptions);
 		}
 
-		// Overridden to fix the fact that the base class version uses NotBefore as the token replay expiry time
-		// Due to the fact that we can't override the ValidateToken function (it's overridden in the base class!)
-		// we have to parse the token again.
-		// This can be removed when:
-		// https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/898
-		// is fixed.
-		protected override void ValidateTokenReplay(DateTime? expirationTime, string securityToken, TokenValidationParameters validationParameters)
-		{
-			var saml2Token = ReadSaml2Token(securityToken);
-			base.ValidateTokenReplay(saml2Token.Assertion.Conditions.NotOnOrAfter,
-				securityToken, validationParameters);
-		}
-
 		// TODO: needed with Microsoft.identitymodel?
 		/// <summary>
 		/// Process authentication statement from SAML assertion. WIF chokes if the authentication statement 
 		/// contains a DeclarationReference, so we clear this out before calling the base method
 		/// http://referencesource.microsoft.com/#System.IdentityModel/System/IdentityModel/Tokens/Saml2SecurityTokenHandler.cs,1970
@@ -82,12 +69,29 @@ namespace Sustainsys.Saml2.Saml2P
                 subject.AddClaim(
                     new Claim(Saml2ClaimTypes.SessionIndex, statement.SessionIndex, null, issuer));
             }
         }
 
-		protected override Saml2SecurityToken ValidateSignature(string token, TokenValidationParameters validationParameters)
+		// Override and build our own logic. The problem is ValidateTokenReplay that serializes the token back. And that
+		// breaks because it expects some optional values to be present.
+		public override ClaimsPrincipal ValidateToken(string token, TokenValidationParameters validationParameters, out Microsoft.IdentityModel.Tokens.SecurityToken validatedToken)
 		{
-			// Just skip signature validation -- we do this elsewhere
-			return ReadSaml2Token(token);
+			var samlToken = ReadSaml2Token(token);
+
+			ValidateConditions(samlToken, validationParameters);
+			ValidateSubject(samlToken, validationParameters);
+
+			var issuer = ValidateIssuer(samlToken.Issuer, samlToken, validationParameters);
+
+			// Just using the assertion id for token replay. As that is part of the signed value it cannot
+			// be altered by someone replaying the token.
+			ValidateTokenReplay(samlToken.Assertion.Conditions.NotOnOrAfter, samlToken.Assertion.Id.Value, validationParameters);
+
+			// ValidateIssuerSecurityKey not called - we have our own signature validation.
+
+			validatedToken = samlToken;
+			var identity = CreateClaimsIdentity(samlToken, issuer, validationParameters);
+
+			return new ClaimsPrincipal(identity);
 		}
-    }
+	}
 }
diff --git a/Sustainsys.Saml2/SAML2P/Saml2Response.cs b/Sustainsys.Saml2/SAML2P/Saml2Response.cs
index 66917a5..c8574e2 100644
--- a/Sustainsys.Saml2/SAML2P/Saml2Response.cs
+++ b/Sustainsys.Saml2/SAML2P/Saml2Response.cs
@@ -595,10 +595,13 @@ namespace Sustainsys.Saml2.Saml2P
 			TokenValidationParameters validationParameters = new TokenValidationParameters();
 			validationParameters.AuthenticationType = "Federation";
 			validationParameters.RequireSignedTokens = false;
 			validationParameters.ValidateIssuer = false;
             validationParameters.ValidAudience = options.SPOptions.EntityId.Id;
+            validationParameters.RequireAudience = false; // Audience restriction optional in SAML2 spec.
+            validationParameters.TokenReplayCache = options.SPOptions.TokenReplayCache;
+            validationParameters.ValidateTokenReplay = true;
 
             options.Notifications.Unsafe.TokenValidationParametersCreated(validationParameters, idp, XmlElement);
 
 			var handler = options.SPOptions.Saml2PSecurityTokenHandler;
 
diff --git a/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs b/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs
index 8f9f7f9..d41d58a 100644
--- a/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs
+++ b/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs
@@ -1916,12 +1916,10 @@ namespace Sustainsys.Saml2.Tests.Saml2P
         }
 
         [TestMethod]
         public void Saml2Response_GetClaims_ThrowsOnReplayAssertionId()
         {
-            Assert.Inconclusive("Deliberately ignored test for now");
-
             var response =
             @"<?xml version=""1.0"" encoding=""UTF-8""?>
             <saml2p:Response xmlns:saml2p=""urn:oasis:names:tc:SAML:2.0:protocol""
             xmlns:saml2=""urn:oasis:names:tc:SAML:2.0:assertion""
             ID = """ + MethodBase.GetCurrentMethod().Name + @""" Version=""2.0"" IssueInstant=""2013-01-01T00:00:00Z"">
@@ -1953,12 +1951,10 @@ namespace Sustainsys.Saml2.Tests.Saml2P
         }
 
         [TestMethod]
         public void Saml2Response_GetClaims_ThrowsOnReplayAssertionIdSameConfig()
         {
-            Assert.Inconclusive("Ingored for now");
-
             var response =
             @"<?xml version=""1.0"" encoding=""UTF-8""?>
             <saml2p:Response xmlns:saml2p=""urn:oasis:names:tc:SAML:2.0:protocol""
             xmlns:saml2=""urn:oasis:names:tc:SAML:2.0:assertion""
             ID = """ + MethodBase.GetCurrentMethod().Name + @""" Version=""2.0"" IssueInstant=""2013-01-01T00:00:00Z"">
diff --git a/Tests/Tests.Shared/WebSSO/Saml2ArtifactBindingTests.cs b/Tests/Tests.Shared/WebSSO/Saml2ArtifactBindingTests.cs
index 7c1f9dc..150a324 100644
--- a/Tests/Tests.Shared/WebSSO/Saml2ArtifactBindingTests.cs
+++ b/Tests/Tests.Shared/WebSSO/Saml2ArtifactBindingTests.cs
@@ -137,12 +137,10 @@ namespace Sustainsys.Saml2.Tests.WebSso
                 new Uri($"http://example.com/path/acs?SAMLart={artifact}&RelayState={relayState}"),
                 null,
                 null,
                 new StoredRequestState(issuer, null, null, null));
 
-            StubServer.LastArtifactResolutionSoapActionHeader = null;
-
             var result = Saml2Binding.Get(Saml2BindingType.Artifact).Unbind(r, StubFactory.CreateOptions());
 
             var xmlDocument = XmlHelpers.XmlDocumentFromString(
                 "<message>   <child-node /> </message>");
 

Vulnerable code: ﻿using Sustainsys.Saml2.Metadata;
			MetadataCacheDuration = new XsdDuration(hours: 1);
                if(value == null)
                saml2PSecurityTokenHandler = value; 
                if(saml2PSecurityTokenHandler != null)
                if(value == null)
			= new List<AttributeConsumingService>();
                foreach(var cert in metaDataCertificates)
        
        
                if(!XmlHelpers.KnownSigningAlgorithms.Contains(value))
    }
		// Overridden to fix the fact that the base class version uses NotBefore as the token replay expiry time
		// Due to the fact that we can't override the ValidateToken function (it's overridden in the base class!)
		// we have to parse the token again.
		// This can be removed when:
		// https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/898
		// is fixed.
		protected override void ValidateTokenReplay(DateTime? expirationTime, string securityToken, TokenValidationParameters validationParameters)
		{
			var saml2Token = ReadSaml2Token(securityToken);
			base.ValidateTokenReplay(saml2Token.Assertion.Conditions.NotOnOrAfter,
				securityToken, validationParameters);
		}

		protected override Saml2SecurityToken ValidateSignature(string token, TokenValidationParameters validationParameters)
			// Just skip signature validation -- we do this elsewhere
			return ReadSaml2Token(token);
    }
            Assert.Inconclusive("Deliberately ignored test for now");

            Assert.Inconclusive("Ingored for now");

            StubServer.LastArtifactResolutionSoapActionHeader = null;

Fixed code: ﻿using Microsoft.IdentityModel.Tokens;
using Sustainsys.Saml2.Metadata;
using Sustainsys.Saml2.Tokens;
            MetadataCacheDuration = new XsdDuration(hours: 1);
                if (value == null)
                saml2PSecurityTokenHandler = value;
                if (saml2PSecurityTokenHandler != null)
                if (value == null)
            = new List<AttributeConsumingService>();
                foreach (var cert in metaDataCertificates)


                if (!XmlHelpers.KnownSigningAlgorithms.Contains(value))

        private ITokenReplayCache tokenReplayCache;
        public ITokenReplayCache TokenReplayCache 
        { 
            get
            {
                if(tokenReplayCache == null)
                {
                    tokenReplayCache = new TokenReplayCache();
                }
                return tokenReplayCache;
            }
            set
            {
                tokenReplayCache = value;
            }
        }
}
		// Override and build our own logic. The problem is ValidateTokenReplay that serializes the token back. And that
		// breaks because it expects some optional values to be present.
		public override ClaimsPrincipal ValidateToken(string token, TokenValidationParameters validationParameters, out Microsoft.IdentityModel.Tokens.SecurityToken validatedToken)
			var samlToken = ReadSaml2Token(token);

			ValidateConditions(samlToken, validationParameters);
			ValidateSubject(samlToken, validationParameters);

			var issuer = ValidateIssuer(samlToken.Issuer, samlToken, validationParameters);

			// Just using the assertion id for token replay. As that is part of the signed value it cannot
			// be altered by someone replaying the token.
			ValidateTokenReplay(samlToken.Assertion.Conditions.NotOnOrAfter, samlToken.Assertion.Id.Value, validationParameters);

			// ValidateIssuerSecurityKey not called - we have our own signature validation.

			validatedToken = samlToken;
			var identity = CreateClaimsIdentity(samlToken, issuer, validationParameters);

			return new ClaimsPrincipal(identity);
	}
            validationParameters.RequireAudience = false; // Audience restriction optional in SAML2 spec.
            validationParameters.TokenReplayCache = options.SPOptions.TokenReplayCache;
            validationParameters.ValidateTokenReplay = true;