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 security transparency issue #80478

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Jan 11, 2023

Fixes #79749

[UPDATE]: The new fix is to remove the APTCA attribute and avoid running in partial trust.

[ORIGINAL fix that is no longer being applied]
This is a partial revert of #78729. That change inadvertently introduced a security transparency issue because the newly added AsSpan() call is security critical and can't be called in a security transparent scope.

@stephentoub
Copy link
Member

stephentoub commented Jan 11, 2023

Why is AsSpan security critical, and why doesn't this impact every other netstandard2.0 library we have that uses AsSpan? Why is this test running in partial trust yet no other tests seem to be?

@jkotas
Copy link
Member

jkotas commented Jan 11, 2023

We have removed .NET Framework partial trust support long time ago. It is not possible to use any new language features for .NET Framework partial trust. Details are in #24122.

Why does the test from #79749 run in a partial trust sandbox?

@noahfalk
Copy link
Member Author

Why does the test from #79749 run in a partial trust sandbox?

Unknown. It isn't an intentional aspect of the tests for this assembly that it should run under partial trust. I assumed that partial trust was simply a test environment that was occasionally used and I had no desire for these tests to generate spurious failures if they were going to be subjected to that.

why doesn't this impact every other netstandard2.0 library we have that uses AsSpan?

Perhaps because System.Diagnostics.DiagnosticSource has a lingering APTCA attribute and other libraries don't?

I am unaware of what scenario requires S.D.DS to maintain the APTCA attribute so I am fine to remove it. It sounds like that would align this assembly with general approach used by the rest of our libraries?

@stephentoub
Copy link
Member

Perhaps because System.Diagnostics.DiagnosticSource has a lingering APTCA attribute and other libraries don't?

Got it. Yeah, it appears to be the only one left. (We also seem to have one test that sets it, but no other production assemblies.)

so I am fine to remove it

That sounds like the right solution to me.

None of our assemblies support partial trust usage any longer. Removing
the APTCA attribute from S.D.DS so that it follows the pattern.

Fixes dotnet#79749
@noahfalk noahfalk force-pushed the fix_metric_security_exception branch from 15542fa to 080ca72 Compare January 11, 2023 23:11
@jkotas
Copy link
Member

jkotas commented Jan 12, 2023

I am unaware of what scenario requires S.D.DS to maintain the APTCA attribute

FWIW, the APTCA attribute was introduced by dotnet/corefx#17076 .

@jkotas
Copy link
Member

jkotas commented Jan 12, 2023

Could you please also delete ALLOW_PARTIALLY_TRUSTED_CALLERS from src\libraries\Common\src\System\HexConverter.cs?

LGTM otherwise.

@noahfalk
Copy link
Member Author

Sure, got HexConverter.cs as well.

@stephentoub stephentoub marked this pull request as ready for review January 13, 2023 00:30
@stephentoub
Copy link
Member

It'd still be interesting to know why the tests are running in partial trust. @ViktorHofer, do you know?

@jkotas
Copy link
Member

jkotas commented Jan 13, 2023

I do not think that the tests are running in partial trust.

.NET Framework validates consistency of the SecurityCritical/SecurityTransparent attributes in APTCA assemblies even when the code is running in full trust. It explains why the test is failing.

@stephentoub
Copy link
Member

even when the code is running in full trust

Interesting, I didn't know that. Thanks.

@jkotas jkotas merged commit 58df150 into dotnet:main Jan 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants