Strip out SecurityCritical attributes #14383

JeremyKuhne · 2016-12-09T23:32:59Z

They don't have any function in Core.

Stripped via regex:

^.*\[(System\.Security\.)?Security(Safe)?Critical(Attribute)?\](\s*//.*|\s*)$[\r\n]*

related to dotnet/coreclr#8571

They don't have any function in Core. Stripped via regex: ^.*\[(System\.Security\.)?Security(Safe)?Critical(Attribute)?\](\s*//.*|\s*)$[\r\n]*

justinvp · 2016-12-09T23:49:14Z

src/System.Diagnostics.Tracing/src/System/Diagnostics/Tracing/UnsafeNativeMethods.cs

@@ -252,15 +248,12 @@ internal static class Win32Native
        private const string CoreLocalizationApiSet = "kernel32.dll";
 #endif

-        [System.Security.SecuritySafeCritical]
-        // Gets an error message for a Win32 error code.


Nit: Do we want to keep this comment?

I'll put it back

stephentoub · 2016-12-10T00:26:46Z

There are still some remaining attributes that don't match this regex, e.g.
https://github.com/dotnet/corefx/pull/14383/files?diff=unified#diff-b3d39e42f75b9efe89d21eb385ed61f6L40
Was the goal here to get them all, or just most and we'll strip the rest later?

Also, is any of this code compiled into assemblies meant to run on desktop? I'm wondering if we need to not strip them from certain files/libs because they're still necessary there.

Otherwise, LGTM.

JeremyKuhne · 2016-12-10T00:40:51Z

Was the goal here to get them all, or just most and we'll strip the rest later?

Most. I wanted to validate the build process to begin with. I'll find the stragglers in another change.

I'm wondering if we need to not strip them from certain files/libs because they're still necessary there.

Hmm. I'm trying to envision how that might work/not work and whether or not we care about the NetStandard on limited-trust desktop. I would assume that we're horribly broken if this matters- we're not testing that scenario afaik.

My personal feeling is to make NS on desktop a non-goal. Having to float security attributes through peoples libraries for "full" compatibility sounds like a nightmare to support and having them around adds confusion.

danmoseley · 2016-12-10T01:45:18Z

Immutable and Compression are the tricky ones. @jkotas in https://github.com/dotnet/corefx/issues/12592 described the criteria.

danmoseley · 2016-12-10T01:45:35Z

What about AllowPartiallyTrustedCallers and SecurityTransparent?

JeremyKuhne · 2016-12-10T01:48:40Z

Heh, I didn't notice there was an existing issue. I've taken it and I'll finish this up.

davidsh · 2016-12-10T01:51:14Z

Slightly related ....but stripping out these [SecurityCritical] attributes will make things like fixing #11100 impossible. Because one solution to fixing #11100 is to add back a ton of these attributes for the cross-compile (for .NET Framework build) of System.Net.Http since it is OOB'd on Desktop.

cc: @karelz @ericstj @weshaggard @stephentoub

karelz · 2016-12-10T01:53:52Z

But only for OOB packages. The packages that don't ship as OOB (i.e. won't ever run on Desktop), which is majority, don't need the attributes.
cc: @morganbr

davidsh · 2016-12-10T01:55:24Z

My comment above was more about reacting to this comment in the thread:

My personal feeling is to make NS on desktop a non-goal. Having to float security attributes through peoples libraries for "full" compatibility sounds like a nightmare to support and having them around adds confusion.

karelz · 2016-12-10T01:58:54Z

The OOB should compile (and run) against Desktop targeting pack (whatever the terminology is), so the attributes at least don't leak into non-OOB packages.
That means the security attributes have to be under #ifdef OOB, of course.

JeremyKuhne · 2016-12-10T02:11:59Z

I pulled it back out until there is a full understanding of what exactly should be done.

To be clear- I'm not against having attributes in if they are blocking scenarios people are actually using.

morganbr · 2016-12-10T02:28:46Z

Removing attributes may break these assemblies on desktop in all cases. Even for full trust scenarios, the attributes are enforced by the desktop runtime and cannot be disabled.

karelz · 2016-12-10T02:31:52Z

Assemblies which are shipping in .NET Core only are not meant to run on Desktop runtime.

morganbr · 2016-12-10T02:34:39Z

@karelz Agreed... The #if OOB idea is ok, though since these are no-ops on .NET Core, leaving them in is equivalent. I just meant that if any of these run outside of .NET Core, we still need attributes.

JeremyKuhne · 2016-12-10T02:44:32Z

