DSAKeyValue in S.S.Cryptography.Xml should key for null on _key

[SteveDunn]

It has a public setter (Key) which doesn't check for null. In GetXml, it is used without checking for null, potentially resulting in an NRE.

Originally posted by @krwq in #67198 (comment)

[ghost]

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

Issue Details

---

It has a public setter (Key) which doesn't check for null. In GetXml, it is used without checking for null, potentially resulting in an NRE.

Originally posted by @krwq in #67198 (comment)

Author:	SteveDunn
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[bartonjs]

The DSAKeyValue(DSA) constructor also accepts null, so there's more than one path to that NullReferenceException; and RSAKeyValue has the same two sources of null.

All four can add in argument validation, but we shouldn't do DSA (fairly unused) without RSA (99%+ usage), and on either type we shouldn't do the ctor or property setter without the other.

[vcsjones]

@bartonjs we explicitly test that null in is null out.

runtime/src/libraries/System.Security.Cryptography.Xml/tests/RSAKeyValueTest.cs

Lines 37 to 42 in a94f6ca

	[Fact]
	public void Ctor_Rsa_Null()
	{
	RSAKeyValue rsaKeyValue = new RSAKeyValue(null);
	Assert.Null(rsaKeyValue.Key);
	}

I can't think of a good reason for this, and I am not sure if the tests exists because we want to make sure the test keeps working, or if the test was just thrown in there to test current behavior.

[bartonjs]

I assume it was just a standard "pass null, record the results" with the intent that changing the behavior is intentional.

In days of yore, new RSAKeyValue()'s RSA.Create() would create an RSACryptoServiceProvider, and the default ctor of RSACryptoServiceProvider would generate an RSA key immediately. So it was possibly an intentional design that you could do new RSAKeyValue(null) and then change the key later. (I forget when we changed RSACryptoServiceProvider to do delayed keygen... before my time, but after netfx20)

Not that we'd do xmldsig today (because... yeah), but if we did it today I don't think we'd make the key have a setter at all... and there'd just be a ctor for a current key instance and one for importing.

I'm also happy to just say "it is what it is" and close the issue. CryptoXml isn't really an area where we expect new development.

[vcsjones]

I'm also happy to just say "it is what it is" and close the issue. CryptoXml isn't really an area where we expect new development.

It's an easy enough change and it would make the nullability annotations that will (hopefully) get merged soon make more sense.

I looked through all public repos in GitHub and don't see a single instance of new RSAKeyValue(null) aside from our own tests and forks of dotnet/runtime.