Allow EnvelopedCms to round-trip correctly cross platform when ContentInfo is not Pkcs7Data by krwq · Pull Request #29926 · dotnet/corefx

krwq · 2018-05-25T19:54:32Z

Fixes https://github.com/dotnet/corefx/issues/29825

This might need to wait until @bartonjs is back from vacation.

We should consider this for 2.1.1 because documents encoded on non-Windows platforms with EnvelopedCms which have ContentInfo Oid different than 1.2.840.113549.1.7.1 will not roundtrip correctly cross platform. Any currently existing document encoded in such way will have additional DER "Sequence" prefix (0x30, <length>) after decryption

…tInfo type is not 1.2.840.113549.1.7.1 (Pkcs7Data)

bartonjs · 2018-05-26T04:43:17Z

src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/AnyOS/ManagedPal.Decrypt.cs

+                    using (AsnWriter writer = new AsnWriter(AsnEncodingRules.DER))
+                    {
+                        writer.PushSequence();
+                        AsnReader reader = new AsnReader(decrypted, AsnEncodingRules.DER);


Most of CMS says that it's BER, meaning indefinite length encodings are permitted. This may be overly restrictive. (Same with the writer, or it will validate the call to WriteEncodedValue)

@bartonjs I did it this way because Windows appends DER prefix and I wasn't sure how to enforce length definite Sequence tag. I was thinking about octet string instead of this and replacing the first byte to 0x30 to avoid validation - I haven't checked what BER produces by default though so I presume you're right about being to restrictive (I also thought this is always DER).

bartonjs · 2018-05-26T04:44:15Z

src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/AnyOS/ManagedPal.Encrypt.cs

+                {
+                    using (MemoryStream memoryStream = new MemoryStream())
+                    {
+                        using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))


There's no need to drag CryptoStream and System.IO into this, because you only have one call to Write. Just build the data and call encryptor.OneShot.

I did it this way to avoid building intermediate buffers - this is a modified copy of OneShot method which tries to not unnecessarily allocate. I thought the purpose of CryptoStream was to feed it with data I want to encrypt/decrypt which I'm doing here.

To build data I'll also need some equivalent of MemoryStream so not sure what the ask is. Unless you mean something else?

@bartonjs - missed the part about "one call to Write" - currently it's reading the sequence so there is more than one call (in most of the cases there will be 1 element but technically there could be more)

CryptoStream and MemoryStream both will create a fair amount of intermediate buffers. But since the call to Write is in a loop I can believe that this code is easier to understand.

Actually, it looks like this is just trying to do reader.PeekContentBytes(). And I think Windows might not care that the outer tag is SEQUENCE.

Looks like this passes on Windows:

ContentInfo content = new ContentInfo( new Oid(Oids.Pkcs7Enveloped), "0403010203".HexToByteArray()); EnvelopedCms cms = new EnvelopedCms(content); using (X509Certificate2 cert = Certificates.RSAKeyTransferCapi1.TryGetCertificateWithPrivateKey()) { cms.Encrypt(new CmsRecipient(cert)); byte[] temp = cms.Encode(); // Double checking. cms = new EnvelopedCms(); cms.Decode(temp); cms.Decrypt(new X509Certificate2Collection(cert)); } Assert.Equal("3003010203", cms.ContentInfo.Content.ByteArrayToHex());

I don't know that I agree with it passing, but since we're making a compat fix, we should be compatible.

bartonjs

I think this will fail when told that 04 03 01 02 03 is nested enveloped content. Please add a compatibility test for that.

048103010203 also seems like a good test case (=> 3003010203, probably).

Windows doesn't seem to be happy to create a wrapped thing with the outer value having an indefinite encoding, but I don't know that we need to fail as aggressively as Windows does.

One final test case to consider is enveloping something using an OID that isn't from the PKCS7 set. Like, oh, "0.0". Just to make sure that Windows is applying "not PKCS7-data" vs "is PKCS7-{enveloped/signed/etc}"

danmoseley · 2018-06-04T18:48:32Z

@krwq if you believe this shoudl be considered for servicing please tag the issue (not PR) with servicing-consider-2.1.x. (It missed 2.1.1 I think)

krwq · 2018-06-07T22:47:09Z

src/System.Security.Cryptography.Pkcs/tests/EnvelopedCms/DecryptTestsUsingCertWithPrivateKey.cs

            Assert.Equal<byte>(content, contentInfo.Content);
        }
+
+        [Fact]


This change is unrelated but I noticed this test does not run (Xunit doesn't run static tests in abstract class)

krwq · 2018-06-07T22:48:09Z

src/System.Security.Cryptography.Pkcs/tests/EnvelopedCms/DecryptTests.cs

        }

        [Fact]
-        public static void DecryptUsingCertificateWithSameSubjectKeyIdentifierButDifferentKeyPair()


I move this to a different place as it is not running right now (two conflicting PRs happened at similar time but they did not cause merge conflict)

krwq · 2018-06-08T17:48:27Z

@bartonjs the only difference between Windows & Manged implementation is indefinite length encoding - Windows doesn't like it - on managed implementation I allowed it as there is no reason not to.

krwq · 2018-06-08T17:50:12Z

src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/AnyOS/ManagedPal.Decrypt.cs

-                            decrypted[0] = 0x30;
-                        }
-                    }
+                    decrypted = GetAsnSequenceWithContentNoValidation(decrypted);


seems like we were always going into the catch path before so this got slightly smaller

krwq · 2018-06-08T20:15:00Z

@bartonjs there is a difference in behavior when encrypting Pkcs7Enveloped and passing an empty array.

NetFX does not allow it, netcoreapp Windows implementation allows it and managed implementation does allow it now (before this PR it did not).

I have mixed feelings if empty array should be valid or not in this context

bartonjs · 2018-06-08T21:24:01Z

src/System.Security.Cryptography.Pkcs/tests/EnvelopedCms/DecryptTests.cs

    {
        private bool _useExplicitPrivateKey;
        public static bool SupportsCngCertificates { get; } = (!PlatformDetection.IsFullFramework || PlatformDetection.IsNetfx462OrNewer);
+        public static bool SupportsIndefiniteLengthEncoding { get; } = !PlatformDetection.IsWindows;


Having talked in person about this, I think it makes sense for us to just make this work on Windows. (Or make it not work on Unix, which is complicated :)). Either in this issue or a separate one is fine.

Something like if we get back CRYPT_E_UNEXPECTED_ENCODING (which is what I'm expecting) from CryptMsgControl then we try again by running the value through WriteOctetString(berReader.PeekContentBytes()).

krwq · 2018-06-08T21:50:33Z

...m.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/Windows/PkcsPalWindows.Encrypt.cs


                if (encodedContent.Length > 0)
                {
+                    // Windows will throw if it encounters indefinite length encoding.


also encodedContent.Length > 0 is probably why empty array works on Windows 😄

bartonjs · 2018-06-08T22:13:57Z

...m.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/Windows/PkcsPalWindows.Encrypt.cs

                {
+                    // Windows will throw if it encounters indefinite length encoding.
+                    // Let's reencode if that is the case
+                    ReencodeIfUsingIndefiniteLengthEncodingOnOuterStructure(ref encodedContent);


Looks like this method isn't very hygenic; but we should fix it while we're here (and not make it worse).

The array from line 36 (id-data) is a copy, we should clear it and be nice to the user data. If Reencode makes a copy then we need to clear that one, too. But if we hit line 42 and don't rewrite it via reencode then we mustn't copy it, because the user still has access to that array. (ContentInfo.Content returns the raw array, not a clone).

bartonjs · 2018-06-08T22:17:53Z

...m.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/Windows/PkcsPalWindows.Encrypt.cs

+                }
+                finally
+                {
+                    Array.Clear(oldContent, 0, oldContent.Length);


I know I said (in person) to switch to CryptographicOperations.ZeroMemory; but this file probably compiles under netstandard, which doesn't have that API. So Array.Clear is fine.

Allow EnvelopedCms to round-trip correctly cross platform when Conten…

ee40378

…tInfo type is not 1.2.840.113549.1.7.1 (Pkcs7Data)

krwq added the Servicing-Consider-2.1.1 label May 25, 2018

krwq requested review from GrabYourPitchforks, bartonjs and morganbr May 25, 2018 19:54

krwq added the area-System.Security label May 25, 2018

krwq self-assigned this May 25, 2018

fix failures on some platforms

27ba447

bartonjs reviewed May 26, 2018

View reviewed changes

Relax DER to BER

a576ac9

bartonjs approved these changes Jun 4, 2018

View reviewed changes

bartonjs suggested changes Jun 4, 2018

View reviewed changes

danmoseley removed the Servicing-Consider-2.1.1 label Jun 4, 2018

krwq added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Jun 4, 2018

Compatibility fixes

9dd621c

krwq commented Jun 7, 2018

View reviewed changes

Add more test cases

79f7a5f

krwq commented Jun 8, 2018

View reviewed changes

fix and rename failing test

8b5bbd5

bartonjs reviewed Jun 8, 2018

View reviewed changes

allow indefinite encoding for enveloped/signed pkcs7

ff14499

krwq commented Jun 8, 2018

View reviewed changes

clear user secret when reencoding

e3969a6

bartonjs reviewed Jun 8, 2018

View reviewed changes

krwq added 2 commits June 8, 2018 15:24

avoid copies of user secrets

902b3df

change back to Array.Clear and ensure we don't erase original data

6af0c00

bartonjs approved these changes Jun 8, 2018

View reviewed changes

bartonjs merged commit 30e3172 into dotnet:master Jun 8, 2018

bartonjs mentioned this pull request Jun 15, 2018

[release/2.1]: Fix compatibility issues vs NetFX in SignedCms and EnvelopedCms #30432

Merged

karelz added this to the 3.0 milestone Jul 8, 2018

Conversation

krwq commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jun 4, 2018

Uh oh!

krwq Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krwq Jun 7, 2018 •

edited

Loading