dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.18k stars 4.72k forks source link

When using EnvelopedCms class to encrypt data in Linux we encounter extra bytes when we decrypt on Windows. #27705

Closed samdearden closed 4 years ago

samdearden commented 6 years ago

We are encrypting data on Linux from within a Docker container using the EnvelopedCms class. We are using dotnet core 2.1.5.

When creating and encrypting a CMS package, we can correctly decrypt without issue from within another Linux docker container using the .NET Core.

Decrypting the same package on a windows machine or via the bouncy castle library (either on windows or Linux) an additional two bytes are included at the start of the data content.

EncryptedCmsPackageOpening.zip EncryptedCmsPackageCreation.zip

The attached sample applications allow the reproduction of this behaviour:

When the EncryptedCmsPackageOpening project is run from a windows machine the results are as follows: Decrypted windows data: [171, 72, 239, 148, 66, 232, 90, 224, 39, 239, 56, 83, 149, 104, 162, 93, 250, 213, 107, 8] Decrypted data from linux: [4, 20, 171, 72, 239, 148, 66, 232, 90, 224, 39, 239, 56, 83, 149, 104, 162, 93, 250, 213, 107, 8] - Note the additional 2 bytes at the start of the content.

When the EncryptedCmsPackageOpening project is run from a docker container the results are as follows: Decrypted windows data: [171, 72, 239, 148, 66, 232, 90, 224, 39, 239, 56, 83, 149, 104, 162, 93, 250, 213, 107, 8] Decrypted data from linux: [171, 72, 239, 148, 66, 232, 90, 224, 39, 239, 56, 83, 149, 104, 162, 93, 250, 213, 107, 8]

vcsjones commented 6 years ago

Looks like the data is accidentally including the ASN.1 tag and length. "4" is the ASN.1 tag type, or Octet String. "20" is the number of octets. "171, 72, 239 ... 107, 8" is 20 bytes long.

/cc @bartonjs

vcsjones commented 6 years ago

This looks like the same issue as dotnet/runtime#26237 which was fixed by dotnet/corefx#30432 and dotnet/corefx#29926.

@samdearden just to double check, are you sure you are running 2.1.5 in the environment this reproduces, and are using the latest package version of System.Security.Cryptography.Pkcs (4.5.1 at the time of writing)?

bartonjs commented 6 years ago

@samdearden Additionally if both Linux and Linux-in-Docker are using 2.1.5, are they both using Microsoft.NETCore.App, or is one of them using a different apphost, like AspNetCore? (In case we have a secondary host bundling-induced difference).

samdearden commented 6 years ago

@vcsjones Confirmed we are running 2.1.5 in the environment, if we run dotnet--list-runtimes it shows the only version is 2.1.5. We are referencing 4.5.1 of System.Security.Cryptography.Pkcs in our project file (attached).

@bartonjs No app host, see attached code which we can use to reproduce the problem.

vcsjones commented 6 years ago

I can reproduce this with the steps provided.

  1. Download and extract the two zip files.
  2. Run EncryptedCmsPackageCreation in Windows using dotnet run.
  3. Take the printed bytes from the previous step and paste them in to inputArrayFromWindows in EncryptedCmsPackageOpening.
  4. Run EncryptedCmsPackageCreation in Linux / Docker using docker build . and running the resulting container.
  5. Take the printed bytes from the previous step and paste them in to inputArrayFromLinux in EncryptedCmsPackageOpening.
  6. Run EncryptedCmsPackageOpening from Windows using dotnet run. Result: the printed values are not the same.
  7. Run EncryptedCmsPackageOpening from Docker / Linux by running docker build .. Result: the printed values are the same.
JochenKalmbach commented 5 years ago

We have the same problem in our code... we also have a small repro code, if needed ;) Any hint, when this will be solved? Or what is the recommended workaround?

Currently this is a show-stopper on deploying our application to Linux...

