Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

How do I modify the defaults for Antiforgery to make it FIPS-compliant #95

Closed
thanhos opened this issue Jul 28, 2016 · 36 comments
Closed

Comments

@thanhos
Copy link

thanhos commented Jul 28, 2016

I'm working on a system that has FIPS-compliant enabled and am wondering is there a way to change the Antiforgery defaults to make it compliant. If someone could provide some direction or feedback that would appreciated.

Thanks

@thanhos
Copy link
Author

thanhos commented Jul 28, 2016

Creating a new ASP.NET Core Web Application using .NET Core works fine. Creating one using the .NET Framework with FIPS enabled doesn't work.

@rynowak
Copy link
Member

rynowak commented Jul 29, 2016

/cc @blowdart

@blowdart
Copy link
Member

Strange.

What OS, and do you have a stack trace?

@rynowak This may end up being deep in data protection. There's a fun bug in creating SHA signing pieces where you tell it to use FIPS algorithms and it doesn't. nuget had the same problem,

@thanhos
Copy link
Author

thanhos commented Aug 1, 2016

I'm on Windows 10.0 (Build 10240).

Here's the stack trace:

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[0]
      An unhandled exception has occurred while executing the request
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidOperationException: This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms.
   at System.Security.Cryptography.SHA256Managed..ctor()
   --- End of inner exception stack trace ---
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Security.Cryptography.CryptoConfig.CreateFromName(String name, Object[] args)
   at System.Security.Cryptography.SHA256.Create()
   at Microsoft.AspNetCore.Antiforgery.Internal.AntiforgeryOptionsSetup.ComputeCookieName(String applicationId)
   at Microsoft.AspNetCore.Antiforgery.Internal.AntiforgeryOptionsSetup.ConfigureOptions(AntiforgeryOptions options, DataProtectionOptions dataProtectionOptions)
   at Microsoft.Extensions.Options.OptionsCache`1.CreateOptions()
   at System.Threading.LazyInitializer.EnsureInitializedCore[T](T& target, Boolean& initialized, Object& syncLock, Func`1 valueFactory)
   at Microsoft.Extensions.Options.OptionsCache`1.get_Value()
   at Microsoft.AspNetCore.Antiforgery.Internal.DefaultAntiforgeryTokenStore..ctor(IOptions`1 optionsAccessor)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ConstructorCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.ScopedCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ConstructorCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.ScopedCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ConstructorCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.ScopedCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ConstructorCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.TransientCallSite.Invoke(ServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.AspNetCore.Mvc.Razor.RazorPageActivator.<>c__DisplayClass16_0.<CreateActivateInfo>b__1(ViewContext context)
   at Microsoft.Extensions.Internal.PropertyActivator`1.Activate(Object instance, TContext context)
   at Microsoft.AspNetCore.Mvc.Razor.RazorPageActivator.Activate(IRazorPage page, ViewContext context)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.<RenderViewStartsAsync>d__16.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.<RenderPageAsync>d__14.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.<RenderAsync>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.<ExecuteAsync>d__18.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.ViewResult.<ExecuteResultAsync>d__26.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeResultAsync>d__32.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeResultFilterAsync>d__31.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeAllResultFiltersAsync>d__29.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeResourceFilterAsync>d__23.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeAsync>d__18.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.<Invoke>d__6.MoveNext()
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 3966.2482ms 500 text/html; charset=utf-8

@rynowak
Copy link
Member

rynowak commented Aug 1, 2016

Thanks for providing this.

This callstack is using Sha256Managed to compute a unique application cookie name. We'll have to do something to address this. https://github.com/aspnet/Antiforgery/blob/dev/src/Microsoft.AspNetCore.Antiforgery/Internal/AntiforgeryOptionsSetup.cs

Can you try the following workaround:

services.Insert(0, ServiceDescriptor.Singleton(
    typeof(IConfigureOptions<AntiforgeryOptions>), 
    new ConfigureOptions<AntiforgeryOptions>(options => options.CookieName = "<choose a name>")));

The purpose of the random cookie name is just to avoid colliding with other applications using our antiforgery system when using shared hosting. If you have your own domain name then there's no risk of collision.

@rynowak
Copy link
Member

rynowak commented Aug 1, 2016

/cc @blowdart - see above, it looks like we have to do something about this, but it's local to antiforgery

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

Oh dear. This is a bug in the desktop framework. SHA256.Create should create a FIPS compliant CSP, but doesn't in some circumstances. nuget had the same problem (NuGet/Home#2335)

Switch from SHA256.Create() to (HashAlgorithm)new SHA256CryptoServiceProvider()

@Eilon Eilon added this to the 1.0.1 milestone Aug 1, 2016
@Eilon
Copy link
Member

Eilon commented Aug 1, 2016

Prospectively putting this in the 1.0.1 milestone, but will need to get approval for it.

@thanhos
Copy link
Author

thanhos commented Aug 1, 2016

I tried the workaround above and it seems like it's no longer failing in Antiforgery but in the TagHelpers. Looks like a general bug. Where should I redirect this issue to? Here's the stack trace.

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[0]
      An unhandled exception has occurred while executing the request
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidOperationException: This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms.
   at System.Security.Cryptography.SHA256Managed..ctor()
   --- End of inner exception stack trace ---
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Security.Cryptography.CryptoConfig.CreateFromName(String name, Object[] args)
   at System.Security.Cryptography.SHA256.Create()
   at Microsoft.AspNetCore.Mvc.TagHelpers.Internal.FileVersionProvider.GetHashForFile(IFileInfo fileInfo)
   at Microsoft.AspNetCore.Mvc.TagHelpers.Internal.FileVersionProvider.AddFileVersionToPath(String path)
   at Microsoft.AspNetCore.Mvc.TagHelpers.ScriptTagHelper.Process(TagHelperContext context, TagHelperOutput output)
   at Microsoft.AspNetCore.Razor.TagHelpers.TagHelper.ProcessAsync(TagHelperContext context, TagHelperOutput output)
   at Microsoft.AspNetCore.Razor.Runtime.TagHelpers.TagHelperRunner.<RunAsync>d__0.MoveNext()

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

@NTaylorMullen ...

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

@thanhos I have a work around for you.

Firstly my apologies. This is a bug in the .NET framework, and it's fixed in 4.6.2 but I presume because you have FIPS enabled that upgrading when that drops is probably painful. I'm currently creating a new FIPS VM to check this.

What you can do is in program.cs, add the following line at the beginning;

CryptoConfig.AddAlgorithm(typeof(SHA256Cng), "SHA256")

You'll need to add the following reference

using System.Security.Cryptography;

If you're targeting .NET Core the error won't occur, we got rid of all the managed crypto packages, so everything is left to the underlying platform, thus no FIPS concerns. We'll also get the code adjusted as well.

@thanhos
Copy link
Author

thanhos commented Aug 1, 2016

Thanks for the feedback so far @blowdart

I tried the suggestion above and still received the same error.

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

Darn I was so hopefully. It's probably too late in the initialization process then (I'm still attempting to get a FIPS machine up and running)

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

I know it's a lot to ask, but could you try installing 4.6.2 beta on a machine and see if that works? It's likely 4.6.2 will drop before we officially release a patch for this.

@thanhos
Copy link
Author

thanhos commented Aug 1, 2016

Sure, I'll give that a try and see how it goes.

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

Thanks. Apologies for forcing a beta on you.

@thanhos
Copy link
Author

thanhos commented Aug 1, 2016

No worries. I installed the 4.6.2 Preview and still getting the same exceptions.

@blowdart
Copy link
Member

blowdart commented Aug 1, 2016

Darn. Emailing some folks.

@kevinchalet
Copy link

FWIW, an ASOS user experienced the same thing and this snippet worked:

// Make sure to replace both "SHA256" and "System.Security.Cryptography.SHA256"
CryptoConfig.AddAlgorithm(typeof(SHA256Cng), typeof(SHA256).Name, typeof(SHA256).FullName);

I presume @blowdart's snippet doesn't work because it doesn't override "System.Security.Cryptography.SHA256".

@bartonjs
Copy link

bartonjs commented Aug 1, 2016

grumble grumble. That's my bad, I advised him without looking at the implementation on referencesource. Aes.Create() queries for "AES", so for some reason I assumed SHA256 queried for "SHA256". How absurd of me 😄.

Installing 4.6.2 preview should've worked. At least, it's in the public changelog (with a terrible description):

The factory method now returns SHA256Cng in FIPS-only mode. [163804]

Guess that's something for me to dig into.

@kevinchalet
Copy link

kevinchalet commented Aug 1, 2016

The factory method now returns SHA256Cng in FIPS-only mode. [163804]

Hahaha, guys who use SHA256.Create() and abusively cast the returned instance to SHA256Managed will have a nice surprise 😄

@bartonjs
Copy link

bartonjs commented Aug 2, 2016

@PinpointTownes Nah, you missed "in FIPS-only mode". Since the ctor of SHA256Managed is what throws SHA256.Create() never exited in that case; so we've changed an exceptional case to a passing case. Non-FIPS mode is left as-was, for precisely that bad pattern.

@kevinchalet
Copy link

@bartonjs ah yeah good point 😄

On a related note, is there a plan to change the default key size used by RSACryptoServiceProvider in the future?

@bartonjs
Copy link

bartonjs commented Aug 2, 2016

@PinpointTownes That's a hard argument with compat (but one I'll have at some point). But hopefully https://github.com/dotnet/corefx/issues/8688 will take the sting off.

@thanhos
Copy link
Author

thanhos commented Aug 2, 2016

@PinpointTownes 's suggestion worked. Thanks!

I'll have to investigate as to why installing 4.6.2 didn't work on my machine.

Thanks for the help everyone.

@dougbu
Copy link
Member

dougbu commented Aug 2, 2016

FYI Antiforgery and MVC use SHA256.Create() a couple more times than were hit above:

  • AntiforgerySerializationContext in Antiforgery
  • CacheTagKey in MVC

@Eilon
Copy link
Member

Eilon commented Aug 2, 2016

FYI, I logged aspnet/Mvc#5103 for the MVC issues.

@kevinchalet
Copy link

@Eilon is this really a MVC issue? It already does the right thing by calling SHA256.Create() instead of trying to use a specific implementation like SHA256Managed.

The fact you don't get a FIPS-compliant instance on .NET Desktop < 4.6.2 is another story, but I'm not sure there's anything you can do in MVC.

@Eilon
Copy link
Member

Eilon commented Aug 2, 2016

@PinpointTownes to be honest, not sure - but MVC's code looks the same as anti-forgery's, so wouldn't it be the same issue?

@kevinchalet
Copy link

Well, IMHO, there's no bug in Antiforgery or MVC, as they both correctly use SHA256.Create().

The "bug" (by design?) was more caused by CryptoConfig, that used to blindly register SHA256Managed as the default implementation without checking whether FIPS was enabled first in < .NET 4.6.2: http://referencesource.microsoft.com/#mscorlib/system/security/cryptography/cryptoconfig.cs,261

I guess the right thing to do is to document that somewhere. People who need FIPS support on .NET Desktop should either migrate to .NET 4.6.2 or override the default SHA-256 implementation to use CAPI (SHA256CryptoServiceProvider) or CNG (SHA256Cng), that both support FIPS: #95 (comment)

@Eilon
Copy link
Member

Eilon commented Aug 2, 2016

@PinpointTownes yeah I think it boils down to: the system as a whole doesn't work on FIPS-enabled machines, so we want to do whatever we can reasonably do to make it work, regardless of where the actual root bug is. So if we need to just work around bugs in our own codebase (while that codebase is getting the "real" fix), then we'll try to do that. Does that make sense?

@kevinchalet
Copy link

kevinchalet commented Aug 2, 2016

Does that make sense?

It does. I'm just concerned by the the fact your workaround might break cross-platform scenarios:

  • SHA256Cng is based on Windows CNG and only works there (I'm not even sure the type exists on Mono).
  • SHA256CryptoServiceProvider relies on CryptoAPI, which is also a Windows-only API. It should work on Mono as the type is there (but it uses a managed implementation internally) but will break everywhere else.

I guess one viable option might be to call CryptoConfig.AllowOnlyFipsAlgorithms to determine whether FIPS was enforced and replace the instance returned by SHA256.Create() by a SHA256Cng instance on .NET Desktop, but you'll break Mono harder 😄

Is it really worth it? IMHO, the "right" fix is something people should add in their entry point depending on their own needs:

// Make sure to replace both "SHA256" and "System.Security.Cryptography.SHA256"
CryptoConfig.AddAlgorithm(typeof(SHA256Cng), typeof(SHA256).Name, typeof(SHA256).FullName);

@bartonjs
Copy link

bartonjs commented Aug 2, 2016

@thanhos 4.6.2 (final) released today: https://blogs.msdn.microsoft.com/dotnet/2016/08/02/announcing-net-framework-4-6-2/. I very much hope/expect that the CryptoConfig issue would be resolved for you with that build installed. The CryptoConfig change is only conditional on the FIPS setting, so it doesn't require changing your calling executable to be explicitly compiled against 4.6.2. (Though, like many things with the FIPS setting, it reads it once at startup and never again, in case your scenario flipped the bit after app start)

@blowdart
Copy link
Member

blowdart commented Aug 2, 2016

@PinpointTownes Having people jump through hoops isn't the right answer. The right answer is making it work on our supported platforms.

@NTaylorMullen
Copy link
Member

PR is ready to go in. Waiting for patch-approved. Once in, will need to work out changes to templates to ensure they reference 1.0.1 Antiforgery.

@NTaylorMullen
Copy link
Member

cdf84eb

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants