Authentication requests now only sign their return_to parameters if a security-critical callback parameter is in it.

8 files changed, 77 insertions, 12 deletions

diff --git a/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToNonceBindingElement.cs b/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToNonceBindingElement.cs
index 9c4c46d..1b6f68d 100644
--- a/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToNonceBindingElement.cs
+++ b/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToNonceBindingElement.cs
@@ -144,6 +144,7 @@ namespace DotNetOpenAuth.OpenId.ChannelElements {
 			SignedResponseRequest request = message as SignedResponseRequest;
 			if (this.UseRequestNonce(request)) {
 				request.AddReturnToArguments(NonceParameter, CustomNonce.NewNonce().Serialize());
+				request.SignReturnTo = true; // a nonce without a signature is completely pointless
 
 				return MessageProtections.ReplayProtection;
 			}
@@ -171,9 +172,13 @@ namespace DotNetOpenAuth.OpenId.ChannelElements {
 		public MessageProtections? ProcessIncomingMessage(IProtocolMessage message) {
 			IndirectSignedResponse response = message as IndirectSignedResponse;
 			if (this.UseRequestNonce(response)) {
+				if (!response.ReturnToParametersSignatureValidated) {
+					Logger.OpenId.Error("Incoming message is expected to have a nonce, but the return_to parameter is not signed.");
+				}
+
 				string nonceValue = response.GetReturnToArgument(NonceParameter);
 				ErrorUtilities.VerifyProtocol(
-					nonceValue != null,
+					nonceValue != null && response.ReturnToParametersSignatureValidated,
 					this.securitySettings.RejectUnsolicitedAssertions ? OpenIdStrings.UnsolicitedAssertionsNotAllowed : OpenIdStrings.UnsolicitedAssertionsNotAllowedFrom1xOPs);
 
 				CustomNonce nonce = CustomNonce.Deserialize(nonceValue);
diff --git a/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToSignatureBindingElement.cs b/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToSignatureBindingElement.cs
index a760c0d..abe31ba 100644
--- a/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToSignatureBindingElement.cs
+++ b/src/DotNetOpenAuth/OpenId/ChannelElements/ReturnToSignatureBindingElement.cs
@@ -94,7 +94,7 @@ namespace DotNetOpenAuth.OpenId.ChannelElements {
 		/// </remarks>
 		public MessageProtections? ProcessOutgoingMessage(IProtocolMessage message) {
 			SignedResponseRequest request = message as SignedResponseRequest;
-			if (request != null && request.ReturnTo != null) {
+			if (request != null && request.ReturnTo != null && request.SignReturnTo) {
 				request.AddReturnToArguments(ReturnToSignatureHandleParameterName, this.secretManager.CurrentHandle);
 				request.AddReturnToArguments(ReturnToSignatureParameterName, this.GetReturnToSignature(request.ReturnTo));
 
diff --git a/src/DotNetOpenAuth/OpenId/Messages/SignedResponseRequest.cs b/src/DotNetOpenAuth/OpenId/Messages/SignedResponseRequest.cs
index 1096468..b10670e 100644
--- a/src/DotNetOpenAuth/OpenId/Messages/SignedResponseRequest.cs
+++ b/src/DotNetOpenAuth/OpenId/Messages/SignedResponseRequest.cs
@@ -107,6 +107,11 @@ namespace DotNetOpenAuth.OpenId.Messages {
 		internal Realm Realm { get; set; }
 
 		/// <summary>
+		/// Gets or sets a value indicating whether the return_to value should be signed.
+		/// </summary>
+		internal bool SignReturnTo { get; set; }
+
+		/// <summary>
 		/// Checks the message state for conformity to the protocol specification
 		/// and throws an exception if the message is invalid.
 		/// </summary>
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
index d7345ac..e1d34c6 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
@@ -55,6 +55,16 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		private Dictionary<string, string> returnToArgs = new Dictionary<string, string>();
 
 		/// <summary>
+		/// A value indicating whether the return_to callback arguments must be signed.
+		/// </summary>
+		/// <remarks>
+		/// This field defaults to false, but is set to true as soon as the first callback argument
+		/// is added that indicates it must be signed.  At which point, all arguments are signed
+		/// even if individual ones did not need to be.
+		/// </remarks>
+		private bool returnToArgsMustBeSigned;
+
+		/// <summary>
 		/// Initializes a new instance of the <see cref="AuthenticationRequest"/> class.
 		/// </summary>
 		/// <param name="endpoint">The endpoint that describes the OpenID Identifier and Provider that will complete the authentication.</param>
@@ -200,7 +210,9 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// </remarks>
 		public void AddCallbackArguments(IDictionary<string, string> arguments) {
 			ErrorUtilities.VerifyArgumentNotNull(arguments, "arguments");
+			ErrorUtilities.VerifyOperation(this.RelyingParty.CanSignCallbackArguments, OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 
+			this.returnToArgsMustBeSigned = true;
 			foreach (var pair in arguments) {
 				ErrorUtilities.VerifyArgument(!string.IsNullOrEmpty(pair.Key), MessagingStrings.UnexpectedNullOrEmptyKey);
 				ErrorUtilities.VerifyArgument(pair.Value != null, MessagingStrings.UnexpectedNullValue, pair.Key);
@@ -227,7 +239,9 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		public void AddCallbackArguments(string key, string value) {
 			ErrorUtilities.VerifyNonZeroLength(key, "key");
 			ErrorUtilities.VerifyArgumentNotNull(value, "value");
+			ErrorUtilities.VerifyOperation(this.RelyingParty.CanSignCallbackArguments, OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 
+			this.returnToArgsMustBeSigned = true;
 			this.returnToArgs.Add(key, value);
 		}
 
@@ -250,6 +264,29 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			ErrorUtilities.VerifyArgumentNotNull(value, "value");
 			ErrorUtilities.VerifyOperation(this.RelyingParty.CanSignCallbackArguments, OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 
+			this.returnToArgsMustBeSigned = true;
+			this.returnToArgs[key] = value;
+		}
+
+		/// <summary>
+		/// Makes a key/value pair available when the authentication is completed without
+		/// requiring a return_to signature to protect against tampering of the callback argument.
+		/// </summary>
+		/// <param name="key">The parameter name.</param>
+		/// <param name="value">The value of the argument.  Must not be null.</param>
+		/// <remarks>
+		/// 	<para>Note that these values are NOT protected against eavesdropping or tampering in transit.  No
+		/// security-sensitive data should be stored using this method. </para>
+		/// 	<para>The value stored here can be retrieved using
+		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>.</para>
+		/// 	<para>Since the data set here is sent in the querystring of the request and some
+		/// servers place limits on the size of a request URL, this data should be kept relatively
+		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
+		/// </remarks>
+		public void SetUntrustedCallbackArgument(string key, string value) {
+			ErrorUtilities.VerifyNonZeroLength(key, "key");
+			ErrorUtilities.VerifyArgumentNotNull(value, "value");
+
 			this.returnToArgs[key] = value;
 		}
 
@@ -498,6 +535,7 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			request.Realm = this.Realm;
 			request.ReturnTo = this.ReturnToUrl;
 			request.AssociationHandle = association != null ? association.Handle : null;
+			request.SignReturnTo = this.returnToArgsMustBeSigned;
 			request.AddReturnToArguments(this.returnToArgs);
 			if (this.endpoint.UserSuppliedIdentifier != null) {
 				request.AddReturnToArguments(UserSuppliedIdentifierParameterName, this.endpoint.UserSuppliedIdentifier.OriginalString);
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
index 58b412f..1a0ff5b 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
@@ -143,6 +143,23 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		void SetCallbackArgument(string key, string value);
 
 		/// <summary>
+		/// Makes a key/value pair available when the authentication is completed without
+		/// requiring a return_to signature to protect against tampering of the callback argument.
+		/// </summary>
+		/// <param name="key">The parameter name.</param>
+		/// <param name="value">The value of the argument.  Must not be null.</param>
+		/// <remarks>
+		/// 	<para>Note that these values are NOT protected against eavesdropping or tampering in transit.  No
+		/// security-sensitive data should be stored using this method. </para>
+		/// 	<para>The value stored here can be retrieved using
+		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>.</para>
+		/// 	<para>Since the data set here is sent in the querystring of the request and some
+		/// servers place limits on the size of a request URL, this data should be kept relatively
+		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
+		/// </remarks>
+		void SetUntrustedCallbackArgument(string key, string value);
+
+		/// <summary>
 		/// Adds an OpenID extension to the request directed at the OpenID provider.
 		/// </summary>
 		/// <param name="extension">The initialized extension to add to the request.</param>
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
index f3539cb..7734b03 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
@@ -610,7 +610,7 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 					}
 
 					// Add state that needs to survive across the redirect.
-					this.Request.SetCallbackArgument(UsePersistentCookieCallbackKey, this.UsePersistentCookie.ToString(CultureInfo.InvariantCulture));
+					this.Request.SetUntrustedCallbackArgument(UsePersistentCookieCallbackKey, this.UsePersistentCookie.ToString(CultureInfo.InvariantCulture));
 				} else {
 					Logger.OpenId.WarnFormat("An invalid identifier was entered ({0}), but not caught by any validation routine.", this.Text);
 					this.Request = null;
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyAjaxControlBase.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyAjaxControlBase.cs
index 065c179..940d720 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyAjaxControlBase.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyAjaxControlBase.cs
@@ -448,20 +448,20 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			// Configure each generated request.
 			int reqIndex = 0;
 			foreach (var req in requests) {
-				req.SetCallbackArgument("index", (reqIndex++).ToString(CultureInfo.InvariantCulture));
+				req.SetUntrustedCallbackArgument("index", (reqIndex++).ToString(CultureInfo.InvariantCulture));
 
 				// If the ReturnToUrl was explicitly set, we'll need to reset our first parameter
 				if (string.IsNullOrEmpty(HttpUtility.ParseQueryString(req.ReturnToUrl.Query)[AuthenticationRequest.UserSuppliedIdentifierParameterName])) {
-					req.SetCallbackArgument(AuthenticationRequest.UserSuppliedIdentifierParameterName, this.Identifier.OriginalString);
+					req.SetUntrustedCallbackArgument(AuthenticationRequest.UserSuppliedIdentifierParameterName, this.Identifier.OriginalString);
 				}
 
 				// Our javascript needs to let the user know which endpoint responded.  So we force it here.
 				// This gives us the info even for 1.0 OPs and 2.0 setup_required responses.
-				req.SetCallbackArgument(OPEndpointParameterName, req.Provider.Uri.AbsoluteUri);
-				req.SetCallbackArgument(ClaimedIdParameterName, (string)req.ClaimedIdentifier ?? string.Empty);
+				req.SetUntrustedCallbackArgument(OPEndpointParameterName, req.Provider.Uri.AbsoluteUri);
+				req.SetUntrustedCallbackArgument(ClaimedIdParameterName, (string)req.ClaimedIdentifier ?? string.Empty);
 
 				// Inform ourselves in return_to that we're in a popup or iframe.
-				req.SetCallbackArgument(UIPopupCallbackKey, "1");
+				req.SetUntrustedCallbackArgument(UIPopupCallbackKey, "1");
 
 				// We append a # at the end so that if the OP happens to support it,
 				// the OpenID response "query string" is appended after the hash rather than before, resulting in the
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
index 829f61e..17b9e26 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
@@ -543,7 +543,7 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			foreach (var req in requests) {
 				if (this.IsPopupAppropriate(req)) {
 					// Inform ourselves in return_to that we're in a popup.
-					req.SetCallbackArgument(UIPopupCallbackKey, "1");
+					req.SetUntrustedCallbackArgument(UIPopupCallbackKey, "1");
 
 					if (req.Provider.IsExtensionSupported<UIRequest>()) {
 						// Inform the OP that we'll be using a popup window consistent with the UI extension.
@@ -553,14 +553,14 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 						// This is so the window can be made the correct size for the extension.
 						// If the OP doesn't advertise support for the extension, the javascript will use
 						// a bigger popup window.
-						req.SetCallbackArgument(PopupUISupportedJSHint, "1");
+						req.SetUntrustedCallbackArgument(PopupUISupportedJSHint, "1");
 					}
 				}
 
 				// Add state that needs to survive across the redirect.
 				if (!this.Stateless) {
-					req.SetCallbackArgument(UsePersistentCookieCallbackKey, this.UsePersistentCookie.ToString());
-					req.SetCallbackArgument(ReturnToReceivingControlId, this.ClientID);
+					req.SetUntrustedCallbackArgument(UsePersistentCookieCallbackKey, this.UsePersistentCookie.ToString());
+					req.SetUntrustedCallbackArgument(ReturnToReceivingControlId, this.ClientID);
 				}
 
 				((AuthenticationRequest)req).AssociationPreference = this.AssociationPreference;