bartonjs commented 5 years ago

I've been tied up on other projects, and continue to be tied up. If anyone has some spare debugging cycles, assistance is welcome.

vcsjones commented 5 years ago

@bartonjs I'll take a look again later this evening.

bartonjs commented 5 years ago

@vcsjones Did you ever make progress on investigating this?

JochenKalmbach commented 5 years ago

@vcsjones, @bartonjs We switched to BouncyCastle portable...it works on all systems perfect...

vcsjones commented 5 years ago

Ah, hm, I started looking at this and it fell off my radar. Let me see where I left off and what I found this evening.

vcsjones commented 5 years ago

I'm still looking at this, I've distilled it down to a failing test:

[Fact, PlatformSpecific(TestPlatforms.Windows), /* OuterLoop */]
public static void Roundtrip_Between_Pal()
{
    Type anyOSPalType = typeof(EnvelopedCms).Assembly.GetType("Internal.Cryptography.Pal.AnyOS.ManagedPkcsPal", throwOnError: true);
    object anyOsPal = Activator.CreateInstance(anyOSPalType, nonPublic: true);
    MethodInfo anyOsEncrypt = anyOSPalType.GetMethod("Encrypt", new Type[] {
        typeof(CmsRecipientCollection), typeof(ContentInfo), typeof(AlgorithmIdentifier),
        typeof(X509Certificate2Collection), typeof(CryptographicAttributeObjectCollection)
    });

    ContentInfo contentInfo = new ContentInfo(new byte[] { 1, 2, 3 });
    CmsRecipientCollection recipients = new CmsRecipientCollection();
    AlgorithmIdentifier encryptionAlgorithm = new AlgorithmIdentifier(Oid.FromOidValue(Oids.TripleDesCbc, OidGroup.EncryptionAlgorithm));
    X509Certificate2Collection originatorCerts = new X509Certificate2Collection();
    CryptographicAttributeObjectCollection unsignedAttributes = new CryptographicAttributeObjectCollection();

    byte[] encodedMessage;

    using (X509Certificate2 issuerSerialCert = Certificates.RSAKeyTransfer1.GetCertificate())
    {
        recipients.Add(new CmsRecipient(SubjectIdentifierType.IssuerAndSerialNumber, issuerSerialCert));
        encodedMessage = (byte[])anyOsEncrypt.Invoke(anyOsPal, new object[] { recipients, contentInfo, encryptionAlgorithm,
            originatorCerts, unsignedAttributes });
    }

    Assert.NotEmpty(encodedMessage);

    EnvelopedCms ecms = new EnvelopedCms();
    ecms.Decode(encodedMessage);

    using (X509Certificate2 privateCert = Certificates.RSAKeyTransfer1.TryGetCertificateWithPrivateKey())
    {
        if (privateCert == null)
            return; // CertLoader can't load the private certificate.

        ecms.Decrypt(new X509Certificate2Collection(privateCert));
    }

    Assert.Equal(contentInfo.ContentType.Value, ecms.ContentInfo.ContentType.Value);
    Assert.Equal<byte>(contentInfo.Content, ecms.ContentInfo.Content);
}

@bartonjs is there a better way to test the managed PAL from Windows without relying on reflection? In general I think trying to add additional tests that validate parity between PALs would be a good idea, so anything we can do that make that easier would be nice...

Will fail with:

 Assert.Equal() Failure
 Expected: Byte[] [1, 2, 3]
 Actual:   Byte[] [4, 3, 1, 2, 3]
JochenKalmbach commented 5 years ago

My suggestion would be: create a cert on linux (and a different one on windows or for each supported platform) and encrypt and sign some data. Then add this data-files to the unit test project and try to decrypt and verify the content... this would be a simple and real test...

vcsjones commented 5 years ago

Then add this data-files to the unit test project and try to decrypt and verify the content... this would be a simple and real test...

