Remove SafeReflectionInvoker Part 2 #7485
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5c6c63d to
c2174ce
Compare
|
I rebased to fix the conflicts. |
|
Just putting some context on why this is a purely CAS thing, now a dead code (even in NetFX after at least 4.6):
/// Critical: Used to detect luring attack described in class-level comments.
/// Safe: Gets the information from reflection.
/// The MethodInfo (and MemberInfo) have an InheritanceDemand so derived class
/// spoofing in PT is not an issue.Class-level comments: /// <SecurityNote>
/// All invocation of user-provided reflection objects inside System.Xaml should be routed through this class.
///
/// Invoking a reflection object directly from this class (or any class in System.Xaml)
/// could potentially expose internal types or methods in System.Xaml, because
/// mscorlib sees the invocation as coming from System.Xaml. Instead, we do
/// the invocation from a dynamic assembly created for this purpose. Because
/// the wrapper methods
8000
live in a separate assembly (with no internals) and are
/// marked as security-transparent, the security checks in mscorlib will treat
/// attempts to use internals of System.xaml the same as it treats attempts to
/// use internals of any other assembly; i.e. the caller (that is, the user's
/// application) must have the appropriate permissions.
///
/// This class exposes proxy methods for each of the reflection methods we need.
/// The static constructor creates the dynamic assembly and populates it with
/// the wrapper methods that actually call into reflection code. The proxy
/// methods in this class merely call the corresponding dynamic wrapper methods.
///
/// The dynamic assembly technique turns out to have perf implications - it loads
/// parts of mscorlib and clr that are otherwise unneeded (for Reflection.Emit et al.),
/// and requires JIT compilation of the resulting IL. We don't need the elaborate
/// techinique in full-trust scenarios (a malicious full-trust app can already
/// invoke internals just by calling reflection directly). So in full-trust we
/// use the old code.
/// </SecurityNote>and from /// This class makes the assumption that any internal ShouldSerialize methods in System.Xaml are safe
/// for public invocation. If this becomes untrue, then ShouldSerializeValue needs an IsSystemXamlNonPublic
/// check, just like GetValue and SetValue have. |
|
Hey @ThomasGoulet73, after going through your and @h3xds1nz comments, we are ready to take this PR. Can you resolve the merge conflicts for this PR. |
c2174ce to
5ea19f6
Compare
|
Done. |
dipeshmsft
approved these changes
Dec 18, 2024
|
Thank you @ThomasGoulet73. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #6693
Description
Removes the remaining methods from SafeReflectionInvoker which was reverted in #6693 because of this comment: #6693 (comment)
@dipeshmsft Let me know if/when you have more details about whether or not this is safe to do. My personal opinion is that this is a reasonable change to do because it is not part of the public API.
Customer Impact
See "Risk".
Regression
No.
Testing
Local build + CI.
Risk
Low. There is a risk that an application uses this internal API which could be considered a breaking change that falls in Bucket 4 in the dotnet/runtime documentation.
Microsoft Reviewers: Open in CodeFlow