My problem with leaving the no-ops in is that it is confusing and we have no mechanism for maintaining them.

Do we have a list of what is OOB and what is not?

karelz · 2016-12-10T02:50:47Z

Exactly, the maintenance problem is a huge problem (new APIs won't have the attributes) and the confusion doesn't help either.

@weshaggard @ericstj do we have list of OOBs in CoreFX repo?

weshaggard · 2016-12-10T07:43:33Z

@weshaggard @ericstj do we have list of OOBs in CoreFX repo?

We don't have a list exactly but any of the libraries that have source code for netfx* builds (and not just pure facades) are ones that are interesting. We could pretty easily produce a list based on that logic.

gulbanana · 2016-12-10T17:44:52Z

Presumably anything which is part of/a dependency of NETStandard.Library is likely to end up running on desktop at some point.

karelz · 2016-12-10T18:14:09Z

@gulbanana that's not the plan. Changes from .NET Core will be eventually ported to Desktop ("full" Framework), but the source code won't likely be used on Desktop directly.

karelz · 2016-12-10T18:15:00Z

@weshaggard do you have hint how to 'findstr' all of them? I think I can just look at ImmutableCollections or Metadata and guess it from there - will try it later.

weshaggard · 2016-12-12T16:47:55Z

Unfortunately I don't think findstr is going to be the best tool for this job. It will find things that are directly targeting desktop but not things that are indirectly targeting desktop via netstandard/PCL. @ericstj do you have an cheap cleaver ways to find all these? Perhaps reference all our packages and resolve for desktop?

ericstj · 2016-12-12T17:28:46Z

Package reports will show all this data. They'll list binaries as well as projects and the configuration they were built with.

karelz · 2016-12-12T17:32:53Z

From offline discussion: What is most interesting to me (now) is list of OOB packages which also ship as part of the platform (e.g. Networking OOB).
Immutable and MetadataReader (and similar) are sitting on top of the platform, so I assume we know how to ship them and they are not source of problems.
Can I find such list somewhere?

ericstj · 2016-12-12T17:54:03Z

@karelz as I mentioned, package reports have all this data. Most of our packages ship some dll on some version of desktop. Many replace the inbox facades, only a couple replace inbox implementations. System.Net.Http and System.IO.Compression are the only two in the latter camp. Please have a look at the package reports (just look for reports folder under the bin//packages folder after your build). There is a lot of data here so you should be able to get any sort of info you want out of it by processing in the right way.

karelz · 2016-12-12T18:27:32Z

Thanks @ericstj!
Looking at bin\packages\Debug\reports\System.IO.Compression.json, how do I tell it ship implementation specific to Desktop?
Entries like:

"PackagePath": "runtimes/win/lib/net463/System.IO.Compression.dll"

ericstj · 2016-12-12T18:32:29Z

If there is a dll listed under the desktop section then it ships an implementation that will run on desktop. You can also see what project built this dll:

    ".NETFramework,Version=v4.6.3": {
      "Framework": ".NETFramework,Version=v4.6.3",
      "RuntimeID": "",
      "CompileAssets": [
        {
          "LocalPath": "C:\\src\\corefx\\bin\\Windows_NT.AnyCPU.Debug\\System.IO.Compression\\net463\\System.IO.Compression.dll",
          "PackagePath": "ref/net463/System.IO.Compression.dll",
          "SourceProject": {
            "Project": "C:\\src\\corefx\\src\\System.IO.Compression\\src\\System.IO.Compression.csproj",
            "AdditionalProperties": "TargetGroup=net463;",
            "UndefineProperties": ";OSGroup;TestTFMs;FilterToOSGroup;FilterToTestTFM;DefaultBuildAllTarget;SerializeProjects;BuildAllOSGroups"
          },
          "TargetFramework": ".NETFramework,Version=v4.6.3",
          "Version": "4.2.0.0"
        }
      ],
      "RuntimeAssets": [
        {
          "LocalPath": "C:\\src\\corefx\\bin\\Windows_NT.AnyCPU.Debug\\System.IO.Compression\\net463\\System.IO.Compression.dll",
          "PackagePath": "lib/net463/System.IO.Compression.dll",
          "SourceProject": {
            "Project": "C:\\src\\corefx\\src\\System.IO.Compression\\src\\System.IO.Compression.csproj",
            "AdditionalProperties": "TargetGroup=net463;",
            "UndefineProperties": ";OSGroup;TestTFMs;FilterToOSGroup;FilterToTestTFM;DefaultBuildAllTarget;SerializeProjects;BuildAllOSGroups"
          },
          "TargetFramework": ".NETFramework,Version=v4.6.3",
          "Version": "4.2.0.0"
        }
      ]
    },

Contrast this to an early version of desktop where this library was inbox:


    ".NETFramework,Version=v4.5": {
      "Framework": ".NETFramework,Version=v4.5",
      "RuntimeID": "",
      "CompileAssets": [
        {
          "LocalPath": "C:\\src\\corefx\\Tools\\_._",
          "PackagePath": "ref/net45/_._"
        }
      ],
      "RuntimeAssets": [
        {
          "LocalPath": "C:\\src\\corefx\\Tools\\_._",
          "PackagePath": "lib/net45/_._"
        }
      ]
    },

They don't have any function in Core. Stripped via regex: ^.*\[(System\.Security\.)?Security(Safe)?Critical(Attribute)?\](\s*//.*|\s*)$[\r\n]*

They don't have any function in Core. Stripped via regex: ^.*\[(System\.Security\.)?Security(Safe)?Critical(Attribute)?\](\s*//.*|\s*)$[\r\n]* Commit migrated from dotnet/corefx@1abd60c

Strip out SecurityCritical attributes

1736419

They don't have any function in Core. Stripped via regex: ^.*\[(System\.Security\.)?Security(Safe)?Critical(Attribute)?\](\s*//.*|\s*)$[\r\n]*

JeremyKuhne requested review from stephentoub and jkotas December 9, 2016 23:32

dnfclas added the cla-already-signed label Dec 9, 2016

JeremyKuhne mentioned this pull request Dec 9, 2016

Remove remaining FEATURE_CORECLR defines dotnet/coreclr#8555

Merged

jkotas approved these changes Dec 9, 2016

View reviewed changes

justinvp reviewed Dec 9, 2016

View reviewed changes

JeremyKuhne merged commit 1abd60c into dotnet:master Dec 10, 2016

JeremyKuhne deleted the seccrit branch December 10, 2016 01:48

JeremyKuhne mentioned this pull request Dec 10, 2016

Revert "Strip out SecurityCritical attributes" #14393

Merged

karelz modified the milestone: 1.2.0 Dec 10, 2016

danmoseley mentioned this pull request Feb 9, 2017

Remove most CAS and Security transparency related attributes #15987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip out SecurityCritical attributes #14383

Strip out SecurityCritical attributes #14383

JeremyKuhne commented Dec 9, 2016 •

edited

Loading

justinvp Dec 9, 2016

JeremyKuhne Dec 10, 2016

stephentoub commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

danmoseley commented Dec 10, 2016

danmoseley commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

davidsh commented Dec 10, 2016 •

edited

Loading

karelz commented Dec 10, 2016

davidsh commented Dec 10, 2016

karelz commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

morganbr commented Dec 10, 2016

karelz commented Dec 10, 2016

morganbr commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

karelz commented Dec 10, 2016

weshaggard commented Dec 10, 2016

gulbanana commented Dec 10, 2016

karelz commented Dec 10, 2016

karelz commented Dec 10, 2016

weshaggard commented Dec 12, 2016

ericstj commented Dec 12, 2016

karelz commented Dec 12, 2016

ericstj commented Dec 12, 2016

karelz commented Dec 12, 2016

ericstj commented Dec 12, 2016

Strip out SecurityCritical attributes #14383

Strip out SecurityCritical attributes #14383

Conversation

JeremyKuhne commented Dec 9, 2016 • edited Loading

justinvp Dec 9, 2016

Choose a reason for hiding this comment

JeremyKuhne Dec 10, 2016

Choose a reason for hiding this comment

stephentoub commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

danmoseley commented Dec 10, 2016

danmoseley commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

davidsh commented Dec 10, 2016 • edited Loading

karelz commented Dec 10, 2016

davidsh commented Dec 10, 2016

karelz commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

morganbr commented Dec 10, 2016

karelz commented Dec 10, 2016

morganbr commented Dec 10, 2016

JeremyKuhne commented Dec 10, 2016

karelz commented Dec 10, 2016

weshaggard commented Dec 10, 2016

gulbanana commented Dec 10, 2016

karelz commented Dec 10, 2016

karelz commented Dec 10, 2016

weshaggard commented Dec 12, 2016

ericstj commented Dec 12, 2016

karelz commented Dec 12, 2016

ericstj commented Dec 12, 2016

karelz commented Dec 12, 2016

ericstj commented Dec 12, 2016

JeremyKuhne commented Dec 9, 2016 •

edited

Loading

davidsh commented Dec 10, 2016 •

edited

Loading