Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Android crypto asserts #61827

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Nov 19, 2021

This fixes three asserts that were started occurring in the native Android cryptographic primitives.

  • One shot hashing now tolerates empty/null input.
  • Hashing and HMAC will now no-op if the append is empty.
  • RSA encryption now tolerates empty/null input.

/cc @AaronRobinsonMSFT.

Closes #61783

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 19, 2021
@ghost
Copy link

ghost commented Nov 19, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes three asserts that were started occurring in the native Android cryptographic primitives.

  • One shot hashing now tolerates empty/null input.
  • Hashing and HMAC will now no-op if the append is empty.
  • RSA encryption now tolerates empty/null input.

/cc @AaronRobinsonMSFT.

Author: vcsjones
Assignees: -
Labels:

area-System.Security, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Nov 19, 2021

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes three asserts that were started occurring in the native Android cryptographic primitives.

  • One shot hashing now tolerates empty/null input.
  • Hashing and HMAC will now no-op if the append is empty.
  • RSA encryption now tolerates empty/null input.

/cc @AaronRobinsonMSFT.

Author: vcsjones
Assignees: -
Labels:

area-System.Security, os-android, community-contribution

Milestone: -

@MaximLipnin
Copy link
Contributor

@vcsjones thanks for your help, let me run some additional CI lanes to verify the fix

@MaximLipnin
Copy link
Contributor

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

Note, I saw a number of failures in System.Security.Cryptography on Android, most of them because it seems to be trying to load openssl or run RC2 tests. However none of the tests are tripping an assert now.

@steveisok
Copy link
Member

steveisok commented Nov 19, 2021

Note, I saw a number of failures in System.Security.Cryptography on Android, most of them because it seems to be trying to load openssl or run RC2 tests. However none of the tests are tripping an assert now.

Why would it pick the wrong DefaultECDsaProvider ? The logs indicate trying the .Unix one when it should be picking .Android.

@vcsjones
Copy link
Member Author

Why would it pick the wrong DefaultECDsaProvider ? The logs indicate trying the .Unix one when it should be picking .Android.

I'm guessing this happened when .Algorithms was consolidated. The OpenSSL initializer in the cctor is firing for Android. I'm guessing that should not be happening.

11-19 16:31:47.506  3525  3548 I DOTNET  :    at System.Security.Cryptography.Tests.ECKeyFileTests`1[[System.Security.Cryptography.ECDsa, System.Security.Cryptography, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]]..cctor() in /_/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyFileTests.cs:line 30
11-19 16:31:47.506  3525  3548 I DOTNET  : ----- Inner Stack Trace -----
11-19 16:31:47.506  3525  3548 I DOTNET  :    at Interop.Crypto..cctor() in /_/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Initialization.cs:line 18
11-19 16:31:47.506  3525  3548 I DOTNET  : ----- Inner Stack Trace -----
11-19 16:31:47.506  3525  3548 I DOTNET  :    at Interop.CryptoInitializer..cctor() in /_/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Initialization.cs:line 34
11-19 16:31:47.506  3525  3548 I DOTNET  :    Execution time: 0
11-19 16:31:47.507  3525  3548 I DOTNET  : 	[FAIL] System.Security.Cryptography.EcDsa.Tests.ECDsaKeyFileTests.ReadWriteSect163k1Key1ExplicitPkcs8
11-19 16:31:47.507  3525  3548 I DOTNET  : 	[FAIL] System.Security.Cryptography.EcDsa.Tests.ECDsaKeyFileTests.ReadWriteSect163k1Key1ExplicitPkcs8   Test name: System.Security.Cryptography.EcDsa.Tests.ECDsaKeyFileTests.ReadWriteSect163k1Key1ExplicitPkcs8
11-19 16:31:47.507  3525  3548 I DOTNET  :    Assembly:  [System.Security.Cryptography.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]
11-19 16:31:47.507  3525  3548 I DOTNET  :    Exception messages: System.TypeInitializationException : The type initializer for 'System.Security.Cryptography.Tests.ECKeyFileTests`1' threw an exception.
11-19 16:31:47.507  3525  3548 I DOTNET  : ---- System.TypeInitializationException : The type initializer for 'Crypto' threw an exception.
11-19 16:31:47.507  3525  3548 I DOTNET  : -------- System.TypeInitializationException : The type initializer for 'CryptoInitializer' threw an exception.
11-19 16:31:47.507  3525  3548 I DOTNET  : ------------ System.DllNotFoundException : libSystem.Security.Cryptography.Native.OpenSsl   Exception stack traces:    at System.Security.Cryptography.EcDsa.Tests.ECDsaKeyFileTests..ctor()
11-19 16:31:47.507  3525  3548 I DOTNET  :    at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object obj, Span`1 parameters, Boolean wrapExceptions)
11-19 16:31:47.507  3525  3548 I DOTNET  : ----- Inner Stack Trace -----

Let me see if I can fix that up. But I would not hold this PR for that. This PR is at least some progress (assuming I did it correctly 😄)

@bartonjs
Copy link
Member

<ItemGroup Condition="('$(TargetsUnix)' == 'true' or '$(TargetsBrowser)' == 'true') and '$(UseAndroidCrypto)' != 'true'">
<Compile Include="DefaultECDsaProvider.Unix.cs" />
<Compile Include="DefaultECDiffieHellmanProvider.Unix.cs" />
<Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs"
Link="Common\Interop\Unix\Interop.Libraries.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Security.Cryptography.Native\Interop.Initialization.cs"
Link="Common\Interop\Unix\System.Security.Cryptography.Native\Interop.Initialization.cs" />
</ItemGroup>
<ItemGroup Condition="'$(UseAndroidCrypto)' == 'true'">
<Compile Include="$(CommonPath)Interop\Android\Interop.Libraries.cs"
Link="Common\Interop\Android\Interop.Libraries.cs" />
<Compile Include="$(CommonPath)Interop\Android\System.Security.Cryptography.Native.Android\Interop.Initialization.cs"
Link="Common\Interop\Android\System.Security.Cryptography.Native.Android\Interop.Initialization.cs" />
<Compile Include="DefaultECDsaProvider.Android.cs" />
<Compile Include="DefaultECDiffieHellmanProvider.Android.cs" />
</ItemGroup>

Looks good, ja? Well, the properties section doesn't defined UseAndroidCrypto. My bad.

@vcsjones
Copy link
Member Author

That fixed the crypto initialization issues. Tests are still failing because it's trying to run the RC2 tests. I will fix that one next.

@vcsjones
Copy link
Member Author

And now the RC2 tests are fixed, so this is green locally for me. I would appreciate if someone could start the relevant pipelines in CI.

(Previously, the RC2 one-shot tests were being excluded by csproj. They aren't anymore, so they are marked conditional as the other RC2 tests are).

@steveisok
Copy link
Member

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

@bartonjs what do y'all think about having Android tests run automatically if anything changes in src/libraries/System.Security.Cryptography/**/*? Seems like changes in S.S.Cryptography have a higher chance of breaking Android since these are, at the core, a massive p/invoke abstraction.

@bartonjs
Copy link
Member

what do y'all think about having Android tests run automatically if anything changes in src/libraries/System.Security.Cryptography/**/*?

SGTM, (even if it increases the CI duration for crypto-affecting PRs) but I don't know how any of that works. What do you think, @steveisok? (Clearly it didn't occur to me that Android tests weren't run when I made the consolidation)

@steveisok
Copy link
Member

what do y'all think about having Android tests run automatically if anything changes in src/libraries/System.Security.Cryptography/**/*?

SGTM, (even if it increases the CI duration for crypto-affecting PRs) but I don't know how any of that works. What do you think, @steveisok? (Clearly it didn't occur to me that Android tests weren't run when I made the consolidation)

We're going to try and get there as part of #61017. By default, only System.Runtime tests will execute on PR's for mobile legs. I think detecting the libraries you change and only run them is a good middle ground.

@vcsjones
Copy link
Member Author

Clearly it didn't occur to me that Android tests weren't run when I made the consolidation

Same, I've broken the Android build a couple of times now: #59812, #59818, #55699, etc.

I think detecting the libraries you change and only run them is a good middle ground.

Even if we run all Android tests for changes in S.S.Cryptography, I think that is an improvement over the current situation. Ideally yes, only run tests for changed projects, but the nature of S.S.Cryptography is "giant abstraction of p/invoke" so there is a fairly high chance that changes here have an impact on Android's CI.

@steveisok
Copy link
Member

Even if we run all Android tests for changes in S.S.Cryptography, I think that is an improvement over the current situation. Ideally yes, only run tests for changed projects, but the nature of S.S.Cryptography is "giant abstraction of p/invoke" so there is a fairly high chance that changes here have an impact on Android's CI.

Good point. Running them all the time again is something to consider.

BTW - the tests look good. The emulators are red because they sometimes fail S.S.Cryptography tests due to running out of memory. Many attempts were made to figure out why, none have solved the problem thus far.

@bartonjs
Copy link
Member

@steveisok I think you're saying it's good to merge even with the failures that are presented; but I'm not sure enough that I'm willing to push the button.

[20:40:58] info: Running instrumentation class net.dot.MonoRunner took 28.2490973 seconds
[20:40:58] dbug: Exit code: 0
                 Standard Output:
                 INSTRUMENTATION_RESULT: shortMsg=Process crashed.
                 INSTRUMENTATION_CODE: 0
                 
                 
[20:40:58] info: Short Message: 
                 Process crashed.
[20:40:58] fail: No value for 'return-code' provided in instrumentation result.  This may indicate a crashed test (see log)
[20:41:01] dbug: Executing command: '/datadisks/disk1/work/A62008FE/p/microsoft.dotnet.xharness.cli/1.0.0-prerelease.21561.1/runtimes/any/native/adb/linux/adb -s emulator-5554 logcat -d '
[20:41:01] info: Wrote current ADB log to /datadisks/disk1/work/A62008FE/w/B7220A4A/uploads/adb-logcat-net.dot.System.Security.Cryptography.Tests-net.dot.MonoRunner.log
[20:41:04] dbug: Executing command: '/datadisks/disk1/work/A62008FE/p/microsoft.dotnet.xharness.cli/1.0.0-prerelease.21561.1/runtimes/any/native/adb/linux/adb -s emulator-5554 bugreport /datadisks/disk1/work/A62008FE/w/B7220A4A/uploads/adb-bugreport-net.dot.System.Security.Cryptography.Tests.zip'
[20:42:03] info: Wrote ADB bugreport to /datadisks/disk1/work/A62008FE/w/B7220A4A/uploads/adb-bugreport-net.dot.System.Security.Cryptography.Tests.zip
[20:42:03] fail: Non-success instrumentation exit code: 82, expected: 0
[20:42:03] info: Attempting to remove apk 'net.dot.System.Security.Cryptography.Tests': 
[20:42:03] dbug: Executing command: '/datadisks/disk1/work/A62008FE/p/microsoft.dotnet.xharness.cli/1.0.0-prerelease.21561.1/runtimes/any/native/adb/linux/adb -s emulator-5554 uninstall net.dot.System.Security.Cryptography.Tests'
[20:42:03] info: Successfully uninstalled net.dot.System.Security.Cryptography.Tests.
[20:42:03] dbug: Saving diagnostics data to '/datadisks/disk1/work/A62008FE/w/B7220A4A/e/diagnostics.json'
XHarness exit code: 1 (TESTS_FAILED)

doesn't look happy to me... but if that's the OOM failure you mentioned, then OK.

(As of this message, at least, all checks have completed. The Browser/wasm failure looks like some piece of infra failed after some tests, so it's only the runtime-manual checks that I'm not sure about)

@steveisok
Copy link
Member

steveisok commented Nov 19, 2021

In the adb logs, you'll see a bunch of these before the test activity crashes:

11-19 20:38:01.647  1735  1735 E lowmemorykiller: Kill 'com.android.dialer' (2973), uid 10101, oom_adj 250 to free 64092kB
11-19 20:38:01.647  1735  1735 I lowmemorykiller: Reclaimed 64092kB, cache(128148kB) and free(134692kB)-reserved(80816kB) below min(129024kB) for oom_adj 250
11-19 20:38:01.647  1735  1735 I lowmemorykiller: Suppressed 24 failed kill reports
11-19 20:38:01.664  1681  1681 I Zygote  : Process 2973 exited due to signal 9 (Killed)

The failures in the other legs are known issues. I think this is good to go. Especially given the crypto tests pass on android devices (arm/arm64 legs).

@MaximLipnin
Copy link
Contributor

Thanks for looking into this. JFR - There are other OOM-related issue, e.g. #55691. so it's not specific for this PR. Since the initial issue seems to be addressed, it would be good to merge this change.

@MaximLipnin
Copy link
Contributor

Thanks for looking into this. JFR - There are other OOM-related issue, e.g. #55691. so it's not specific for this PR. Since the initial issue seems to be addressed, it would be good to merge this change.

Merging it as it will help to clean up the failures on android lanes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member os-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android](CryptoNative_HmacUpdate): Parameter 'data' must be a valid pointer'
4 participants