Sure, but I still think there is value in an end-to-end roundtrip test to mitigate any other future incompatibilities between PALs.

bartonjs commented 5 years ago

@vcsjones IIRC, the managed pal is used on Windows when you use the Decrypt(RecipientInfo recipientInfo, AsymmetricAlgorithm privateKey) overload.

JochenKalmbach commented 5 years ago

Your unit test runs always one ONE platform at a given time. You will never be able to archive this goal...

vcsjones commented 5 years ago

Your unit test runs always one ONE platform at a given time. You will never be able to archive this goal...

It's less about platforms and more about the PAL. EnvelopedCms has two PALs: Windows P/Invoke and a managed implementation. A test running on Windows can exercise both the Windows and managed PAL just fine (which is exactly what that test does, it just does so with reflection). So even though the test is running on one platform at a given time, I can use the same code that macOS, Linux, etc are using.

vcsjones commented 5 years ago

@bartonjs I'm looking through the CMS spec and trying to reconcile it with the existing code. I think the problem is that we are just unnecessarily wrapping the content in an octet string prior to encryption when the content type is id-data:

https://github.com/dotnet/corefx/blob/31781586a489a7cecc61bc3d1f14fc0edbb3be06/src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/AnyOS/ManagedPal.Encrypt.cs#L185

And also unnecessarily unwrapping it from an octet string during decrypt:

https://github.com/dotnet/corefx/blob/31781586a489a7cecc61bc3d1f14fc0edbb3be06/src/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/AnyOS/ManagedPal.Decrypt.cs#L116

I don't see anything in the CMS spec that says the EncryptedContentInfo's EncryptedContent needs to be pushed in to an octet string before encryption. 6.3 makes a note about using PKCS#7 padding for block algorithms, but that's about it.

The encrypted data does get written as an octet string to the ECI with an implicit tag of 0 which is handled by the serializer.

Is there a part of the spec that I missed that makes this more nuanced than "Just don't wrap it in an octet string"?

bartonjs commented 5 years ago

https://tools.ietf.org/html/rfc5652#section-4

The data content type is intended to refer to arbitrary octet strings, such as ASCII text files; the interpretation is left to the application.

But maybe this was a bad application of Signing's needs to Enveloping, since Envelope is encrypting it.

I'd like to think I added it because of interop with either Windows or OpenSSL; but I'm having trouble justifying it to myself right now :smile:.

vcsjones commented 5 years ago

Hm, OK. If the fix is as simple as "stop encoding as an octet string prior to encrypting" that is an easy enough change, but doing so in a compatible way is trickier.. if there are EncryptedCmss that have been made by the managed PAL, persisted, then we stop attempting to decode it, then the managed PAL will start returning the extra octet string bytes and windows will stop.

I suppose we could try to decode it, make sure the length matches, and return that. If we can't decode it or the length doesn't match then we return the content as-is.

vcsjones commented 5 years ago

@bartonjs

I took a look at other implementations including BouncyCastle and it seems they do not wrap the contents in an octet string before encryption. BouncyCastle is taking the contents, a CmsProcessable and shoves it through a CryptoStream.

I would then suggest we make the following changes.

  1. Remove the call to EncodeOctetString(toEncrypt) during encrypt.
  2. Leave Decrypt alone. Decrypt currently tries to unwrap the contents from an octet string. When that fails, it just returns the contents as-is if decoding the octet string fails. I suggest leaving this alone so that any enveloped CMS's that were created continue to return only the inner contents of the octet string. This has the unfortunate side-effect that it is incorrectly decoding the contents as an octet string, however that behavior already exists today if someone tried using EnvelopedCms's decrypt on a CMS that was not made in .NET Core. Perhaps we add a comment to explain why that is there.
  3. Add a unit test to ensure that CMS's with the extra octet string wrapping continue to decrypt correctly.

What do you think?

bartonjs commented 5 years ago

@vcsjones I believe the answer I'm looking for is

Make it so