Add crypto *Formatter classes and extend HashAlgorithm by steveharter · Pull Request #11959 · dotnet/corefx

steveharter · 2016-09-22T00:44:20Z

Three sets of changes:
(primary) #9990 Port System.Security.Cryptography.*Formatter
(dependency) #9983 Port System.Security.Cryptography.HashAlgorithm
(trivial) #9991 Port KeyedHashAlgorithm.KeyValue

The abstract formatter types mentioned in #9990 were placed into the Algorithms assembly instead of Primitives to make the code cleaner when converting from a HashAlgorithm type to the string-based algorithm version of that type (see HashAlgorithmNames.ToAlgorithmName())

The formatter types have some questionable semantics but kept it this way for desktop compat. This includes Parameters property, 'dead' code in DSA's SetHashAlgorithm, and some missed validation on null parameters.

steveharter · 2016-09-22T00:45:41Z

@bartonjs please review

bartonjs · 2016-09-22T01:21:27Z

+        [Fact]
+        public static void FormatterArguments()
+        {
+            AsymmetricSignatureFormatterTests.FormatterArguments(new DSASignatureFormatter());


The tests which don't involve instantiating an object should move to the Algorithms library specifically (instead of being in the factory/conformance include files)

Done. Moved those two tests and from RSA side to new files in Algorithms

bartonjs · 2016-09-22T01:21:42Z

+        {
+            using (DSA dsa = DSAFactory.Create())
+            {
+


Nit: whitespace

bartonjs · 2016-09-22T01:51:39Z

+                return DSAFactory.SupportsFips186_3;
+            }
+        }
+    }


RSA has a known value test for PKCS#1, why doesn't DSA have a known value test?

Added a test for a known signature although not sure how valuable that is w.r.t. RSA PKCS tests because those validate a given hash will create the same signature each time which isn't true in DSA (not deterministic)

bartonjs · 2016-09-22T01:52:40Z

+        [Fact]
+        public static void VerifyDecryptKeyExchangeOaep()
+        {
+            using (RSA rsa = RSAFactory.Create(1024))


is the 1024-ness necessary? If so, can we write that down?

No. Removed it and using default

bartonjs · 2016-09-22T01:53:47Z

+
+namespace System.Security.Cryptography.Tests
+{
+    public class AsymmetricKeyExchangeFormatterTests


This can entirely move to Algorithms by itself.

bartonjs · 2016-09-22T02:10:17Z

+
+        public override void SetHashAlgorithm(string strName)
+        {
+            // The implemenation is symmetric with RSAPKCS1SignatureFormatter even though _algName


Same "not what Desktop does".

bartonjs · 2016-09-22T02:11:32Z

+
+        public override string Parameters
+        {
+            get


can we lift these up to one-liners?

public override string Parameters { get { return null; } set { } }

You thumbs-up'd this, but didn't make the corresponding change. I'm confused...

Looks like I lifted the other formatter but not deformatter. Done

bartonjs · 2016-09-22T02:12:53Z

+{
+    public class DSASignatureFormatter : AsymmetricSignatureFormatter
+    {
+        DSA _dsaKey;


Style: use explicit modifiers (pervasive)

Sure. Forgot that. Thanks

bartonjs · 2016-09-22T02:15:21Z

+                // Verify the name
+                Oid.FromFriendlyName(strName, OidGroup.HashAlgorithm);
+
+                // Keep the raw strName as Oid may change the case ("SHA1" to "sha1") making it incompatible.


Whatever places we care that it's cased right are probably porting bugs we didn't see because of the predefined HashAlgorithmName values; but they'd be good to identify.

The Oid code round-tripping from friendlyname->value->friendlyname causes "SHA1" to become "sha1" which doesn't work later on...
https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs#L189

Right... the "doesn't work later on..." is (probably) a bug due to having a switch on non-normalized inputs. One concern I have with this is if someone passes "sha1" here we'll validate it, then let it fail later.

The "sha1" in this case is passed all the way to ncrypt which fails with NTE_NOT_SUPPORTED
https://github.com/dotnet/corefx/blob/master/src/Common/src/Internal/Cryptography/CngCommon.SignVerify.cs#L25 (*pPaddingInfo contains a struct that has "sha1"). And ncrypt doesn't like "1.3.14.3.2.26" either.

CryptoConfig in desktop uses "SHA1"
ht.Add("SHA1", Constants.OID_OIWSEC_SHA1);

In latest code, I perform a check against known hash algorithms and do an string.ToUpper

bartonjs · 2016-09-22T04:09:16Z

+            }
+            else
+            {
+                outputBytes = new byte[0];


Array.Empty()

steveharter · 2016-09-22T18:14:32Z

A new build is required to fix the refs. See 5c2c440#diff-bd3e14409afbfd7c5cb1880ed53c7ae0

steveharter · 2016-09-26T13:45:34Z

test Innerloop OSX Debug Build and Test

bartonjs · 2016-09-26T15:43:25Z

+        internal static byte[] GetDSA1024_186_2_Data()
+        {
+            return Encoding.ASCII.GetBytes("abc");
+        }


These three methods feel like they should be one method returning three values. It's pretty clunky this way.

The original intent was that the three parts could be used independently (i.e. not always all 3 would be used in a given test), but since that is not the case I can combine

bartonjs · 2016-09-26T15:50:00Z

+            VerifySignatureWithHashBytes(formatter, deformatter, hash);
+
+            // Check that the hash is preserved from the ComputeHash above
+            VerifySignatureWithHashAlgorithm(formatter, deformatter, hashAlgorithm);


That comment makes it feel more like a HashAlgorithm test than an AsymmetricSignatureFormatter test. But I think it's the comment that needs adjusting, instead of the test (since it is testing the other overload).

I removed the comment

bartonjs · 2016-09-26T15:53:52Z

        public byte[] P;
        public byte[] Q;
    }
+    [System.Runtime.InteropServices.ComVisibleAttribute(true)]


In NetFx both the Deformatter and Formatter are ComVisible. I don't know that we're preserving ComVisible attributes at all, but I'm definitely confused as to why it isn't being done consistently.

I removed it. Apparently I ran the genapi tool before I had removed all of the attributes from the corresponding source file.

bartonjs · 2016-09-26T15:58:09Z

+        {
+            if (ALL_NAMES.IndexOf(hashAlgorithName, StringComparison.OrdinalIgnoreCase) >= 0)
+            {
+                return hashAlgorithName.ToUpperInvariant();


This seems pretty weird. A HashSet seems much more suited to this than strstr. Especially because you're not checking a word boundary. So "m", "d", "ha", "a2", etc are all treated as "known" by this algorithm.

The Oid is already verified at this point to be a valid hash algorithm... HashSet seems like overkill but if you agree that we should only check against known names (and not do a simple ToUpper on any potentially future\other hash alg) I will change it

In current usage it is; that's not anything asserted by or guaranteed in this routine. If we added support for a new hash algorithm we'd have to add it to the ALL_NAMES string unless it was conveniently a substring of one of the existing names. So HashSet or just an array, or whatever; but it needs to be a hard list, not a soft one.

Done. Added HashSet

bartonjs · 2016-09-26T16:03:26Z

+            try
+            {
+                // Verify the name is a valid HashAlgorithm even though it is not currently used
+                Oid.FromFriendlyName(strName, OidGroup.HashAlgorithm);


Please make this just check that the input is a legal identifier for SHA1 to match Desktop.

Desktop currently only supports SHA1, and we should be thoughtful when that behavior is changed. Do we allow only FIPS 186-3-acceptable algorithms (SHA-1 and SHA-2-x)? Do we widen it up to all resolvable HashAlgorithms? Do we just get rid of the check altogether?

We should keep porting differences to a minimum; particularly when it's not due to not being able to easily replicate a behavior on Unix.

Let's take this offline for a bit

Per discussion, hard-code to SHA1 for now (to match desktop) and then log an issue to allow additional

bartonjs · 2016-09-26T16:05:15Z

+            _rsaKey = (RSA)key;
+        }
+
+        /// <internalonly/>


We can probably get rid of these comments. Especially since I have no idea what they mean.

Sure will remove. I noticed that there's a bunch of others in our source so I left them originally.

bartonjs · 2016-09-26T16:12:27Z

+            {
+                VerifyICryptoTransformStream(stream, output);
            }
+#endif


There are some quirky behaviors in NetFx HashAlgorithm that we should have tests to ensure we don't change unless we mean to.

TransformBlock(a) followed by ComputeHash(b) is the same as ComputeHash(a.Concat(b))

TransformBlock(a), TransformFinalBlock(b), TransformBlock(a), TransformFinalBlock(b) is (I think) ComputeHash(a.Concat(b).Concat(a).Concat(b))

TransformBlock(a), TransformFinalBlock(b) => x, ComputeHash(Array.Empty() => same x => ComputeHash(Array.Empty()) => correct empty hash answer

TransformBlock(a), TransformFinalBlock(b) => x, ComputeHash(Array.Empty() => same x => TransformBlock(a), TransformFinalBlock(b) => same x (because ComputeHash reset the state).

Also, a whole bunch of exception tests for TransformBlock and TransformFinalBlock.

Done. Note after a call to TransformFinalBlock an exception is raised if TransformBlock or TransformFinalBlock is immediately called again unless transforming 0 bytes..

There are differences between netfx and corefx here. See the ActiveIssue and todo in the tests

bartonjs · 2016-09-26T16:16:30Z

        public override long Position { get { return default(long); } set { } }
        protected override void Dispose(bool disposing) { }
        public override void Flush() { }
-        public override System.Threading.Tasks.Task FlushAsync(System.Threading.CancellationToken cancellationToken) { return default(System.Threading.Tasks.Task); }


Where'd this override go?

I didn't remove it :) It looks like it was never overidden to begin with; it exists in the base class

The netfx code does override FlushAsync, but the implementation is the same as the base class (Stream) so I suppose it wasn't carried over. It was a no-op to begin with; Flush() is also no-op

@weshaggard I assume we need (in the end) all the overrides to align on non-sealed types for base. invocations; is that being well-flagged by the current netstandard20 deviations process?

In general we don't enforce that a member is overridden but we do try to keep the overridden member in the ref if we know of any implementations that have the override.

bartonjs · 2016-09-26T16:17:37Z

+                if (_disposed)
+                    throw new ObjectDisposedException(null);
+                if (State != 0)
+                    throw new CryptographicUnexpectedOperationException(SR.Cryptography_HashNotYetFinalized);


Let's see some tests for these exceptions, too, please.

bartonjs · 2016-09-26T16:20:17Z

+        public CryptographicUnexpectedOperationException()
+            : base(SR.Arg_CryptographyException)
+        {
+        }


Set HResult to 0x80131431?

Possibly , but then we should also change CryptographicException as well as that doesn't set CORSEC_E_CRYPTO

internal const int CORSEC_E_CRYPTO = unchecked((int)0x80131430); internal const int CORSEC_E_CRYPTO_UNEX_OPER = unchecked((int)0x80131431);

That's fair. @stephentoub any thoughts on if we care about making the HResult values of the default ctors of these two exceptions match desktop? The HResult value of CryptographicException is pretty hard to depend on since we don't try to normalize the codes between Windows and Unix (or across versions of Windows).

steveharter · 2016-09-28T00:14:45Z

+        }
+
+        [Fact]
+        [ActiveIssue(0)] // Todo: netfx treats HashFinal and Initialize separately, corefx doesn't and Initialize is a no-op


Compare corefx at https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/SHA1.cs#L54-L59 and netfx at http://referencesource.microsoft.com/#mscorlib/system/security/cryptography/sha1managed.cs,49

If there is an issue that cannot be resolved then an actual issue needs to be created and it tracked here. If it's a behavior deviation that we're okay with (I don't know if we have a way of really deciding/codifying what we're okay with as a deviation or not) then we need to make the test do the thing we decide on.

But [ActiveIssue(0)] isn't acceptable.

The ActiveIssue is temporary and for discussion; I do not intend to submit that way. For now I will emulate netfx behavior unless I hear objections otherwise.

bartonjs · 2016-09-28T00:22:27Z

+                hash.TransformBlock(block, 0, block.Length, null, 0);
+                hash.TransformFinalBlock(block, 0, block.Length);
+
+                // Todo: these do not throw on corefx, but do on netfx


Doesn't seem like it would be hard to make this also throw on corefx; just requires tracking being in this state.

bartonjs · 2016-09-28T00:25:25Z

+
+                // Todo: this will also fail due to the ActiveIssue below in VerifyUseAfterTransformFinalBlock
+                // but commented out so the other tests can run
+                // actual = hash.ComputeHash(Array.Empty<byte>(), 0, 0);


I'm not sure what the comment means here. Do we give the empty hash answer on corefx, but the tainted hash answer on netfx? I'd expect the call to ComputeHash to be necessary on netfx to set up for the next test...

yes corefx=empty, netfx=tainted\same as last. On whether ComputeHash is necessary on netfx for the next call - yes it is (that's what the ActiveIssue \ VerifyUseAfterTransformFinalBlock method checks for)

bartonjs · 2016-09-28T00:32:25Z

        public override long Position { get { return default(long); } set { } }
        protected override void Dispose(bool disposing) { }
        public override void Flush() { }
-        public override System.Threading.Tasks.Task FlushAsync(System.Threading.CancellationToken cancellationToken) { return default(System.Threading.Tasks.Task); }


@weshaggard I assume we need (in the end) all the overrides to align on non-sealed types for base. invocations; is that being well-flagged by the current netstandard20 deviations process?

bartonjs · 2016-09-28T00:34:00Z

+        public CryptographicUnexpectedOperationException()
+            : base(SR.Arg_CryptographyException)
+        {
+        }


That's fair. @stephentoub any thoughts on if we care about making the HResult values of the default ctors of these two exceptions match desktop? The HResult value of CryptographicException is pretty hard to depend on since we don't try to normalize the codes between Windows and Unix (or across versions of Windows).

bartonjs · 2016-09-28T00:36:40Z

+            {
+                var formatter = new DSASignatureFormatter(dsa);
+                var deformatter = new DSASignatureDeformatter(dsa);
+                VerifySignature(formatter, deformatter, SHA1.Create(), "SHA1");


I'd like to see the hash algorithm object creations put in using statements. (Either I never made that comment or it got lost, because I can't find it now)

bartonjs · 2016-09-28T00:38:46Z

+        }
+
+        [Fact]
+        [ActiveIssue(0)] // Todo: netfx treats HashFinal and Initialize separately, corefx doesn't and Initialize is a no-op


If there is an issue that cannot be resolved then an actual issue needs to be created and it tracked here. If it's a behavior deviation that we're okay with (I don't know if we have a way of really deciding/codifying what we're okay with as a deviation or not) then we need to make the test do the thing we decide on.

But [ActiveIssue(0)] isn't acceptable.

bartonjs · 2016-09-29T00:12:36Z

        }

-        private byte[] CaptureHashCodeAndReinitialize()
+        private byte[] CaptureHashCodeAndReinitialize(int inputCount)


Why do we process the input count after doing all the work? I'm a bit confused...

This is for netfx compat to throw when appropriate. In netfx the only way to "clear" after a TransformFinalBlock so it can hash again is to call ComputeHash(empty) which calls Initialize(). It's messy but due to the differences in Initialize + HashFinal between corefx and netfx -- corefx doesn't do anything in Initialize since it does everything in HashFinal thus it wouldn't normally throw when netfx does. Hope that helps, otherwise we can take it offline

steveharter · 2016-09-29T01:53:21Z

test Innerloop CentOS7.1 Release Build and Test
(MSBuild Error)

bartonjs · 2016-09-29T17:16:21Z

New information leads to new conclusions. I found out this morning that I was aware only of one of the (at least) THREE different behaviors we have regarding TransformFinalBlock / ComputeHash interaction.

SHA1Managed: Never throws, has the concatenation effect I described in my comment previously.
SHA1CryptoServiceProvider: Has the behavior that you are trying to emulate. (Throws in some cases, gets fixed by ComputeHash(empty))
Sha1Cng: Once it throws once it never seems to recover. TransformFinalBlock appears to be the death of this object.

Given that there's no one behavior to match for compatibility; I'm now happiest with the 4th behavior of "always give the correct answer", and will pursue change for future NetFx.

bartonjs · 2016-09-29T17:16:55Z

(Having said that, for the "match SHA1CryptoServiceProvider" approach I like the revisions of this latest commit)

bartonjs

Aside from not understanding why some csproj conditions got rewritten, LGTM.

bartonjs · 2016-10-04T18:10:52Z

-    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>
-    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.6</NugetTargetMoniker>
+    <DefineConstants Condition="'$(TargetGroup)'==''">$(DefineConstants);netstandard17</DefineConstants>
+    <NugetTargetMoniker Condition="'$(TargetGroup)'==''">.NETStandard,Version=v1.7</NugetTargetMoniker>


Why is this condition changing? (I understand the value changing, but not the condition)

bartonjs · 2016-10-04T18:23:13Z

    <Compile Include="TripleDesTests.cs" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetGroup)'=='netstandard1.7'">
+  <ItemGroup Condition="'$(TargetGroup)'==''">


Why is this condition changing?

Tests were changed globally to run on 1.7 instead of 1.6 by default. I didn't change it in the rebase so it appeared here.

bartonjs · 2016-10-04T18:35:04Z

Okay, I guess I'm happy after a squash.

…#11959) Commit migrated from dotnet/corefx@eb2d649

dnfclas added the cla-already-signed label Sep 22, 2016

bartonjs suggested changes Sep 22, 2016

View reviewed changes

steveharter force-pushed the AddCryptoFormattersMaster branch 6 times, most recently from a2e4f8c to 444962d Compare September 24, 2016 21:24

karelz added the area-System.Security label Sep 26, 2016

bartonjs suggested changes Sep 26, 2016

View reviewed changes

steveharter commented Sep 28, 2016

View reviewed changes

bartonjs suggested changes Sep 28, 2016

View reviewed changes

bartonjs reviewed Sep 29, 2016

View reviewed changes

bartonjs mentioned this pull request Sep 30, 2016

LeaveOpen ctor for CryptoStream #11587

Closed

steveharter added 6 commits October 4, 2016 10:19

Add crypto formatters

4e5ff50

Code review feedback

54e4e14

fix comment

6cdb948

Code review feedback

139d4cf

Review feedback plus new crypto exception ctors

a845a14

HashAlgorithm netfx compat refactor

b4f6fc4

steveharter force-pushed the AddCryptoFormattersMaster branch from 0a5bb9b to b4f6fc4 Compare October 4, 2016 15:23

TransformFinalBlock assumes Initialize semantics

48f63a4

bartonjs reviewed Oct 4, 2016

View reviewed changes

bartonjs approved these changes Oct 4, 2016

View reviewed changes

steveharter merged commit eb2d649 into dotnet:master Oct 4, 2016

steveharter deleted the AddCryptoFormattersMaster branch October 6, 2016 18:17

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add crypto *Formatter classes and extend HashAlgorithm (dotnet/corefx…

796c571

…#11959) Commit migrated from dotnet/corefx@eb2d649

Conversation

steveharter commented Sep 22, 2016

Uh oh!

steveharter commented Sep 22, 2016

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!

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!

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!

steveharter Sep 23, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter commented Sep 22, 2016

Uh oh!

steveharter commented Sep 26, 2016

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!

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

steveharter Sep 23, 2016 •

edited

Loading

bartonjs Sep 26, 2016 •

edited

Loading

steveharter Sep 27, 2016 •

edited

Loading