search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.54k stars 4.54k forks source link

SignedXml does not work by default in trimmed applications #97274

Open vcsjones opened 5 months ago

vcsjones commented 5 months ago

If you use SignedXml within a trimmed application, the out-of-the-box experience is going to be a CryptographicException.

This is because SignedXml uses CryptoConfig to resolve hash algorithm identifiers (URIs) to an appropriate hash implementation. The trimmer cannot see through this, and even if it could, it could not handle scenarios where the algorithm is part of the document input. The trimmer ends up removing various pieces from System.Security.Cryptography that SignedXml actually needs. This includes types like RSAPKCS1SignatureFormatter and RSAPKCS1SignatureDeformatter, SHA1Managed, etc.

The error you get in this situation is very difficult to resolve.

System.Security.Cryptography.CryptographicException: Could not create hash algorithm object. If the application has been trimmed, ensure the required algorithm implementations are preserved.

What hash algorithm? It actually needs to be the *Managed ones because that's what CryptoConfig resolves for legacy purposes. If you just add S.S.C.SHA1 to your root list, it's not going to work. Without a detailed knowledge of how CryptoConfig and the runtime works, it's difficult to solve this problem on your own. How is someone supposed to know they need to add an obsolete class to their roots?

I am sure we can fix CryptoConfig to resolve things to better algorithms in some situations, or we can improve the error message. You fix one, then you move on to the next error about signature formatters. That presents as a null-ref.

One option is we add DynamicDependencyAttribute to SignedXml for common things that get end up getting resolved by CryptoConfig.

Something like:

partial class SignedXml {
    [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA1Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA256Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureFormatter", "System.Security.Cryptography.Algorithms")]
    [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureDeformatter", "System.Security.Cryptography.Algorithms")]
    // SHA-384 and SHA-512 too, I suppose.
    public SignedXml(XmlDocument doc);

    // Repeat for other constructors or methods
}

The knee-jerk reaction here might be to say "But this ends up rooting types that might not be used" which is correct. If you never use SHA-1, SHA-1 is rooted for no good reason. It's on the "small" side, we aren't rooting an actual implementation of SHA-1, just a public shim around an internal algorithm that dispatches by name.

This is a common issue with other cryptographic stacks. OIDs in various things like CMS have the same problem. We don't know the hash algorithm in a document until we try to process the document at runtime.

You could also say, "But if someone changes their CryptoConfig to use something else, we've rooted these things for nothing" which, yes, is also correct. But I believe this to be an uncommon scenario.

I also believe that if we were building S.S.C.Xml from scratch without CryptoConfig, it would look something like this:

HashAlgorithm alg = hasher switch {
    "http://www.w3.org/2000/09/xmldsig#sha1" => SHA1.Create(),
    "http://www.w3.org/2001/04/xmlenc#sha256" => SHA256.Create(),
    // etc
    _ => throw new CryptographicException(SR.IDontKnowWhatHashYouAreTalkingAbout),
};

Which leads us back to rooting hash algorithms.

An alternative thing to do is to handle null better when we get back from CryptoConfig, like GetSha1FromCryptoConfig() ?? SHA1.Create() but this is going to end up rooting common algorithms anyway through the factory method. Likewise for the signature formatters.

A final alternative option is to not fix this - but instead (significantly) improve the error along with a link to documentation explaining which things they need to manually add to their roots.

ghost commented 5 months ago

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones See info in area-owners.md if you want to be subscribed.

Issue Details
If you use `SignedXml` within a trimmed application, the out-of-the-box experience is going to be a `CryptographicException`. This is because `SignedXml` uses `CryptoConfig` to resolve hash algorithm identifiers (URIs) to an appropriate hash implementation. The trimmer cannot see through this, and even if it could, it could not handle scenarios where the algorithm is part of the document input. The trimmer ends up removing various pieces from `System.Security.Cryptography` that `SignedXml` actually needs. This includes types like `RSAPKCS1SignatureFormatter` and `RSAPKCS1SignatureDeformatter`, `SHA1Managed`, etc. The error you get in this situation is very difficult to resolve. > System.Security.Cryptography.CryptographicException: Could not create hash algorithm object. If the application has been trimmed, ensure the required algorithm implementations are preserved. What hash algorithm? It actually needs to be the `*Managed` ones because that's what CryptoConfig resolves for legacy purposes. If you just add `S.S.C.SHA1` to your root list, it's not going to work. Without a detailed knowledge of how CryptoConfig and the runtime works, it's difficult to solve this problem on your own. How is someone supposed to know they need to add an obsolete class to their roots? I am sure we can fix CryptoConfig to resolve things to better algorithms in some situations, or we can improve the error message. You fix one, then you move on to the next error about signature formatters. That presents as a null-ref. One option is we add `DynamicDependencyAttribute` to `SignedXml` for common things that get end up getting resolved by `CryptoConfig`. Something like: ```C# partial class SignedXml { [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA1Managed", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA256Managed", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography. RSAPKCS1SignatureFormatter", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography. RSAPKCS1SignatureDeformatter", "System.Security.Cryptography.Algorithms")] // SHA-384 and SHA-512 too, I suppose. public SignedXml(XmlDocument doc); // Repeat for other constructors or methods } ``` The knee-jerk reaction here might be to say "But this ends up rooting types that might not be used" which is correct. If you never use SHA-1, SHA-1 is rooted for no good reason. It's on the "small" side, we aren't rooting an actual implementation of SHA-1, just a public shim around an internal algorithm that dispatches by name. This is a common issue with other cryptographic stacks. OIDs in various things like CMS have the same problem. We don't know the hash algorithm in a document until we try to process the document at runtime. You could also say, "But if someone changes their CryptoConfig to use something else, we've rooted these things for nothing" which, yes, is also correct. But I believe this to be an uncommon scenario. I also believe that if we were building S.S.C.Xml from scratch without CryptoConfig, it would look something like this: ```C# HashAlgorithm alg = hasher switch { "http://www.w3.org/2000/09/xmldsig#sha1" => SHA1.Create(), "http://www.w3.org/2001/04/xmlenc#sha256" => SHA256.Create(), // etc _ => throw new CryptographicException(SR.IDontKnowWhatHashYouAreTalkingAbout), }; ``` Which leads us back to rooting hash algorithms. ----- An alternative thing to do is to handle `null` better when we get back from CryptoConfig, like `GetSha1FromCryptoConfig() ?? SHA1.Create()` but this is going to end up rooting common algorithms anyway through the factory method. Likewise for the signature formatters. A final alternative option is to not fix this - but instead (significantly) improve the error along with a link to documentation explaining which things they need to manually add to their roots.
Author: vcsjones
Assignees: -
Labels: `area-System.Security`
Milestone: Future
ghost commented 5 months ago

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar See info in area-owners.md if you want to be subscribed.

Issue Details
If you use `SignedXml` within a trimmed application, the out-of-the-box experience is going to be a `CryptographicException`. This is because `SignedXml` uses `CryptoConfig` to resolve hash algorithm identifiers (URIs) to an appropriate hash implementation. The trimmer cannot see through this, and even if it could, it could not handle scenarios where the algorithm is part of the document input. The trimmer ends up removing various pieces from `System.Security.Cryptography` that `SignedXml` actually needs. This includes types like `RSAPKCS1SignatureFormatter` and `RSAPKCS1SignatureDeformatter`, `SHA1Managed`, etc. The error you get in this situation is very difficult to resolve. > System.Security.Cryptography.CryptographicException: Could not create hash algorithm object. If the application has been trimmed, ensure the required algorithm implementations are preserved. What hash algorithm? It actually needs to be the `*Managed` ones because that's what CryptoConfig resolves for legacy purposes. If you just add `S.S.C.SHA1` to your root list, it's not going to work. Without a detailed knowledge of how CryptoConfig and the runtime works, it's difficult to solve this problem on your own. How is someone supposed to know they need to add an obsolete class to their roots? I am sure we can fix CryptoConfig to resolve things to better algorithms in some situations, or we can improve the error message. You fix one, then you move on to the next error about signature formatters. That presents as a null-ref. One option is we add `DynamicDependencyAttribute` to `SignedXml` for common things that get end up getting resolved by `CryptoConfig`. Something like: ```C# partial class SignedXml { [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA1Managed", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA256Managed", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureFormatter", "System.Security.Cryptography.Algorithms")] [DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureDeformatter", "System.Security.Cryptography.Algorithms")] // SHA-384 and SHA-512 too, I suppose. public SignedXml(XmlDocument doc); // Repeat for other constructors or methods } ``` The knee-jerk reaction here might be to say "But this ends up rooting types that might not be used" which is correct. If you never use SHA-1, SHA-1 is rooted for no good reason. It's on the "small" side, we aren't rooting an actual implementation of SHA-1, just a public shim around an internal algorithm that dispatches by name. This is a common issue with other cryptographic stacks. OIDs in various things like CMS have the same problem. We don't know the hash algorithm in a document until we try to process the document at runtime. You could also say, "But if someone changes their CryptoConfig to use something else, we've rooted these things for nothing" which, yes, is also correct. But I believe this to be an uncommon scenario. I also believe that if we were building S.S.C.Xml from scratch without CryptoConfig, it would look something like this: ```C# HashAlgorithm alg = hasher switch { "http://www.w3.org/2000/09/xmldsig#sha1" => SHA1.Create(), "http://www.w3.org/2001/04/xmlenc#sha256" => SHA256.Create(), // etc _ => throw new CryptographicException(SR.IDontKnowWhatHashYouAreTalkingAbout), }; ``` Which leads us back to rooting hash algorithms. ----- An alternative thing to do is to handle `null` better when we get back from CryptoConfig, like `GetSha1FromCryptoConfig() ?? SHA1.Create()` but this is going to end up rooting common algorithms anyway through the factory method. Likewise for the signature formatters. A final alternative option is to not fix this - but instead (significantly) improve the error along with a link to documentation explaining which things they need to manually add to their roots.
Author: vcsjones
Assignees: -
Labels: `area-System.Security`, `linkable-framework`
Milestone: Future
ceztko commented 3 months ago

I use something similar to this class inheriting SignedXML and I put this in my Native AOT compiled code:

    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA1Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA256Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA384Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.SHA512Managed", "System.Security.Cryptography.Algorithms")]
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureFormatter", "System.Security.Cryptography.Algorithms")]
    [DynamicDependency(DynamicallyAccessedMemberTypes.All, "System.Security.Cryptography.RSAPKCS1SignatureDeformatter", "System.Security.Cryptography.Algorithms")]
    private XadesSignedXml(XmlDocument document)
#pragma warning disable IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code
#pragma warning disable IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
        : base(document)
#pragma warning restore IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
#pragma warning restore IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code
    {
        // ...
    }

This helped me solve System.Security.Cryptography.CryptographicException and reduces the warnings during compilation. If something like DynamicDependencyAttribute could be set as an assembly level attribute, instead of setting it per class member, it would be more meaningful to me. If you have suggestions how this issue could be solved differently in a Native AOT application/library please tell me.

DanSmith commented 5 days ago

On net core 8, the classes were in a different assembly than in great suggestions from @ceztko and @vcsjones

DynamicDependency using this style with typeof should work wherever:

[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(System.Security.Cryptography.SHA1))]
...
...