Add DSA support to OpenSsl and Cng assemblies by steveharter · Pull Request #11359 · dotnet/corefx

steveharter · 2016-09-01T22:11:34Z

Includes some changes to Algorithms as well.

steveharter · 2016-09-01T23:24:30Z

@bartonjs if you want to take a look. Most of the effort was setting up the contracts\packages.
@joperezr for netcoreapp1.1\package work. Should have all three projects with 1.0 and 1.1 (netstandard 1.6\1.7)

stephentoub · 2016-09-02T11:13:07Z

+        internal static extern SafeDsaHandle EvpPkeyGetDsa(SafeEvpPKeyHandle pkey);
+
+        [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_EvpPkeySetDsa")]
+        [return: MarshalAs(UnmanagedType.Bool)]


Nit: this is the default

I believe this attribute is required for fxcop CA1414... To encourage thinking about whether .U1 should be used if marshalling c++ bool. https://msdn.microsoft.com/en-us/library/ms182206.aspx

steveharter · 2016-09-02T13:36:11Z

test Innerloop Windows_NT Release Build and Test please

bartonjs · 2016-09-02T14:51:52Z

-            /// continue to manage the lifetime of their reference.
-            /// </remarks>
-            /// <param name="handle">A pointer to an OpenSSL <c>EC_KEY*</c></param>
-            /// <exception cref="ArgumentException"><paramref name="handle" /> is invalid</exception>


This is public; why are you deleting it?

Nevermind, I see that it was just moved.

joperezr · 2016-09-02T16:16:01Z

@steveharter I'm about to merge #11366 could we hold this change until that's done? I expect this to have some conflicts, and I think it will be easier to fix in your side, since basically I would have to start all over again with my merge.

joperezr · 2016-09-02T20:52:47Z

To fix your CI failures, run:

msbuild \src\System.Security.Cryptography.OpenSsl\pkg\System.Security.Cryptography.OpenSsl.pkgproj /t:UpdatePackageIndex

and

msbuild \src\System.Security.Cryptography.Cng\pkg\System.Security.Cryptography.Cng.pkgproj /t:UpdatePackageIndex

and then commit the changes on the packageIndex file and add them to your PR

joperezr · 2016-09-02T20:54:50Z

<AssemblyVersion Condition="'$(TargetGroup)'=='net46'">4.0.0.0</AssemblyVersion>

I was expecting a change in file src\System.Security.Cryptography.Algorithms\dir.props where you would rev up the version to 4.3.0.0. Can you add that?

Refers to: src/System.Security.Cryptography.Algorithms/src/System.Security.Cryptography.Algorithms.csproj:13 in ba41851. [](commit_id = ba41851, deletion_comment = False)

joperezr · 2016-09-02T20:57:29Z

 <Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
+    <AssemblyVersion>4.3.0.0</AssemblyVersion>


Instead of adding a default assemblyversion here, could you remove this line from here (and from your src project as well) and update the dir.props bellow src/System.Security.Cryptography.Cng instead?

bartonjs · 2016-09-06T16:55:19Z

@@ -65,7 +65,7 @@ internal static int DsaKeySize(SafeDsaHandle dsa)
            // there were some leading zero bytes truncated due to BIGNUM semantics.
            // This minimizes the change the key size is wrong due to leading zero bits; there is only
            // a 2^64 chance that the key has 64 leading zero bits and thus the keysize will be wrong.


I'm not sure what the second two lines of this comment are trying to say. Particularly why 64 zeroes are relevant.

The value is returned from OpenSsl via bignum which will truncate the storage for leading zeros... So in theory without the rounding the odds of getting an incorrect key size would be 1:256.

Here's a reference point: https://www.openssl.org/docs/man1.0.1/crypto/BN_num_bits.html (see the NOTES section). In our case, we're getting the size in bytes of the P parameter. There is an OpenSsl method DSA_bits() which returns the size of P in bits, but that does not take in account an leading zeros from looking at that source

@steveharter Yeah, I understand why we add 7, divide by 8, and multiply by 8. The comment says there's a 1:2^64 chance that there are 64 leading zeroes; but that's not particularly relevant.

p = a prime modulus, where 2^L-1 < p < 2^L for 512 <= L <= 1024 and L a multiple of 64

(FIPS 186-2, section 4)

Technically, BN_num_bits should always just be correct for p (or, rather L, since L is the size of p in bits); unless it's always off by 1 for indicating the significance of the "not negative" padding bit/byte. q appears to be equally rigid, but g, x and y are soft. Those relationships seem to also hold under FIPS 186-3.

That said, I'm not opposed to byte-alignment-normalization of the value for just-in-case (defense in depth for a future enhancement to the DSA spec); just that the comment seems to be a bit off.

I'm assuming for each bit in a given p there is a 50% chance of it being zero. So for example if the first 8 bits are zero, there is one less byte required to store p in a bignum due to the semantics of bignum which doesn't pad leading bytes (thus the requested keysize would be off by 8 bits in this case). So 2^64 comes into place because we round 8 bytes (64 bits), thus the key size would be wrong (off by 64) if the first 64 bits are all zero.

Because p has to be between 2^L-1and 2^L the high bit is guaranteed set. And since it's prime, and not the number 2, the low bit is also guaranteed set.

This formula is rounding up to the next byte (in bits), not to a 64-bit boundary:
f(7) => (7 + 7) / 8 * 8 => 14 / 8 * 8 => 1 * 8 => 8.
f(8) => (8 + 7) / 8 * 8 => 15 / 8 * 8 => 1 * 8 => 8.
f(9) => (9 + 7) / 8 * 8 => 16 / 8 * 8 => 2 * 8 => 16.

That makes sense as I said I was assuming there was a 50% which if true then would have no guarantees about the strength of the key based on the key size passed in. I did remove part of that comment with last sync, but will tweak it again to remove reference to leading zeros.

Also, that formula is in bytes, not bits. So it will round up to nearest 8 byte boundary.

Now that System.Collections.NonGeneric is published with serialization support, we can enable serialization tests for System.Collection.Specialized.

…otnet#11363) * Add tests to cover the XmlSchema and XmlSerializerFactory scenarios. * Update the code based on Shin's feedback. * change the code based on Shin's feedback.

* Add 2 missing members to Exception

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

steveharter added the 3 - Ready For Review label Sep 1, 2016

steveharter changed the title ~~Add DSA support for OpenSsl and Cng~~ Add DSA support to OpenSsl and Cng assemblies Sep 1, 2016

stephentoub reviewed Sep 2, 2016
View reviewed changes

bartonjs reviewed Sep 2, 2016
View reviewed changes

steveharter force-pushed the DsaSupportForCngAndOpenSsl branch 2 times, most recently from a8d26a7 to ba41851 Compare September 2, 2016 18:26

joperezr reviewed Sep 2, 2016
View reviewed changes

steveharter force-pushed the DsaSupportForCngAndOpenSsl branch 2 times, most recently from 2972a20 to 95c46c1 Compare September 6, 2016 15:52

bartonjs reviewed Sep 6, 2016
View reviewed changes

stephentoub and others added 5 commits September 7, 2016 11:10

Fix specialized collections serialization tests

98f37cc

Now that System.Collections.NonGeneric is published with serialization support, we can enable serialization tests for System.Collection.Specialized.

Add issue number to TODO in .csproj

da4e4ce

Add tests to cover the XmlSchema and XmlSerializerFactory scenarios. (d…

1ab2ca3

…otnet#11363) * Add tests to cover the XmlSchema and XmlSerializerFactory scenarios. * Update the code based on Shin's feedback. * change the code based on Shin's feedback.

Add 2 missing members to Exception (dotnet#11442)

f3458ac

* Add 2 missing members to Exception

Add DSA support for OpenSsl and Cng

a0165df

Uh oh!

Conversation

steveharter commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveharter commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter commented Sep 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 2, 2016

Uh oh!

joperezr commented Sep 2, 2016

Uh oh!

joperezr commented Sep 2, 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!

steveharter Sep 6, 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!

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!

7 participants

steveharter commented Sep 1, 2016 •

edited

Loading

steveharter Sep 6, 2016 •

edited

Loading