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

API Proposal: Obsolete XmlSecureResolver #73636

Closed
GrabYourPitchforks opened this issue Aug 9, 2022 · 2 comments · Fixed by #73676
Closed

API Proposal: Obsolete XmlSecureResolver #73636

GrabYourPitchforks opened this issue Aug 9, 2022 · 2 comments · Fixed by #73676
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Xml breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

API Proposal: Obsolete XmlSecureResolver

The class System.Xml.XmlSecureResolver does not work correctly in .NET. It relies on Code Access Security (CAS), but the CAS infrastructure was never ported from .NET Framework to .NET Core. The end result is that the type no-ops, allowing resolution which should be blocked.

We investigated making a behavioral change to the class to add a naïve restriction mechanism. However, this is complicated by the fact that the XmlSecureResolver type does not itself perform resolution. Instead, it wraps an inner XmlResolver, providing a sandbox controlling which actions are allowed by the inner resolver. Because we don't control the behavior of the inner resolver, we have no way of asking it to honor our policy. Anything we do on top of this would be fragile.

The best course of action is two-fold:

  • Slap [Obsolete] on the class (possible source breaking change)
  • Make its GetEntity method unconditionally throw an exception at runtime (behavioral breaking change)
// System.Xml.ReaderWriter.dll
namespace System.Xml
{
    [Obsolete(/* ... SYSLIB0047 ... */)]
    public class XmlSecureResolver : XmlResolver
    {
        public XmlSecureResolver(XmlResolver resolver, string? securityUrl); // existing ctor, no change

        public override object? GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn)
        {
            throw new XmlException(/* ... */);
        }
    }
}

Further discussion

We really need the ability to retroactively obsolete this across netstandard2.0 and downlevel runtimes. Otherwise devs won't see these obsoletions and won't know not to call it. I'm updating the documentation for the type to reflect what's discussed here, but doc updates only go so far.

New CodeQL rules have been created, flagging calls to this type across all netstandard and netcoreapp targets. I expect those rules to make their way into the public drops eventually.

@GrabYourPitchforks GrabYourPitchforks added area-System.Xml blocking Marks issues that we want to fast track in order to unblock other important work breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Aug 9, 2022
@GrabYourPitchforks GrabYourPitchforks added this to the 7.0.0 milestone Aug 9, 2022
@GrabYourPitchforks GrabYourPitchforks self-assigned this Aug 9, 2022
@ghost
Copy link

ghost commented Aug 9, 2022

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

API Proposal: Obsolete XmlSecureResolver

The class System.Xml.XmlSecureResolver does not work correctly in .NET. It relies on Code Access Security (CAS), but the CAS infrastructure was never ported from .NET Framework to .NET Core. The end result is that the type no-ops, allowing resolution which should be blocked.

We investigated making a behavioral change to the class to add a naïve restriction mechanism. However, this is complicated by the fact that the XmlSecureResolver type does not itself perform resolution. Instead, it wraps an inner XmlResolver, providing a sandbox controlling which actions are allowed by the inner resolver. Because we don't control the behavior of the inner resolver, we have no way of asking it to honor our policy. Anything we do on top of this would be fragile.

The best course of action is two-fold:

  • Slap [Obsolete] on the class (possible source breaking change)
  • Make its GetEntity method unconditionally throw an exception at runtime (behavioral breaking change)
// System.Xml.ReaderWriter.dll
namespace System.Xml
{
    [Obsolete(/* ... SYSLIB0047 ... */)]
    public class XmlSecureResolver : XmlResolver
    {
        public XmlSecureResolver(XmlResolver resolver, string? securityUrl); // existing ctor, no change

        public override object? GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn)
        {
            throw new XmlException(/* ... */);
        }
    }
}

Further discussion

We really need the ability to retroactively obsolete this across netstandard2.0 and downlevel runtimes. Otherwise devs won't see these obsoletions and won't know not to call it. I'm updating the documentation for the type to reflect what's discussed here, but doc updates only go so far.

New CodeQL rules have been created, flagging calls to this type across all netstandard and netcoreapp targets. I expect those rules to make their way into the public drops eventually.

Author: GrabYourPitchforks
Assignees: GrabYourPitchforks
Labels:

area-System.Xml, blocking, breaking-change

Milestone: 7.0.0

@bartonjs bartonjs added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Aug 9, 2022
@bartonjs
Copy link
Member

bartonjs commented Aug 9, 2022

Video

  • Looks good as proposed
// System.Xml.ReaderWriter.dll
namespace System.Xml
{
    [Obsolete(/* ... SYSLIB0047 ... */)]
    public class XmlSecureResolver : XmlResolver
    {
        public XmlSecureResolver(XmlResolver resolver, string? securityUrl); // existing ctor, no change

        public override object? GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn)
        {
            throw new XmlException(/* ... */);
        }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 9, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 10, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Xml breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants