Fix AppLocker to WDAC conversion issues and improve macro handling#517
Fix AppLocker to WDAC conversion issues and improve macro handling#517D3vil0p3r wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR improves the AppLocker → WDAC conversion tooling by fixing several rule-conversion correctness issues (friendly names, hash algorithm selection, path exceptions), enhancing path macro handling, and modernizing the project’s target framework.
Changes:
- Improve rule labeling/correctness: use
Nameconsistently forFriendlyName, fix per-hash algorithm selection, and add per-hashSourceFileNamecontext. - Convert
FilePathRuleexceptions into WDAC-equivalent rules by inverting the parent action, and avoid ID collisions between parent/exception rules. - Improve path macro handling (convertible + user-scoped warnings) and update the project to
net10.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
AppLocker-Policy-Converter/.../cipolicy.cs |
Removes unused generated fields from SiPolicy. |
AppLocker-Policy-Converter/.../Helper.cs |
Fixes rule naming/hash conversion details, adds exception conversion for path rules, improves macro handling, and changes XML deserialization error behavior. |
AppLocker-Policy-Converter/.../AppLocker-Policy-Converter.csproj |
Updates target framework to net10.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
AppLocker-Policy-Converter/app/AppLocker-Policy-Converter/AppLocker-Policy-Converter/Helper.cs:756
- In
MakeValidPathRule, when a macro is found inconvertibleMacrosyou setappLockerPathRule = wdacPathRule, but then execution continues into the "Unknown macro" fallback using the originalmacroParts, and the method returns the wildcard-stripped path. This means the deterministic conversion for%PROGRAMFILES%/%PROGRAMDATA%etc will never actually be returned. Fix by returning the convertedwdacPathRuleimmediately (or recomputemacroPartsand re-run the supported-macro path).
if (convertibleMacros.TryGetValue(macroName, out string wdacMacroReplacement))
{
wdacPathRule = wdacMacroReplacement + macroParts[2];
WarningMessages.Add(String.Format(
"WARNING: AppLocker macro '%{0}%' is not supported in WDAC. " +
"Automatically converted \"{1}\" to \"{2}\".",
macroName, appLockerPathRule, wdacPathRule));
appLockerPathRule = wdacPathRule;
}
// Unknown macro - fall back to wildcard stripping as before
if (String.IsNullOrEmpty(macroParts[0]))
{
if(macroParts[2] == @"\*")
{
// E.g. %UNKNOWNMACRO%\* would result in Path=*\* or just Path="*" which we do not want to create
ErrorMessages.Add(String.Format("ERROR: AppLocker Path Rule \"{0}\" is not a valid WDAC Path Rule.", appLockerPathRule));
return null;
}
wdacPathRule = "*" + macroParts[2];
}
else
{
// Keep only the outside edges
wdacPathRule = macroParts[0] + macroParts[2];
}
WarningMessages.Add(String.Format("WARNING: AppLocker Path Rule \"{0}\" is not a valid WDAC Path Rule. Replacing with the " +
"following Path Rule: \"{1}\"", appLockerPathRule, wdacPathRule));
return wdacPathRule;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
AppLocker-Policy-Converter/app/AppLocker-Policy-Converter/AppLocker-Policy-Converter/Helper.cs:735
SerializePolicytoXMLstill usesStreamWriterwithout ausing/Disposepattern. If serialization throws, the file handle may be left open and the output file can remain locked or partially written. Consider switching this tousing var writer = new StreamWriter(...)(similar to the other deserialization helpers updated in this PR).
// Serialize policy to XML file
XmlSerializer serializer = new XmlSerializer(typeof(SiPolicy));
StreamWriter writer = new StreamWriter(xmlPath);
serializer.Serialize(writer, siPolicy);
writer.Close();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
AppLocker-Policy-Converter/app/AppLocker-Policy-Converter/AppLocker-Policy-Converter/Helper.cs:739
SerializePolicytoXMLmanually closes theStreamWriter, but ifserializer.Serialize(...)throws, the writer won't be disposed and the file handle can remain open. Use ausing/using varforStreamWriterto ensure disposal even on exceptions.
// Serialize policy to XML file
XmlSerializer serializer = new XmlSerializer(typeof(SiPolicy));
StreamWriter writer = new StreamWriter(xmlPath);
serializer.Serialize(writer, siPolicy);
writer.Close();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net5.0</TargetFramework> | ||
| <TargetFramework>net10.0</TargetFramework> |
Bug Fixes
FileHashRule, all hashes in a rule were getting the sameFriendlyName(the rule-levelName). Each hash now gets its own name from AppLocker XMLSourceFileNamefield, making the output policy much easier to read and trace in event logs.algoinConvertFileHashRulewas always read from index[0]of the condition array instead of the current hash in the loop. Harmless in practice since mixed hash types are rare, but wrong nonetheless.FriendlyNamewas set fromDescription, which is optional and often empty in real-world AppLocker exports.Nameis now always used as the primary label.FilePathRuleexceptions on Allow path rules are automatically converted to WDAC Deny path rules, with a warning that WDAC Deny overrides any Allow. Hash-based exceptions and exceptions on Deny path rules cannot be safely auto-converted. They are skipped and an actionable warning is emitted asking the user to handle them manually.DeserializeXMLtoPolicyandDeserializeXMLStringtoPolicywere swallowing exceptions and returning null silently, making it very hard to understand what went wrong during deserialization. They now throw with the original exception attached.Improvements
%PROGRAMFILES%,%PROGRAMFILES(X86)%and%PROGRAMDATA%are now automatically converted to their%OSDRIVE%-based WDAC equivalents instead of falling back to wildcard stripping. Per-user macros like%APPDATA%and%LOCALAPPDATA%correctly emit an actionable warning since they can't be safely mapped (in case someone is using them).