Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Added additional OIDs for CMS signatures.#32666

Merged
bartonjs merged 1 commit intodotnet:masterfrom
vcsjones:fix-32639
Oct 8, 2018
Merged

Added additional OIDs for CMS signatures.#32666
bartonjs merged 1 commit intodotnet:masterfrom
vcsjones:fix-32639

Conversation

@vcsjones
Copy link
Member

@vcsjones vcsjones commented Oct 7, 2018

This adds additional OIDs for RSA CMS validation when the OID is specific about which hash algorithm should be used.

This is a slightly different approach from the DSA / ECDsa approach. This allows for a null HashAlgorithmName and does not enforce the hash algorithm if one is not specified when using the "1.2.840.113549.1.1.1" OID.

Fixes #32639

@vcsjones
Copy link
Member Author

vcsjones commented Oct 7, 2018

/cc @bartonjs @onovotny

@vcsjones
Copy link
Member Author

vcsjones commented Oct 7, 2018

Hm. Now that I think about it, we can do 1.2.840.113549.1.1.4 for MD5 support. @bartonjs, do we want an MD5WithRsa OID here?

"50EBAAC4460DAA35185C16670F597E0E6E0CB0AA83F51AAEF452F3367DD9350A" +
"8A49A5A8F79DF8E921303AB5D6646A482F0F59D9980310E1AE3EE8D77CB857").HexToByteArray();

internal static readonly byte[] RsaPkcs1signedSha1DeclaredSha256WithRsa = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the S in signed should be uppercase :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. 🐪

@bartonjs
Copy link
Member

bartonjs commented Oct 8, 2018

@bartonjs, do we want an MD5WithRsa OID here?

Eh, I'd leave it out unless there's a reason to bother later.

@vcsjones
Copy link
Member Author

vcsjones commented Oct 8, 2018

Eh, I'd leave it out unless there's a reason to bother later.

Makes sense. I have no compelling reason to put it there other than completeness, and I'm not particularly fond of MD5-anything.

Fixed the casing issue.

@bartonjs
Copy link
Member

bartonjs commented Oct 8, 2018

@dotnet-bot Test this please (lots of failures due to what looks like a Jenkins restart)

@bartonjs bartonjs merged commit 6e80c3d into dotnet:master Oct 8, 2018
@vcsjones vcsjones deleted the fix-32639 branch October 8, 2018 17:18
@bartonjs
Copy link
Member

bartonjs commented Oct 8, 2018

@vcsjones The (valid) test document you added doesn't have indefinite length content, right? (I don't see an 0x24, so it shouldn't be; but making sure I'm not being redundant)

@vcsjones
Copy link
Member Author

vcsjones commented Oct 8, 2018

@bartonjs no they do not, I didn't want to risk cramming two issues into the same CMS.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants