Skip to content

Removed declarative CAS permission attributes and comments (Part 2) #1035

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 24, 2019
Merged

Removed declarative CAS permission attributes and comments (Part 2) #1035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3121717
removed attributes
ojhad Jun 20, 2019
4d95964
removed attributes
ojhad Jun 20, 2019
ff493a5
removed securitynote comments
ojhad Jun 20, 2019
f7a57f6
removed attributes
ojhad Jun 20, 2019
2ce2af5
removed attributes
ojhad Jun 20, 2019
ac251d3
removed attributes
ojhad Jun 20, 2019
8e1cc5c
addressed comments
ojhad Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 0 additions & 30 deletions src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/Common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,6 @@ private ref class Util sealed

public:

/// <SecurityNote>
/// Critical - Call security critical method ThrowExceptionForHR().
/// - Asserts unmanaged code permissions to call ThrowExceptionForHR.
/// Safe - We are using ThrowExceptionForHR() in a safe way
// by ignoring the IErrorInfo of the current thread.
/// </SecurityNote>
[SecuritySafeCritical]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
__declspec(noinline) void static ConvertHresultToException(HRESULT hr)
{

Expand Down Expand Up @@ -59,12 +51,6 @@ private ref class Util sealed
}
}

/// <SecurityNote>
/// Critical - Asserts unmanaged code permissions to call ThrowExceptionForHR.
/// - Exposes a pointer to the contents of a managed string.
/// </SecurityNote>
[SecurityCritical]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
__declspec(noinline) const cli::interior_ptr<const System::Char> static GetPtrToStringChars(System::String^ s)
{
return CriticalPtrToStringChars(s);
Expand All @@ -75,12 +61,6 @@ private ref class Util sealed
/// The implementation of this method is taken from this msdn article:
/// http://msdn.microsoft.com/en-us/library/wb8scw8f(VS.100).aspx
/// </summary>
/// <SecurityNote>
/// Critical - Asserts unmanaged code permissions.
/// Safe - Does not expose critical data.
/// </SecurityNote>
[SecuritySafeCritical]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
__declspec(noinline) static _GUID ToGUID( System::Guid& guid )
{
array<System::Byte>^ guidData = guid.ToByteArray();
Expand All @@ -98,10 +78,6 @@ private ref class Util sealed
/// The IErrorInfo is taken into account in a call to GetExceptionForHR(HRESULT), see MSDN for more details.
/// </summary>

/// <SecurityNote>
/// Critical - Calls critical IsFullTrustCaller and Marshal::GetExceptionForHR
/// </SecurityNote>
[SecurityCritical]
void static SanitizeAndThrowIfKnownException(HRESULT hr)
{
if (hr == COR_E_INVALIDOPERATION)
Expand All @@ -124,12 +100,6 @@ private ref class Util sealed
/// <summary>
/// Checks if the caller is in full trust mode.
/// </summary>
/// <SecurityNote>
/// Critical - Performs a demand. Transparent methods should not be responsible
/// for verifying the security of an operation, and therefore should not demand permissions.
/// Safe - It is safe to perform a demand.
/// </SecurityNote>
[SecurityCritical]
static bool IsFullTrustCaller()
{
#ifndef _CLR_NETCORE
Expand Down
28 changes: 0 additions & 28 deletions src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/DWriteInterfaces.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// IMPORTANT: ReadFileFragment() implementations must check whether the requested file fragment
/// is within the file bounds. Otherwise, an error should be returned from ReadFileFragment.
/// </remarks>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT ReadFileFragment(
[Out] const void **fragmentStart,
Expand All @@ -51,10 +47,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// Releases a fragment from a file.
/// </summary>
/// <param name="fragmentContext">The client defined context of a font fragment returned from ReadFileFragment.</param>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
void ReleaseFileFragment(
[In] void *fragmentContext
Expand All @@ -73,10 +65,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// either require complete font file to be loaded (e.g., copying a font file) or need to make
/// decisions based on the value of the file size (e.g., validation against a persisted file size).
/// </remarks>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT GetFileSize(
[Out/*, MarshalAs(UnmanagedType::U8)*/] UINT64 *fileSize
Expand All @@ -92,10 +80,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// Standard HRESULT error code. For resources that don't have a concept of the last modified time, the implementation of
/// GetLastWriteTime should return E_NOTIMPL.
/// </returns>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT GetLastWriteTime(
[Out/*, MarshalAs(UnmanagedType::U8)*/] UINT64 *lastWriteTime
Expand Down Expand Up @@ -125,10 +109,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// <returns>
/// Standard HRESULT error code.
/// </returns>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT CreateStreamFromKey(
[In] void const* fontFileReferenceKey,
Expand Down Expand Up @@ -165,10 +145,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// <returns>
/// Standard HRESULT error code.
/// </returns>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT GetCurrentFontFile(
/*[Out, MarshalAs(UnmanagedType::Interface)]*/ IDWriteFontFile** fontFile
Expand Down Expand Up @@ -198,10 +174,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface { n
/// <returns>
/// Standard HRESULT error code.
/// </returns>
/// <SecurityNote>
/// Critical - receives native pointers as parameters.
/// </SecurityNote>
[SecurityCritical]
[PreserveSig]
HRESULT CreateEnumeratorFromKey(
/*[In, MarshalAs(UnmanagedType::Interface)]*/ IntPtr factory,
Expand Down
1 change: 0 additions & 1 deletion src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/DWriteTypeConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
/// <summary>
/// This class is used to convert data types back anf forth between DWrite and DWriteWrapper.
/// </summary>
[System::Security::SecurityCritical(System::Security::SecurityCriticalScope::Everything)]
private ref class DWriteTypeConverter sealed
{
internal:
Expand Down
79 changes: 0 additions & 79 deletions src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/Factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,10 @@ using namespace System::Threading;

typedef HRESULT (WINAPI *DWRITECREATEFACTORY)(DWRITE_FACTORY_TYPE factoryType, REFIID iid, IUnknown **factory);

/// <SecurityNote>
/// Critical - Returns a pointer to the DWriteCreateFactory method which
/// can be used to access the shared factory.
/// </SecurityNote>
[System::Security::SecurityCritical]
extern void *GetDWriteCreateFactoryFunctionPointer();

namespace MS { namespace Internal { namespace Text { namespace TextInterface
{
/// <SecurityNote>
/// Critical - Calls security critical Factory ctor().
/// </SecurityNote>
[SecurityCritical]
Factory^ Factory::Create(
FactoryType factoryType,
IFontSourceCollectionFactory^ fontSourceCollectionFactory,
Expand All @@ -37,16 +28,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
return gcnew Factory(factoryType, fontSourceCollectionFactory, fontSourceFactory);
}

/// <SecurityNote>
/// Critical - references security critical member '_pFactory'.
/// references security critical method 'MS.Internal.Text.TextInterface.FontFileLoader..ctor(MS.Internal.Text.TextInterface.IFontSourceFactory)'.
/// references security critical method 'MS.Internal.Text.TextInterface.FontCollectionLoader..ctor(MS.Internal.Text.TextInterface.IFontSourceCollectionFactory, MS.Internal.Text.TextInterface.FontFileLoader)'.
/// references security critical method 'System.Runtime.InteropServices.Marshal.GetComInterfaceForObject(System.Object, System.Type)' &
/// 'System.Runtime.InteropServices.Marshal.Release(System.IntPtr)' but this is ok since they are called for objects that this method create.
/// Asserts unmanaged code permissions to call Marshal.*
/// </SecurityNote>
//[SecurityCritical] - tagged in header file
//[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
Factory::Factory(
FactoryType factoryType,
IFontSourceCollectionFactory^ fontSourceCollectionFactory,
Expand Down Expand Up @@ -94,12 +75,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
ConvertHresultToException(hr, "Factory::Factory");
}

/// <SecurityNote>
/// Critical - Calls security critical GetDWriteCreateFactoryFunctionPointer().
/// Assigns security critical member _pFactory.
/// Safe - Does not expose any critical info.
/// </SecurityNote>
[SecuritySafeCritical]
__declspec(noinline) void Factory::Initialize(
FactoryType factoryType
)
Expand All @@ -118,15 +93,7 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
_pFactory = (IDWriteFactory*)factoryTemp;
}

/// <SecurityNote>
/// Critical - Manipulates security critical member _pFactory.
/// - Asserts Unmanaged code permissions to call Marshal.*
/// Safe - Just releases the interface.
/// - Marshal is called with trusted inputs.
/// </SecurityNote>
[SecuritySafeCritical]
[ReliabilityContract(Consistency::WillNotCorruptState, Cer::Success)]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
__declspec(noinline) bool Factory::ReleaseHandle()
{
if (_wpfFontCollectionLoader != nullptr)
Expand Down Expand Up @@ -160,10 +127,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
return true;
}

/// <SecurityNote>
/// Critical - Assumes that the user has permissions to access filePathUri.
/// </SecurityNote>
[SecurityCritical]
__declspec(noinline) FontFile^ Factory::CreateFontFile(
System::Uri^ filePathUri
)
Expand Down Expand Up @@ -192,10 +155,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface

}

/// <SecurityNote>
/// Critical - Calls security critical CreateFontFace.
/// </SecurityNote>
[SecurityCritical]
FontFace^ Factory::CreateFontFace(
System::Uri^ filePathUri,
unsigned int faceIndex
Expand All @@ -208,10 +167,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
);
}

/// <SecurityNote>
/// Critical - Calls security critical CreateFontFile.
/// </SecurityNote>
[SecurityCritical]
FontFace^ Factory::CreateFontFace(
System::Uri^ filePathUri,
unsigned int faceIndex,
Expand Down Expand Up @@ -287,11 +242,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
return GetSystemFontCollection(false);
}

/// <SecurityNote>
/// Critical - Uses security critical _pFactory pointer.
/// Safe - It does not expose the pointer it uses.
/// </SecurityNote>
[SecuritySafeCritical]
__declspec(noinline) FontCollection^ Factory::GetSystemFontCollection(
bool checkForUpdates
)
Expand All @@ -308,19 +258,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
return gcnew FontCollection(dwriteFontCollection);
}

/// <SecurityNote>
/// Critical - The caller of this method should own the verification of
/// the access permissions to the given Uri.
///
/// Other reasons why this method should be critical (but safe)
/// ----------------------------------------------------------
/// - Uses security critical _pFactory pointer. But
/// It does not expose the pointer it uses.
/// - Asserts Unmanaged code permissions to call Marshal.* But
/// Marshal is called with trusted inputs.
/// </SecurityNote>
[SecurityCritical]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
__declspec(noinline) FontCollection^ Factory::GetFontCollection(System::Uri^ uri)
{
System::String^ uriString = uri->AbsoluteUri;
Expand Down Expand Up @@ -350,15 +287,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
return gcnew FontCollection(dwriteFontCollection);
}

/// <SecurityNote>
/// Critical - Receives and returns native pointers.
/// - References security critical method 'System.Runtime.InteropServices.Marshal.GetComInterfaceForObject(System.Object, System.Type)'.
/// - References security critical method 'System.Runtime.InteropServices.Marshal.Release(System.IntPtr)'.
/// - Asserts unmanaged code permissions to call Marshal.* However the call to Marshal is safe
/// because it is called with trusted inputs.
/// </SecurityNote>
[SecurityCritical]
[SecurityPermission(SecurityAction::Assert, UnmanagedCode=true)]
HRESULT Factory::CreateFontFile(
IDWriteFactory* factory,
FontFileLoader^ fontFileLoader,
Expand Down Expand Up @@ -482,13 +410,6 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
_timeStampCache->Clear();
}

/// <SecurityNote>
/// Critical - Uses security critical _pFactory pointer.
/// - Calls security critical TextAnalyzer ctor()
/// Safe - It does not expose the pointer it uses.
/// - TextAnalyzer ctor() is called with a trusted pointer.
/// </SecurityNote>
[SecuritySafeCritical]
__declspec(noinline) TextAnalyzer^ Factory::CreateTextAnalyzer()
{
IDWriteTextAnalyzer* textAnalyzer = NULL;
Expand Down
Loading