FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
821 stars 342 forks source link

Replace FhirOperationException.GetObjectData override for compatibility with low-trust environments #737

Closed wmrutten closed 5 years ago

wmrutten commented 5 years ago

@mbaltus reported that the API packages are incompatible with https://dotnetfiddle.net/ (an online C# playground). DotNetFiddle refuses to load the Core package, reporting a security violation on override method FhirOperationException.GetObjectData.

Other libraries have reported a similar issue: https://github.com/axuno/SmartFormat.NET/issues/104

Explanation: Method Exception.GetObjectData is marked with the [SecurityCritical] attribute (in the dotNet runtime). Hl7.Fhir.Core.FhirOperationException overrides this method and also specifies the [SecurityCritical] attribute, as required. However in a low trust environment, all explicit security attribute declarations are ignored. Consequently, the override is no longer valid.

Fortunately, dotNet runtime provides an alternative to overriding GetObjectData that also works in low-trust environments: https://stackoverflow.com/questions/14124874/how-do-i-implement-exception-getobjectdata-in-net-4-in-a-library-assembly-that

I propose that we update the implementation to the alternative approach.

wmrutten commented 5 years ago

Turns out that the System.Exception.SerializeObjectState event is not supported in netcore...

https://github.com/dotnet/coreclr/blob/3b807944d8822b44eb5085d6b95b130b4a91808f/src/System.Private.CoreLib/src/System/Exception.cs#L421

protected event EventHandler<SafeSerializationEventArgs> SerializeObjectState
{
    add { throw new PlatformNotSupportedException(SR.PlatformNotSupported_SecureBinarySerialization); }
    remove { throw new PlatformNotSupportedException(SR.PlatformNotSupported_SecureBinarySerialization); }
}

However, it also turns out that in dotnetcore, the Exception.GetObjectData method is NOT decorated with the SecurityCrititical attribute.

https://github.com/dotnet/coreclr/blob/3b807944d8822b44eb5085d6b95b130b4a91808f/src/System.Private.CoreLib/src/System/Exception.cs#L427

Suggestion:

ewoutkramer commented 5 years ago

Is this not a complete duplicate of #735?

wmrutten commented 5 years ago

Is this not a complete duplicate of #735?

Indeed... Github was experiencing some stability issues yesterday, resulting in duplicate post. I'll close #735.

wmrutten commented 5 years ago

Challenge: it appears that the alternative ISafeSerializationInfo interface is not widely supported...

Conclusion:

So in dotnetcore, we can simply keep using the GetObjectData override, SecurityCritical attribute is not necessary. In dotnetFramework, we can implement ISafeSerializationInfo instead of GetObjectData to support low-trust environment; that might break exception serialization in Orleans, however such server/cloud environments typically use dotnetCore.

ewoutkramer commented 5 years ago

Done. Dotnetfiddle is happy now!