Remove most CAS and Security transparency related attributes by danmoseley · Pull Request #15987 · dotnet/corefx

danmoseley · 2017-02-09T07:02:11Z

Fixes https://github.com/dotnet/corefx/issues/12592

Remove CAS and Security Transparency attributes, and related message suppressions and pragma disable 618's with the exception of (1) no changes to Compression and Immutable and (2) no changes to SecurityCritical in refs (only removed some SecurityPermission's)

Are there any other OOBs that I should exclude? System.Net.Http was mentioned in #14383, should I reverse the couple changes there?

@jkotas @stephentoub @JeremyKuhne @davidsh

…ion, PermissionSet, ReflectionPermission, SecurityPermission, HostProtection

jkotas · 2017-02-09T07:19:46Z

Rollback src/System.Diagnostics.Tracing/src/... as usual...

danmoseley · 2017-02-09T07:21:11Z

Oh, that's frozen in corefx also? Sure...

davidsh · 2017-02-09T15:12:16Z

Are there any other OOBs that I should exclude? System.Net.Http was mentioned in #14383, should I reverse the couple changes there?

You need to exclude System.Net.Http and all sources that it brings in from src/Common like tracing (DiagnosticSource, EventSource) and any other common files. Otherwise, we will break CAS on the 'net46' build of System.Net.Http.

And to verify your changes, you should run the 'secannotate' tool against the 'net46' binary of System.Net.Http. It needs to continue to show 0 errors. @morganbr can advise.

davidsh · 2017-02-09T15:26:35Z

cc: @karelz

danmoseley · 2017-02-09T15:30:43Z

I reverted all changes to files which are compiled into S.N.Http. That includes NetEventSource.Common.cs.

Is it acceptable to put #if NET46 around those? eg

#if NET46    
    [SecuritySafeCritical]
#endif

davidsh · 2017-02-09T15:36:50Z

Is it acceptable to put #if NET46 around those? eg

Yes. The current source code already uses that convention in places.

davidsh · 2017-02-09T15:38:53Z

Although for files like HttpClientHandler.Net46.cs, you shouldn't need any 'if NET46' because the whole file is only compiled if it is building NET46.

davidsh · 2017-02-09T18:20:14Z

                return _proxy;
            }

+#if NET46


nit: I think it is unnecessary to put any if-def wrappers in this file. This file is already only compiled for the NET46 build.

I decided to do it anyway so that if and when someone makes a similar pass again (as we paste in more reference source, more are brought in) it's quite clear they don't need to delete this one.

danmoseley · 2017-02-09T18:25:33Z

@morganbr can you advise about the security check tool mentioned above.

[edit - I found it -- I"ll run it]

morganbr · 2017-02-09T18:19:38Z

    internal static partial class WebSocket
    {
        [DllImport(Libraries.WebSocket)]
-        [SuppressUnmanagedCodeSecurity]


If this gets used in OOBs, it can affect correctness or performance.

We want to leave all attributes in all Common code? Or just those used in OOBs (which was my goal)?

I don't know which ones get used in OOBs. We only need attributes in files that get used in OOBs (and perhaps ones that would get used in the future).

morganbr · 2017-02-09T18:21:44Z

    public sealed partial class ProjectData
    {
        internal ProjectData() { }
-        [System.Security.SecuritySafeCriticalAttribute]


I believe SafeCritical is important in contracts as well

There is no difference between SafeCritical and transparent (ie no annotation) in the contract.

In other words, changing from SafeCritical to transparent or vice versa, is not a contract breaking change.

There are a couple differences related to inheritance documented at https://msdn.microsoft.com/en-us/library/dd233102(v=vs.110).aspx

Yes, but those only matter at runtime. They do not matter at compile time where the contracts are consumed.

morganbr · 2017-02-09T18:22:43Z

 using System.Reflection;
 using System.Collections;

-[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2112:SecuredTypesShouldNotExposeFields", Scope = "type", Target = "System.ComponentModel.AttributeCollection")]


Please don't remove FXCop suppressions -- those affect our ability to sign off on the code.

I removed this because it relates to LinkDemands, which is CAS. Does it have another purpose?

A public or protected type contains public fields and is secured by a Link Demands.

The tooling we use for security signoffs doesn't know that we don't use CAS. Suppressions help us weed out the noise.

But it's triggered by the presence of a link demand, right? Which we don't have. It doesn't just trigger on all public or protected type contains public fields

morganbr · 2017-02-09T18:24:12Z

 namespace System.Configuration
 {
-    // obsolete
-    [ComVisible(false)]


ComVisible is used for interop, not security. Are these intended to be in this change?

Yes, per Jan's comments in https://github.com/dotnet/corefx/issues/12592

danmoseley · 2017-02-09T18:44:55Z

@ericstj , @joperezr pointed out there are more than these 3 oobs. In my naive grep, many apply to desktop:

C:\git\corefx\src>for /f %i in ('dir /s/b *pkgproj') do @(echo %i & @type %i > out & @\t\grep -i "supportedframework.*net4" out)
C:\git\corefx\src\Microsoft.CSharp\pkg\Microsoft.CSharp.pkgproj
      <SupportedFramework>net463;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\Microsoft.VisualBasic\pkg\Microsoft.VisualBasic.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\Microsoft.Win32.Registry\pkg\Microsoft.Win32.Registry.pkgproj
      <SupportedFramework>net463;netcoreapp2.0</SupportedFramework>
C:\git\corefx\src\Microsoft.Win32.Registry.AccessControl\pkg\Microsoft.Win32.Registry.AccessControl.pkgproj
      <SupportedFramework>net46;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\Native\pkg\runtime.native.System.Data.SqlClient.sni\runtime.native.System.Data.SqlClient.sni.pkgproj
C:\git\corefx\src\Native\pkg\runtime.native.System.Data.SqlClient.sni\win\runtime.native.System.Data.SqlClient.sni.pkgproj
C:\git\corefx\src\System.Buffers\pkg\System.Buffers.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.CodeDom\pkg\System.CodeDom.pkgproj
      <SupportedFramework>net461;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Collections.Immutable\pkg\System.Collections.Immutable.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.ComponentModel.Annotations\pkg\System.ComponentModel.Annotations.pkgproj
      <SupportedFramework>net463;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Composition\pkg\System.Composition.pkgproj
C:\git\corefx\src\System.Composition.AttributedModel\pkg\System.Composition.AttributedModel.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Composition.Convention\pkg\System.Composition.Convention.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Composition.Hosting\pkg\System.Composition.Hosting.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Composition.Runtime\pkg\System.Composition.Runtime.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Composition.TypedParts\pkg\System.Composition.TypedParts.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Configuration.ConfigurationManager\pkg\System.Configuration.ConfigurationManager.pkgproj
      <SupportedFramework>netcoreapp2.0;net461;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Data.SqlClient\pkg\System.Data.SqlClient.pkgproj
      <SupportedFramework>net463;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Diagnostics.DiagnosticSource\pkg\System.Diagnostics.DiagnosticSource.pkgproj
      <SupportedFramework>net46;net45;netcore45;netcoreapp1.0;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.DirectoryServices\pkg\System.DirectoryServices.pkgproj
C:\git\corefx\src\System.DirectoryServices.AccountManagement\pkg\System.DirectoryServices.AccountManagement.pkgproj
C:\git\corefx\src\System.DirectoryServices.Protocols\pkg\System.DirectoryServices.Protocols.pkgproj
C:\git\corefx\src\System.IO.Compression\pkg\System.IO.Compression.pkgproj
      <SupportedFramework>netcoreapp2.0;net463;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.IO.FileSystem.AccessControl\pkg\System.IO.FileSystem.AccessControl.pkgproj
      <SupportedFramework>net46;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\System.IO.Packaging\pkg\System.IO.Packaging.pkgproj
      <SupportedFramework>net46;netcore50;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.IO.Pipes.AccessControl\pkg\System.IO.Pipes.AccessControl.pkgproj
      <SupportedFramework>net46;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\System.IO.Ports\pkg\System.IO.Ports.pkgproj
      <SupportedFramework>net463;netcoreapp2.0</SupportedFramework>
C:\git\corefx\src\System.Json\pkg\System.Json.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Memory\pkg\System.Memory.pkgproj
      <SupportedFramework>net45;netcore45;wp8;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Net.Http\pkg\System.Net.Http.pkgproj
      <SupportedFramework>net46</SupportedFramework>
C:\git\corefx\src\System.Net.Http.Rtc\pkg\System.Net.Http.Rtc.pkgproj
C:\git\corefx\src\System.Net.Http.WinHttpHandler\pkg\System.Net.Http.WinHttpHandler.pkgproj
      <SupportedFramework>net46;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\System.Numerics.Vectors.WindowsRuntime\pkg\System.Numerics.Vectors.WindowsRuntime.pkgproj
C:\git\corefx\src\System.Reflection.Context\pkg\System.Reflection.Context.pkgproj
      <SupportedFramework>net45;netcore45;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Reflection.DispatchProxy\pkg\System.Reflection.DispatchProxy.pkgproj
      <SupportedFramework>net46;netcore50;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Reflection.Metadata\pkg\System.Reflection.Metadata.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Runtime.CompilerServices.Unsafe\pkg\System.Runtime.CompilerServices.Unsafe.pkgproj
      <SupportedFramework>net45;netcore45;wp8;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Runtime.InteropServices.RuntimeInformation\pkg\System.Runtime.InteropServices.RuntimeInformation.pkgproj
      <SupportedFramework>net45;netcore45;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Runtime.WindowsRuntime\pkg\System.Runtime.WindowsRuntime.pkgproj
C:\git\corefx\src\System.Runtime.WindowsRuntime.UI.Xaml\pkg\System.Runtime.WindowsRuntime.UI.Xaml.pkgproj
C:\git\corefx\src\System.Security.AccessControl\pkg\System.Security.AccessControl.pkgproj
      <SupportedFramework>net463;netcoreapp2.0</SupportedFramework>
C:\git\corefx\src\System.Security.Cryptography.OpenSsl\pkg\System.Security.Cryptography.OpenSsl.pkgproj
      <SupportedFramework>net461;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Security.Cryptography.Pkcs\pkg\System.Security.Cryptography.Pkcs.pkgproj
      <SupportedFramework>net46;netcore50;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\System.Security.Cryptography.ProtectedData\pkg\System.Security.Cryptography.ProtectedData.pkgproj
      <SupportedFramework>net46;netcore50;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Security.Cryptography.Xml\pkg\System.Security.Cryptography.Xml.pkgproj
      <SupportedFramework>net462;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Security.Permissions\pkg\System.Security.Permissions.pkgproj
      <SupportedFramework>netcoreapp2.0;net461;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Security.Principal.Windows\pkg\System.Security.Principal.Windows.pkgproj
      <SupportedFramework>net463;netcoreapp2.0</SupportedFramework>
C:\git\corefx\src\System.ServiceProcess.ServiceController\pkg\System.ServiceProcess.ServiceController.pkgproj
      <SupportedFramework>net463;netcoreapp2.0</SupportedFramework>
C:\git\corefx\src\System.Text.Encoding.CodePages\pkg\System.Text.Encoding.CodePages.pkgproj
      <SupportedFramework>net461;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Text.Encodings.Web\pkg\System.Text.Encodings.Web.pkgproj
      <SupportedFramework>net45;netcore45;wp8;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Threading.AccessControl\pkg\System.Threading.AccessControl.pkgproj
      <SupportedFramework>net46;netcoreapp1.0</SupportedFramework>
C:\git\corefx\src\System.Threading.Tasks.Dataflow\pkg\System.Threading.Tasks.Dataflow.pkgproj
      <SupportedFramework>net45;netcore45;wp8;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Threading.Tasks.Extensions\pkg\System.Threading.Tasks.Extensions.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wpa81;wp8;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.Transactions\pkg\System.Transactions.pkgproj
      <SupportedFramework>net461;netcoreapp2.0;$(AllXamarinFrameworks)</SupportedFramework>
C:\git\corefx\src\System.ValueTuple\pkg\System.ValueTuple.pkgproj
      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>

Will we actually support these on desktop? If not, which? @karelz

jkotas · 2017-02-09T19:15:49Z

In my naive grep, many apply to desktop:

These are OOB packages. Most of them do not actually have any actual corefx code in them for desktop - they just contain forwarders for desktop.

danmoseley · 2017-02-09T20:45:16Z

@weshaggard for above question about OOB.s

davidsh · 2017-02-09T21:00:34Z

@danmosemsft

This is what a successful (no errors) secannotate tool shows when there are no errors in CAS attributes for System.Net.Http for the 'net46' binary.

danmoseley · 2017-02-09T21:09:42Z

OK, well neither the net46 nor netfx flavors (not sure if both are relevant) of S.N.Http have any errors when I do that on the latest commit.

danmoseley · 2017-02-09T21:31:25Z

OK so it seems what I need to do is examine anything that builds for net46/netfx to determine what is not a pure runtime facade (ie actually has types in) and #if NETFX the attributes in those.

Apparently @ericstj is cleaning out some of these desktop facades that CoreFX builds so I'll sit on this PR until that is done and then rebase and do the analysis above.

morganbr · 2017-02-09T23:11:50Z

@danmosemsft that sounds pretty good. You can confirm by running secannotate over whatever does build for net46/netfx.

danmoseley · 2017-02-16T00:06:47Z

@ericstj what is the issue # for the work you are doing that this depends on (to clear up the pkgprojs wedon't need)

ericstj · 2017-02-16T16:28:25Z

@danmosemsft Anything we need for packaging is currently committed and builds with the AllConfigurations build. Also pkgprojs have been cleaned up for some time. All remaining ones we expect to ship. The PR that enables build of these pkgrpojs is currently pending: #16191

danmoseley · 2017-02-16T17:10:31Z

OK, so I should avoid touching any security annotations on any contract with a pkgproj - right?

jkotas · 2017-02-16T18:06:47Z

avoid touching any security annotations on any contract with a pkgproj - right?

You will leave most of the cruft in with this filter. For example, the security annotations can be deleted under \corefx\src\System.DirectoryServices.* - because of the implementation is ,NET Core specific - even though it has pkgproj files.

I believe https://github.com/dotnet/corefx/issues/12592#issuecomment-253971384 is still accurate way how to identify where it is fine to clean it up.

danmoseley · 2017-03-02T00:27:24Z

I just won't have time to do this anytime soon unfortunately. Maybe I or someone else can reheat this in future.

danmoseley added 8 commits February 8, 2017 21:02

Remove ComVisible (except for reflection tests)

40f46c9

Remove SecurityCritical except on OOB

b487306

Remove SecuritySafeCritical except on OOB

8f553b4

Remove CAS attributes DirectoryServicesPermission, EnvironmentPermiss…

ea349e3

…ion, PermissionSet, ReflectionPermission, SecurityPermission, HostProtection

Remove SuppressUnmanagedCodeSecurity attribute

c8bac66

Remove suppress-messages relating to CAS

f196430

Remove CA2112

62502d2

Remove CA2113

1bba6fb

dnfclas added the cla-already-signed label Feb 9, 2017

Revert Tracing

f59ffdc

Revert System.Net.Http

44eca2f

Revert parts of Common that S.N.Http uses

0dde1cc

karelz added the area-Meta label Feb 9, 2017

karelz assigned danmoseley Feb 9, 2017

Wrap cases in System.Net.Http and related with #if NET46

2c82bcb

davidsh reviewed Feb 9, 2017

View reviewed changes

morganbr suggested changes Feb 9, 2017

View reviewed changes

danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 9, 2017

danmoseley closed this Mar 2, 2017

karelz modified the milestone: 2.0.0 Mar 7, 2017

filipnavara mentioned this pull request Jul 19, 2018

System.IO.Pipes from CoreFX mono/mono#9573

Merged

Conversation

danmoseley commented Feb 9, 2017

Uh oh!

jkotas commented Feb 9, 2017

Uh oh!

danmoseley commented Feb 9, 2017

Uh oh!

davidsh commented Feb 9, 2017

Uh oh!

davidsh commented Feb 9, 2017

Uh oh!

danmoseley commented Feb 9, 2017

Uh oh!

davidsh commented Feb 9, 2017

Uh oh!

davidsh commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Feb 9, 2017

Uh oh!

jkotas commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Feb 9, 2017

Uh oh!

davidsh commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Feb 9, 2017

Uh oh!

danmoseley commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganbr commented Feb 9, 2017

Uh oh!

danmoseley commented Feb 16, 2017

Uh oh!

ericstj commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Feb 16, 2017

Uh oh!

jkotas commented Feb 16, 2017

Uh oh!

danmoseley commented Mar 2, 2017

Uh oh!

danmoseley commented Feb 9, 2017 •

edited

Loading

jkotas commented Feb 9, 2017 •

edited

Loading

davidsh commented Feb 9, 2017 •

edited

Loading

danmoseley commented Feb 9, 2017 •

edited

Loading

ericstj commented Feb 16, 2017 •

edited

Loading