Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Security.Cryptography.Xml.Utils.GetAnyPublicKey returns only RSA or null? #55194

Closed
alexaka1 opened this issue Jul 6, 2021 · 5 comments · Fixed by #55294
Closed

System.Security.Cryptography.Xml.Utils.GetAnyPublicKey returns only RSA or null? #55194

alexaka1 opened this issue Jul 6, 2021 · 5 comments · Fixed by #55294
Assignees
Labels
area-System.Security bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@alexaka1
Copy link

alexaka1 commented Jul 6, 2021

This is the line in question:

return (AsymmetricAlgorithm)certificate.GetRSAPublicKey();

But what about DSA or ECDsa keys? Shouldn't it check for them when GetRSAPublicKey() returns null?
How about return (AsymmetricAlgorithm) (certificate.GetRSAPublicKey() ?? certificate.GetDSAPublicKey() ?? certificate.GetECDsaPublicKey()); ?

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jul 6, 2021
@ghost
Copy link

ghost commented Jul 6, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This is the line in question:

return (AsymmetricAlgorithm)certificate.GetRSAPublicKey();

But what about DSA or ECDsa keys? Shouldn't it check for them when GetRSAPublicKey() returns null?
How about return (AsymmetricAlgorithm) (certificate.GetRSAPublicKey() ?? certificate.GetDSAPublicKey() ?? certificate.GetECDsaPublicKey()); ?

Author: alexaka1
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2021

Do you have a specifically impacted scenario, or is this just a code inspection question?

My recollection is that SignedXml doesn't support ECDSA, so having GetECDsaPublicKey in there doesn't make sense. And GetDSAPublicKey isn't part of .NET Standard 2.0, so we can't call it without splitting the library. Given that there's not a lot usage of DSA we thought it wasn't worth splitting for it.

@alexaka1
Copy link
Author

alexaka1 commented Jul 7, 2021

@bartonjs I am trying to validate XAdES signatures in XML files. I already use SignedXML for signing such documents with ECDSA keys. The SigningKey property has no trouble accepting an ECDsa object, since it's just an AsymmetricAlgorithm. The only thing I had to do is register a System.Security.Cryptography.SignatureDescription.
Also I just need to put eg.: http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512 into the SignedInfo.SignatureMethod and it works like a charm.

The problem comes when I try to check the signature of an XML signed with something else than an RSA key. I make a new SignedXml, load the signature, and it fails (CheckSignature() returns false) because the key returned is null. (I debugged it with Rider and the above referenced code was where it starts to fail, because it returns null.) My use case doesn't need DSA keys, but my country will start to require ECC based keys for digital signatures starting with Q3 2021, and we're going ahead implementing it sooner rather than later.

What would you suggest I try to do? I could just rip the entire source code of the classes required to reach that particular line, and re-implement them with the needed changes in our app, but I'd rather avoid re-inventing the wheel.

ps.: To be honest I didn't research what platforms support what functions of the Cryptography library, my use case is Windows x64, though I believe we compile into portable runtime.

@bartonjs bartonjs added bug tenet-compatibility Incompatibility with previous versions or .NET Framework and removed untriaged New issue has not been triaged by the area owner labels Jul 7, 2021
@bartonjs
Copy link
Member

bartonjs commented Jul 7, 2021

It looks like:

  • NET Framework's version does try all three
  • GetECDsaPublicKey is part of .NET Standard 2.0, so no problems adding that one
  • We recently split the compilation of this assembly as part of unifying how we build our NuGet packages, so we can add the DSA call in the .NET (nee Core) specific line.

So adding those calls is a reasonable compat thing to do.

@bartonjs bartonjs added this to the 6.0.0 milestone Jul 7, 2021
@vcsjones vcsjones self-assigned this Jul 7, 2021
@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2021

@bartonjs I can pick this one up for 6.0.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants