Obsolete XmlSecureResolver

[GrabYourPitchforks]

Resolves #73636.
Resolves #73637.
Resolves CVE-2022-34716.

Obsoletes the System.Xml.XmlSecureResolver type and introduces a new accelerator XmlResolver.ThrowingResolver for callers who need to forbid external entity resolution.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Resolves #73636.
Resolves #73637.
Resolves CVE-2022-34716.

Obsoletes the System.Xml.XmlSecureResolver type and introduces a new accelerator XmlResolver.ThrowingResolver for callers who need to forbid external entity resolution.

Author:	GrabYourPitchforks
Assignees:	GrabYourPitchforks
Labels:	Security, area-System.Xml, breaking-change
Milestone:	7.0.0

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[campersau]

Make Singelton fields readonly?

[campersau]

Suggested change

	internal static XmlThrowingResolver Singleton = new();
	internal static readonly XmlThrowingResolver Singleton = new();

[campersau]

Suggested change

	internal static XmlThrowingResolver Singleton = new();
	internal static readonly XmlThrowingResolver Singleton = new();

[krwq]

this seems a bit weird, if user explicitly chooses throwing resolver we will still go with XmlUrlResolver?

[GrabYourPitchforks]

Yeah, thought the same. It looks like a bug, but perhaps I was being too deferential to preserving back-compat. Maybe killing that clause is the best option.

[krwq]

yes, I'd honestly go with just:
xmlResolver ??= XmlResolver.ThrowingResolver or xmlResolver ??= new XmlUrlResolver() depending if we expect unsafe inputs in those API

[GrabYourPitchforks]

Looks like that line is meant to override whatever the "default" resolver is. For example, if you call XslCompiledTransform.Load(string), it will eventually hit:

runtime/src/libraries/System.Private.Xml/src/System/Xml/Xslt/XslTransform.cs

Lines 259 to 269 in e71a958

	private static XmlResolver CreateDefaultResolver()
	{
	if (LocalAppContextSwitches.AllowDefaultResolver)
	{
	return new XmlUrlResolver();
	}
	else
	{
	return XmlNullResolver.Singleton;
	}
	}

And the "ignore the null resolver" logic was apparently meant to work around this. This seems completely broken to me, since why bother putting in a secure default if we're just going to ignore it?

[GrabYourPitchforks]

Looks like it was added because the URL to the stylesheet itself (the arg passed to XslCompiledTransform.Load(string)) also goes through the resolver for some reason. The system needs to strip it off temporarily - just while loading the outermost stylesheet - so that resolution can take place.

[jeffhandley]

Has my approval once the build errors are resolved.

[stephentoub]

Other than the test compilation issues, LGTM.

[GrabYourPitchforks]

Note to reviewers (@bartonjs @krwq) - the latest iteration does three things.

1. Clean up build errors.
2. Flow the original resolver through the XSLT loader (see the giant comment in XsltLoader.cs).
3. Include the URI of the banned external entity as part of the error message.

That third point really helped me with debugging the issues here, and if we're concerned that real-world customers might run into this as a regression then the improved error message can help them as well.

(I'll merge once CI is green and there's an additional affirmative review.)

[bartonjs]

Do we care about inside vs outside the task here?

It looks like other implementations are throwing XmlException "outside" the task, so I guess this isn't entirely wrong.

[GrabYourPitchforks]

It's one of those weird edge cases that doesn't cleanly fit into any bucket. If I squint I can kinda claim that it's a usage error, since the caller shouldn't be invoking it in the first place. But ultimately I followed the pattern that the previous implementation threw the exception synchronously rather than wrapped within a task.

[krwq]

Thanks, LGTM