Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add FixedTimeEquals and other crypto helper routines as public API #27103

Merged
merged 6 commits into from
Feb 14, 2018

Conversation

bartonjs
Copy link
Member

Fixes #10749.

@bartonjs bartonjs added this to the 2.1.0 milestone Feb 13, 2018
@bartonjs bartonjs self-assigned this Feb 13, 2018
@bartonjs
Copy link
Member Author

For stats lovers, here's some of the data from perf runs (augmented by Excel-fu) comparing the subtract-OR-accumulator to the early aborting if:

Approach Test Mean sd Min Max t p
SUB 256Bit_AllBitsDifferent 36.892 0.996 34.851 43.256 1.492526 0.137153
SUB 256Bit_CascadingErrors 36.743 0.67 35.452 38.138 0.388437 0.69811
SUB 256Bit_Equal 36.703 0.782 35.139 38.528 0 1
SUB 256Bit_FirstBitDifferent 36.576 0.767 35.117 38.605 -1.15944 0.247675
SUB 256Bit_LastBitDifferent 36.748 0.722 35.323 39.043 0.422799 0.6729
SUB 256Bit_SameReference 36.753 0.653 35.226 38.327 0.490778 0.624127
SUB 256Bit_VersusZero 36.589 0.572 35.533 37.983 -1.17663 0.240756
Early abort if 256Bit_AllBitsDifferent 4.737 0.849 4.277 12.62 -349.128 3.5E-278
Early abort if 256Bit_CascadingErrors 4.613 0.291 4.244 5.852 -588.171 0
Early abort if 256Bit_Equal 39.405 0.515 37.965 40.545 0 1
Early abort if 256Bit_FirstBitDifferent 4.484 0.219 4.137 5.082 -624.001 0
Early abort if 256Bit_LastBitDifferent 39.372 0.595 37.893 41.941 -0.41935 0.675412
Early abort if 256Bit_SameReference 39.418 0.535 38.085 40.591 0.175061 0.86121
Early abort if 256Bit_VersusZero 4.639 0.337 4.297 5.984 -564.876 0

SUB | 256Bit_AllBitsDifferent was the first run of that set, so it paid the JIT/init cost, so it should be given some leeway. I also didn't have a particularly clean machine at the time, so context switches factor in. "Early abort if" doesn't really need to bother saying who paid the JIT cost.

{
GetBytes(ref MemoryMarshal.GetReference(data), data.Length);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't FillSpan identical in each implementation? Can it just be in the shared file, and just be part of the Fill implementation there?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is currently. I think I was being defensive that at some point one of the platforms could end up needing some sort of state managed... but I guess that ambiguous future can deal with things then.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

@stephentoub
Copy link
Member

There are several places in corefx where we create a RandomNumberGenerator instance just to be able to use GetBytes. Now that this is adding a static Fill that avoids the RNG allocation, it'd be nice to use it as part of this PR.

For example, this:

private static ulong GenerateSeed()
{
using (RandomNumberGenerator rng = RandomNumberGenerator.Create())
{
var bytes = new byte[sizeof(ulong)];
rng.GetBytes(bytes);
return BitConverter.ToUInt64(bytes, 0);
}
}

could now be:

 private static ulong GenerateSeed() 
 { 
    Span<byte> bytes = stackalloc byte[sizeof(long)];
    RandomNumberGenerator.Fill(bytes);
    return BitConverter.ToUInt64(bytes, 0);
}

There's one in SocketsHttpHandler for digest auth:


That static can be removed and replaced with just a call to Fill.

Similarly there's one in ManagedWebSocket here:

s_random.GetBytes(buffer, offset, MaskLength);

though that one might be more complicated and not worth it, as I believe we compile that file into an assembly that targets netstandard20 as well.

// The chances of this failing are 1 in 1.2e24, unless the RNG is broken.
for (int i = 0; i < 10 && !hasData; i++)
{
rng.GetBytes(testSpan);
Copy link
Member

Choose a reason for hiding this comment

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

You don't want to just use Fill? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Drat, missed one :). Fill was written last :).

@bartonjs
Copy link
Member Author

There are several places in corefx where we create a RandomNumberGenerator instance just to be able to use GetBytes.

I changed Marvin, which also builds as UAP/UAPAOT for System.Private.Xml. I'll look at some others in a bit, since I have to rebase/rebuild to pull in the sockets HTTP handler.

@bartonjs
Copy link
Member Author

Looks like Marvin builds under NetFx, so it can't be changed to Fill without some #defines:

15:45:57 D:\j\workspace\windows-TGrou---2a8f9c29\src\Common\src\System\Marvin.cs(112,35): error CS0117: 'RandomNumberGenerator' does not contain a definition for 'Fill' [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Net.Primitives\tests\UnitTests\System.Net.Primitives.UnitTests.Tests.csproj]
15:45:57 D:\j\workspace\windows-TGrou---2a8f9c29\src\Common\src\System\Marvin.cs(113,33): error CS7036: There is no argument given that corresponds to the required formal parameter 'startIndex' of 'BitConverter.ToUInt64(byte[], int)' [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Net.Primitives\tests\UnitTests\System.Net.Primitives.UnitTests.Tests.csproj]

@stephentoub
Copy link
Member

Ok, thanks.

@@ -10,11 +10,12 @@
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Release|AnyCPU'" />
<ItemGroup>
<Compile Include="System.Security.Cryptography.Primitives.cs" />
<Compile Include="System.Security.Cryptography.Primitives.netcoreapp.cs" Condition="'$(TargetGroup)' == 'netcoreapp'" />
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need it specialized like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the last library I made API changes to wasn't part of netstandard, apparently. Merged it into the other file.

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

Successfully merging this pull request may close these issues.

3 participants