fix: block behavior gadgets in loose XAML#3048
Open
lhx077 wants to merge 1 commit into
Open
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
Reviewer's Guide为图标加载添加集中式的“松散 XAML”安全验证，并加强现有的 XAML 清洗器，以阻止行为 gadget 以及可启动进程的类型/程序集。 IconManager 中新的“松散 XAML”验证时序图sequenceDiagram
participant Caller
participant IconManager
participant ValidateSafeLooseXaml
participant ValidateLooseXamlType
participant ValidateLooseXamlValue
participant WpfXamlReader
Caller->>IconManager: TryLoadIconFromXaml(xamlString)
IconManager->>ValidateSafeLooseXaml: ValidateSafeLooseXaml(xamlString)
loop XamlXmlReader.Read
ValidateSafeLooseXaml-->>ValidateSafeLooseXaml: reader.Read()
alt reader.Type.UnderlyingType exists
ValidateSafeLooseXaml->>ValidateLooseXamlType: ValidateLooseXamlType(type)
end
alt reader.Member exists
alt member.Name in DisallowedLooseXamlMembers
ValidateSafeLooseXaml-->>Caller: throw UnauthorizedAccessException
Note over Caller: return
end
alt member.DeclaringType.UnderlyingType exists
ValidateSafeLooseXaml->>ValidateLooseXamlType: ValidateLooseXamlType(declaringType)
end
end
alt reader.Value is string
ValidateSafeLooseXaml->>ValidateLooseXamlValue: ValidateLooseXamlValue(value)
alt value or type matches disallowed
ValidateLooseXamlType-->>Caller: throw UnauthorizedAccessException
Note over Caller: return
end
end
end
IconManager->>WpfXamlReader: Parse(xamlString)
WpfXamlReader-->>IconManager: UIElement
IconManager-->>Caller: success
文件级变更
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来：
Getting HelpOriginal review guide in EnglishReviewer's GuideAdds centralized loose-XAML safety validation for icon loading and tightens the existing XAML sanitizer to block behavior gadgets and process-launching types/assemblies. Sequence diagram for new loose-XAML validation in IconManagersequenceDiagram
participant Caller
participant IconManager
participant ValidateSafeLooseXaml
participant ValidateLooseXamlType
participant ValidateLooseXamlValue
participant WpfXamlReader
Caller->>IconManager: TryLoadIconFromXaml(xamlString)
IconManager->>ValidateSafeLooseXaml: ValidateSafeLooseXaml(xamlString)
loop XamlXmlReader.Read
ValidateSafeLooseXaml-->>ValidateSafeLooseXaml: reader.Read()
alt reader.Type.UnderlyingType exists
ValidateSafeLooseXaml->>ValidateLooseXamlType: ValidateLooseXamlType(type)
end
alt reader.Member exists
alt member.Name in DisallowedLooseXamlMembers
ValidateSafeLooseXaml-->>Caller: throw UnauthorizedAccessException
Note over Caller: return
end
alt member.DeclaringType.UnderlyingType exists
ValidateSafeLooseXaml->>ValidateLooseXamlType: ValidateLooseXamlType(declaringType)
end
end
alt reader.Value is string
ValidateSafeLooseXaml->>ValidateLooseXamlValue: ValidateLooseXamlValue(value)
alt value or type matches disallowed
ValidateLooseXamlType-->>Caller: throw UnauthorizedAccessException
Note over Caller: return
end
end
end
IconManager->>WpfXamlReader: Parse(xamlString)
WpfXamlReader-->>IconManager: UIElement
IconManager-->>Caller: success
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层次的反馈：
- 当前松散 XAML 的 denylist 逻辑（类型、成员、程序集）在
IconManager和ModBase中都有一份，现在是重复的；可以考虑把它们集中到共享的 helper/常量中，以避免逻辑分叉，并让后续更新更安全。 - 在
ValidateSafeLooseXaml和GetObjectFromXML中，一些字符串比较/Contains 检查依赖默认比较器，或者每次都重新分配new[] { ... }；建议统一使用StringComparison.Ordinal，并把这些数组提升为静态 readonly 字段，这样既能让检查行为更可预测，又能降低开销。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- The loose XAML denylist logic (types, members, assemblies) is now duplicated between `IconManager` and `ModBase`; consider centralizing this into shared helpers/constants to avoid divergence and make future updates safer.
- In both `ValidateSafeLooseXaml` and `GetObjectFromXML`, several string comparisons/Contains checks rely on default comparers or repeated allocation of `new[] { ... }`; using `StringComparison.Ordinal` consistently and hoisting these arrays to static readonly fields would make the checks more predictable and reduce overhead.
## Individual Comments
### Comment 1
<location path="PCL.Core/UI/IconManager.cs" line_range="152-157" />
<code_context>
+ }
+
+ private static void ValidateLooseXamlValue(string value) {
+ if (DisallowedLooseXamlTypes.Any(disallowedType => string.Equals(value, disallowedType.Name, StringComparison.Ordinal))) {
+ throw new UnauthorizedAccessException($"不允许使用 {value} 值。");
+ }
+
+ if (DisallowedLooseXamlAssemblies.Any(disallowedAssembly => value.Contains(disallowedAssembly, StringComparison.Ordinal))) {
+ throw new UnauthorizedAccessException($"不允许引用 {value}。");
+ }
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** The value-based type check may miss dangerous types referenced via fully qualified or markup-extension syntax.
`ValidateLooseXamlValue` compares `value` only to `disallowedType.Name`, so inputs like `System.Diagnostics.Process`, `{x:Type Process}`, or other namespace/markup forms will not be caught. If this check is intended as a safety net for string-only values, consider at least using `EndsWith(disallowedType.Name, StringComparison.Ordinal)` or normalizing common patterns before comparison so dangerous types cannot bypass the filter via qualified or markup syntax.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈来改进后续的 review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The loose XAML denylist logic (types, members, assemblies) is now duplicated between
IconManagerandModBase; consider centralizing this into shared helpers/constants to avoid divergence and make future updates safer. - In both
ValidateSafeLooseXamlandGetObjectFromXML, several string comparisons/Contains checks rely on default comparers or repeated allocation ofnew[] { ... }; usingStringComparison.Ordinalconsistently and hoisting these arrays to static readonly fields would make the checks more predictable and reduce overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The loose XAML denylist logic (types, members, assemblies) is now duplicated between `IconManager` and `ModBase`; consider centralizing this into shared helpers/constants to avoid divergence and make future updates safer.
- In both `ValidateSafeLooseXaml` and `GetObjectFromXML`, several string comparisons/Contains checks rely on default comparers or repeated allocation of `new[] { ... }`; using `StringComparison.Ordinal` consistently and hoisting these arrays to static readonly fields would make the checks more predictable and reduce overhead.
## Individual Comments
### Comment 1
<location path="PCL.Core/UI/IconManager.cs" line_range="152-157" />
<code_context>
+ }
+
+ private static void ValidateLooseXamlValue(string value) {
+ if (DisallowedLooseXamlTypes.Any(disallowedType => string.Equals(value, disallowedType.Name, StringComparison.Ordinal))) {
+ throw new UnauthorizedAccessException($"不允许使用 {value} 值。");
+ }
+
+ if (DisallowedLooseXamlAssemblies.Any(disallowedAssembly => value.Contains(disallowedAssembly, StringComparison.Ordinal))) {
+ throw new UnauthorizedAccessException($"不允许引用 {value}。");
+ }
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** The value-based type check may miss dangerous types referenced via fully qualified or markup-extension syntax.
`ValidateLooseXamlValue` compares `value` only to `disallowedType.Name`, so inputs like `System.Diagnostics.Process`, `{x:Type Process}`, or other namespace/markup forms will not be caught. If this check is intended as a safety net for string-only values, consider at least using `EndsWith(disallowedType.Name, StringComparison.Ordinal)` or normalizing common patterns before comparison so dangerous types cannot bypass the filter via qualified or markup syntax.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+152
to
+157
| if (DisallowedLooseXamlTypes.Any(disallowedType => string.Equals(value, disallowedType.Name, StringComparison.Ordinal))) { | ||
| throw new UnauthorizedAccessException($"不允许使用 {value} 值。"); | ||
| } | ||
|
|
||
| if (DisallowedLooseXamlAssemblies.Any(disallowedAssembly => value.Contains(disallowedAssembly, StringComparison.Ordinal))) { | ||
| throw new UnauthorizedAccessException($"不允许引用 {value}。"); |
There was a problem hiding this comment.
🚨 issue (security): 基于值的类型检查可能会漏掉通过全限定名或标记扩展语法引用的危险类型。
ValidateLooseXamlValue 只把 value 和 disallowedType.Name 做比较，因此像 System.Diagnostics.Process、{x:Type Process} 或其他带命名空间/标记形式的输入不会被拦截。如果这个检查是作为字符串值的安全兜底机制，建议至少使用 EndsWith(disallowedType.Name, StringComparison.Ordinal)，或者在比较前对常见模式做归一化处理，避免危险类型通过限定名或标记语法绕过过滤。
Original comment in English
🚨 issue (security): The value-based type check may miss dangerous types referenced via fully qualified or markup-extension syntax.
ValidateLooseXamlValue compares value only to disallowedType.Name, so inputs like System.Diagnostics.Process, {x:Type Process}, or other namespace/markup forms will not be caught. If this check is intended as a safety net for string-only values, consider at least using EndsWith(disallowedType.Name, StringComparison.Ordinal) or normalizing common patterns before comparison so dangerous types cannot bypass the filter via qualified or markup syntax.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
Microsoft.Xaml.Behaviors.Wpfand aTriggerAction-based animation exposed behavior action types to loose XAML, creating a gadget that can invoke arbitrary methods (e.g.Process.Start) when attacker-controlled XAML is parsed.XamlReader.Parse/XamlReader.Load) had no explicit checks to deny behavior assemblies, process-related types, or dangerous XAML members, so a pre-parse sanitizer is required to prevent remote XAML RCE.Description
ValidateSafeLooseXamltoPCL.Core/UI/IconManager.csand call it before invoking the WPF XAML parser, rejecting disallowed types, members, and assembly references.IconManagernow includesProcessandProcessStartInfo, existing dangerous XAML types (WebBrowser,Frame,ObjectDataProvider, etc.), and assembly references starting withMicrosoft.Xaml.Behaviors, and it blocks members likeCode,FactoryMethod, andStatic.WpfXamlReaderalias for clarity when invoking the WPF parser and centralize loose-XAML checks into re-usable validation helpers.Plain Craft Launcher 2/Modules/Base/ModBase.csto also rejectProcess/ProcessStartInfoand any references to theMicrosoft.Xaml.Behaviorsassembly during theXamlXmlReaderscan.Testing
git diff --checkwhich produced no whitespace or diff-check errors.dotnet build PCL.Core/PCL.Core.csproj -c Debugbut thedotnetSDK was not available in the execution environment so a full build could not be performed.Codex Task
Summary by Sourcery
在图标和启动器的 XML 处理流水线中加强对松散 XAML 的加载校验，以阻止基于行为（behavior）的 gadgets 以及其他危险的类型、成员和程序集。
Bug 修复：
Microsoft.Xaml.Behaviors的引用。增强内容：
Original summary in English
Summary by Sourcery
Harden loose XAML loading in the icon and launcher XML pipelines to block behavior-based gadgets and other dangerous types, members, and assemblies.
Bug Fixes:
Enhancements: