SteveGilham / altcover

Cross-platform coverage gathering and processing tool set for dotnet/.Net Framework and Mono
MIT License
501 stars 18 forks source link

Changes so AltCover will run when FIPS compliance is required #214

Closed hfickes closed 7 months ago

hfickes commented 7 months ago

Short Version: Can Altcover use a FIPS compliant algorithm such as SHA384 instead of SHA-1?

Long Version Altcover throws an exception if a machine is configured to be FIPS compliant. The US government requires compliance with FIPS-140-3 for many systems. A description of this is at:

https://csrc.nist.gov/CSRC/media/Projects/cryptographic-module-validation-program/documents/fips%20140-3/FIPS%20140-3%20IG.pdf

and there are plenty of discussions regarding the usefulness of FIPS, though many refer to older versions of the specification.

Anyhow, we use AltCover on development VMs where we have FIPS compliance turned off but we need to run these tests on government machines where FIPS compliance is enabled.

You can enable/disable FIPS compliance on Windows via Group Policy or by setting the DWORD registry entry at:

HKLM\System\CurrentControlSet\Control\Lsa\FIPSAlgorithmPolicy\Enabled to 1 for enabled, and 0 for disabled

When FIPS compliance is on, running Altcover throws an exception while instrumenting the assemblies at:

Unhandled Exception: System.InvalidOperationException: This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms. at System.Security.Cryptography.SHA1Managed..ctor() at Mono.Cecil.CryptoService.HashStream(Stream stream, ImageWriter writer, Int32& strong_namepointer) at Mono.Cecil.CryptoService.StrongName(Stream stream, ImageWriter writer, WriterParameters parameters) at Mono.Cecil.ModuleWriter.Write(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleWriter.WriteModule(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleDefinition.Write(Stream stream, WriterParameters parameters) at AltCover.Instrument.I.writeAssembly(AssemblyDefinition assembly, String path) in ///AltCover.Engine/Instrument.fs:line 512 at AltCover.Instrument.I.writeAssemblies(AssemblyDefinition definition, String file, IEnumerable1 targets, FSharpFunc2 sink) in ///AltCover.Engine/Instrument.fs:line 870 at AltCover.Instrument.I.visitAfterAssembly(InstrumentContext state, AssemblyEntry assembly) in ///AltCover.Engine/Instrument.fs:line 1132 at AltCover.Instrument.I.instrumentationVisitorWrapper(FSharpFunc2 core, InstrumentContext state, Node node) in /_//AltCover.Engine/Instrument.fs:line 1468 at AltCover.Main.I.visitors@755.Invoke(InstrumentContext state, Node node) at AltCover.Visitor.stateful@1652-1.Invoke(T node) in /_//AltCover.Engine/Visitor.fs:line 1654 at Microsoft.FSharp.Primitives.Basics.List.mapToFreshConsTail[a,b](FSharpList1 cons, FSharpFunc2 f, FSharpList1 x) in D:\a_work\1\s\src\FSharp.Core\local.fs:line 236 at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc2 mapping, FSharpList1 x) in D:\a_work\1\s\src\FSharp.Core\local.fs:line 247 at Microsoft.FSharp.Collections.SeqModule.Fold[T,TState](FSharpFunc2 folder, TState state, IEnumerable1 source) in D:\a_work\1\s\src\FSharp.Core\seq.fs:line 913 at AltCover.Visitor.visit(IEnumerable1 visitors, IEnumerable1 assemblies) in ///AltCover.Engine/Visitor.fs:line 1639 at AltCover.Main.I.result@735.Invoke(Unit unitVar0) in ///AltCover.Engine/Main.fs:line 758 at AltCover.PathOperation.DoPathOperation[TAny](FSharpFunc2 operation, FSharpFunc2 handle) in ///AltCover.Engine/Output.fs:line 23 at AltCover.CommandLine.doPathOperation@433-1.Invoke(FSharpFunc`2 f, a defaultValue, Boolean store) at AltCover.Main.I.doInstrumentation(String[] arguments) in ///AltCover.Engine/Main.fs:line 730

I've looked at the code (first time I've read F# code outside of articles) and I can see where SHA-1 is being used. Could this be replaced with something that is compliant for signing such as SHA-384.

SHA-1 is compliant for some uses, but not for signing. From the document linked above:

"Whether a security function is approved may be context sensitive to the service in which it is included. For example, at Overall Security Rating 1, the SHA-1 function is an approved algorithm if it is used for an integrity check service, but it is not approved if it is used as part of a digital signature generation service. Therefore, it is not required to provide the indicator at the API level of cryptographic functions, as long as the service implementing the API provides the corresponding indicator that unambiguously indicates the approved security services"

Thank, Herb

SteveGilham commented 7 months ago

That's a simple one in principle - use of SHA1Managed rather than the compliant SHA1CryptoServiceProvider implementation. Unfortunately the exception is being thrown from the third party Mono.Cecil library while writing the instrumented assemblies, and not my code. I've opened issue jbevain/cecil#938 as a forwarding of this one, and submitted PR jbevain/cecil#939 with the fix.

SteveGilham commented 7 months ago

As to why SHA1 - the use that's causing the issue is that the strong name PublicKeyToken value is formed from the first 8 bytes of the SHA1 hash of the strong name public key. The hash is also used in the OpenCover format where a whole-file hash is taken of each assembly to distinguish different builds of the same name.

SteveGilham commented 7 months ago

Release 8.8.10 uses a patched version of Cecil that invokes the compliant SHA1 implementation, so hopefully will address the issue.

hfickes commented 7 months ago

Unfortunately, that didn't help. What I see in the stack trace is the following:

Unhandled Exception: System.InvalidOperationException: This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms.

at System.Security.Cryptography.SHA1Managed..ctor() at Mono.Cecil.CryptoService.HashStream(Stream stream, ImageWriter writer, Int32& strong_namepointer) at Mono.Cecil.CryptoService.StrongName(Stream stream, ImageWriter writer, WriterParameters parameters) at Mono.Cecil.ModuleWriter.Write(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleWriter.WriteModule(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleDefinition.Write(Stream stream, WriterParameters parameters) at AltCover.Instrument.I.writeAssembly(AssemblyDefinition assembly, String path) in ///AltCover.Engine/Instrument.fs:line 512 at AltCover.Instrument.I.writeAssemblies(AssemblyDefinition definition, String file, IEnumerable1 targets, FSharpFunc2 sink) in ///AltCover.Engine/Instrument.fs:line 870 at AltCover.Instrument.I.visitAfterAssembly(InstrumentContext state, AssemblyEntry assembly) in ///AltCover.Engine/Instrument.fs:line 1132 at AltCover.Instrument.I.instrumentationVisitorWrapper(FSharpFunc2 core, InstrumentContext state, Node node) in /_//AltCover.Engine/Instrument.fs:line 1468 at AltCover.Main.I.visitors@755.Invoke(InstrumentContext state, Node node) at AltCover.Visitor.stateful@1652-1.Invoke(T node) in /_//AltCover.Engine/Visitor.fs:line 1654 at Microsoft.FSharp.Primitives.Basics.List.mapToFreshConsTail[a,b](FSharpList1 cons, FSharpFunc2 f, FSharpList1 x) in D:\a_work\1\s\src\FSharp.Core\local.fs:line 236 at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc2 mapping, FSharpList1 x) in D:\a_work\1\s\src\FSharp.Core\local.fs:line 247 at Microsoft.FSharp.Collections.SeqModule.Fold[T,TState](FSharpFunc2 folder, TState state, IEnumerable1 source) in D:\a_work\1\s\src\FSharp.Core\seq.fs:line 913 at AltCover.Visitor.visit(IEnumerable1 visitors, IEnumerable1 assemblies) in ///AltCover.Engine/Visitor.fs:line 1639 at AltCover.Main.I.result@735.Invoke(Unit unitVar0) in ///AltCover.Engine/Main.fs:line 758 at AltCover.PathOperation.DoPathOperation[TAny](FSharpFunc2 operation, FSharpFunc2 handle) in ///AltCover.Engine/Output.fs:line 23 at AltCover.CommandLine.doPathOperation@433-1.Invoke(FSharpFunc`2 f, a defaultValue, Boolean store) at AltCover.Main.I.doInstrumentation(String[] arguments) in ///AltCover.Engine/Main.fs:line 730

SteveGilham commented 7 months ago

Are you certain that you are using the 8.8.x release?

I had wondered if maybe I had ended up getting the unpatched Cecil in the package by mistake, but having just downloaded 8.8.10 from nuget.org, decompiling the Mono.Cecil assembly from that reassures me that it is not invoking the non-compliant SHA1Managed implementation that is being reported.

Screenshot 2024-04-09 171751

hfickes commented 7 months ago

You are correct. I forgot to make a change in the path I was using so I still was running the 8.7.3 version of Altcover.

But when I changed to the 8.8.10 version, I got a completely new error. The following is from the log file.

Mono.Cecil.AssemblyResolutionException: Failed to resolve assembly: 'System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' at Mono.Cecil.BaseAssemblyResolver.Resolve(AssemblyNameReference name, ReaderParameters parameters) at Mono.Cecil.DefaultAssemblyResolver.Resolve(AssemblyNameReference name) at Mono.Cecil.MetadataResolver.Resolve(TypeReference type) at Mono.Cecil.TypeReference.Resolve() at Mono.Cecil.Mixin.CheckedResolve(TypeReference self) at Mono.Cecil.MetadataBuilder.GetConstantType(TypeReference constant_type, Object constant) at Mono.Cecil.MetadataBuilder.AddConstant(IConstantProvider owner, TypeReference type) at Mono.Cecil.MetadataBuilder.AddField(FieldDefinition field) at Mono.Cecil.MetadataBuilder.AddFields(TypeDefinition type) at Mono.Cecil.MetadataBuilder.AddType(TypeDefinition type) at Mono.Cecil.MetadataBuilder.AddTypes() at Mono.Cecil.MetadataBuilder.BuildTypes() at Mono.Cecil.MetadataBuilder.BuildModule() at Mono.Cecil.MetadataBuilder.BuildMetadata() at Mono.Cecil.ModuleWriter.<>c.b__20(MetadataBuilder builder, MetadataReader ) at Mono.Cecil.ModuleDefinition.Read[TItem,TRet](TItem item, Func3 read) at Mono.Cecil.ModuleWriter.Write(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleWriter.WriteModule(ModuleDefinition module, Disposable1 stream, WriterParameters parameters) at Mono.Cecil.ModuleDefinition.Write(Stream stream, WriterParameters parameters) at AltCover.Instrument.I.writeAssembly(AssemblyDefinition assembly, String path) in /_//AltCover.Engine/Instrument.fs:line 512 at AltCover.Instrument.I.writeAssemblies(AssemblyDefinition definition, String file, IEnumerable1 targets, FSharpFunc2 sink) in /_//AltCover.Engine/Instrument.fs:line 870 at AltCover.Instrument.I.visitAfterAssembly(InstrumentContext state, AssemblyEntry assembly) in /_//AltCover.Engine/Instrument.fs:line 1132 at AltCover.Instrument.I.instrumentationVisitorWrapper(FSharpFunc2 core, InstrumentContext state, Node node) in ///AltCover.Engine/Instrument.fs:line 1468 at AltCover.Main.I.visitors@776.Invoke(InstrumentContext state, Node node) at AltCover.Visitor.stateful@1659-1.Invoke(T node) in ///AltCover.Engine/Visitor.fs:line 1661 at Microsoft.FSharp.Primitives.Basics.List.mapToFreshConsTail[a,b](FSharpList1 cons, FSharpFunc2 f, FSharpList1 x) in D:\a\_work\1\s\src\FSharp.Core\local.fs:line 236 at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc2 mapping, FSharpList1 x) in D:\a\_work\1\s\src\FSharp.Core\local.fs:line 247 at Microsoft.FSharp.Collections.SeqModule.Fold[T,TState](FSharpFunc2 folder, TState state, IEnumerable1 source) in D:\a\_work\1\s\src\FSharp.Core\seq.fs:line 913 at AltCover.Visitor.visit(IEnumerable1 visitors, IEnumerable1 assemblies) in /_//AltCover.Engine/Visitor.fs:line 1646 at AltCover.Main.I.result@756.Invoke(Unit unitVar0) in /_//AltCover.Engine/Main.fs:line 779 at AltCover.PathOperation.DoPathOperation[TAny](FSharpFunc2 operation, FSharpFunc`2 handle) in /_//AltCover.Engine/Output.fs:line 21 AssemblyReference = System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 FileName =

FusionLog = Data = seq [] InnerException = TargetSite = Mono.Cecil.AssemblyDefinition Resolve(Mono.Cecil.AssemblyNameReference, Mono.Cecil.ReaderParameters) HelpLink = Source = "Mono.Cecil" HResult = -2147024894
SteveGilham commented 7 months ago

When writing an assembly, Cecil needs to perform assembly linkage against the dependencies. One or more of your assemblies is using types from System.Windows.Forms, so it's asking you to tell it where that assembly is, so it can perform the linkage, since it's not in the same folder as is being instrumented, nor is it in the nuget cache.

This is done with the --dependency=[full path of assembly] option (or equivalents).

This wouldn't happen to be a .Net Framework application expecting to find files in the GAC, would it now?

hfickes commented 7 months ago

Yes, it is a .Net Framework 4.8 application using the GAC. However, this worked before with 8.7.3. The only change was to use the newer altcover version 8.8.10. Switching back, it worked again.

I suspect this has to do with changes other than the FIPS one you made, so I can open a different issue if you'd prefer.

SteveGilham commented 7 months ago

That's odd, as the changes since 8.7.3 have just a) added an entry to each of 2 enums and b) allowed the report file name to be a relative path and c) patched the SHA1 implementation used by Cecil.

However, it's simple enough to add the GAC to the list of places to look for dependencies, if it's not being picked up automagically.

SteveGilham commented 7 months ago

It occurs to me to ask if this error failing to find the System.Windows.Forms assembly is being observed on one of the machines where the FIPS compliance failure was happening - in particular, if the difference in environment is between a development machine and a user machine without developer tools, more than simply a switch of a registry setting.

hfickes commented 7 months ago

It happens both on the machine with FIPS compliance enabled, and on our test machine where we keep it turned off so that we can run Altcover 8.7 and earlier.

This happens on the same machine we've been using all along. All that I change to make it happen is to switch from 8.7.3 to 8.8.10.

SteveGilham commented 7 months ago

Release 8.8.21 explicitly adds the GAC to the locations searched for dependencies; which I trust will resolve the newer problem.

hfickes commented 7 months ago

That works great for us! Thank you very much.