Support additional OIDs for SignedCms digest algorithms #38845
Conversation
cannot be used for creation.
|
@InternetLogic I tested these changes with your supplied CMS (I did not commit it to the repository). This change fixes this particular issue you were hitting. However, I believe you are still going to have issues with your CMS. Fixing this issue only uncovered the next one, which is that
|
src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/PkcsHelpers.cs
Outdated
Show resolved
Hide resolved
src/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfo.cs
Outdated
Show resolved
Hide resolved
| DigestAlgorithm.Value, | ||
| out string[] expectedSignatureAlgorithmOids, | ||
| forVerification: true); | ||
| if (expectedSignatureAlgorithmOids != null && |
There was a problem hiding this comment.
This could probably use a comment. Something like
// Allow a signature algorithm identifier to be used as a digest algorithm identifier,
// but only when it matches the actual signature algorithm
| signer.CheckSignature(true); | ||
| } | ||
|
|
||
|
|
Use static arrays so they are not created every call. Fix indentation. Add code comment.
|
Hm. The Windows x64 leg had three unrelated tests fail for the same reason. An out-of-process test had an exit code of C0000374 which, I think, usually means heap corruption. |
|
@bartonjs moved some tests to netcoreapp specific locations since two of them do not apply to NetFx. Bizarre that Desktop allows an ECDSA signed CMS to use SHA256withRSA digest algorithm identifiers. This means we are still more strict than Desktop, but mixing and matching RSA and ECDSA OIDs doesn't seem like like something we'd want to do. |
|
@vcsjones Please don't force push, it makes it hard to tell what has changed (and therefore what to review). The Squash and Merge button pretty much replaces any need for force-pushing PRs. It looks like those two tests "passed" on NetFX, i.e. they allowed the use of id-sha1WithRsaEncryption as the digest identifier with ECDSA... so maybe we should let CoreFX validate those. Unless I missed something about the result it was showing. |
Ah, sorry. I wanted to beat you to a "Nit: whitespace". One of the tests was something failing I expected but forgot to account for with test placement. NetFx allows creating a
The other test failed for the reason you stated. Either seems fine to me. In a previous issue when something similar came up for the SignatureAlgorithm at https://github.com/dotnet/corefx/issues/32639#issuecomment-427471531. The approach seems to be such that if the OID is somehow more restrictive, then we enforce that. Perhaps I am overthinking this with digest algorithms. |
|
Well, really, it's if we're taking a change to be compatible with NetFX we shouldn't be half-compatible 😄. I don't think it's super sensible to say that a document is signed with ECDSA / sha1WithRsaEncryption (when it meant ECDSA / sha1), but if Windows does the "well.... I can figure out what you meant...." then maybe we can also do the simpler code (if we don't bother doing the sensibility matching we lose the new arrays and out parameter entirely). I'll also accept "the parts that we're allowing through now are based on documents in the wild, these negative tests aren't things we've seen in the wild, and they're here to demarcate where we're drawing the line". I'm sort of exactly torn. If not exact, I may have just convinced myself by writing it down that I'm happier with your checks and demarcation... but if my words made you flip positions I'll accept that 😄. |
|
@vcsjones @bartonjs Hey guys thanks so much for your efforts on this. You've saved me a lot of work porting our product to CoreFx. I uncovered this from a previous developer's misconception and so I've also amended our product's code to silently enforce the correct hash algorithm for these Oids so there should be less of these signatures in the wild in future! Thanks again :-D |
|
@bartonjs OK, I slept on it and am swayed by code simplicity and matching netfx behavior. There isn't any inherent risk in allowing it that I am aware of, other than it being strange, and strange things do indeed happen with regard to CMS. |
|
The Windows x64 Debug leg failures look like #38858. |
|
I'm going to use discretion and say that this didn't cause STATUS_HEAP_CORRUPTION in System.Reflection.Tests on Nano. (Another hit of #38858) |
If an existing CMS has a digest algorithm identifier of {Digest}withRSA, the algorithm will be treated as unknown, however this works in the Desktop .NET Framework. This adds support for those algorithm identifiers for signature verification, but not for creation.
Fixes #38814.