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

Fixes issue verifying Windows CSP profiles that contain ADMX policies. #25528

Merged
merged 7 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improving ADMX XML parsing.
  • Loading branch information
getvictor committed Jan 17, 2025
commit 118ad0e7e078cb2fd36802b016e17cf13f5d2a60
32 changes: 8 additions & 24 deletions server/mdm/microsoft/admx/admx.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@
package admx

import (
"bytes"
"encoding/xml"
"errors"
"fmt"
"html"
"regexp"
"slices"
"strings"
)

var cdataRegexp = regexp.MustCompile(`(?s)<!\[CDATA\[(.*?)]]>`)

Check failure on line 23 in server/mdm/microsoft/admx/admx.go

View workflow job for this annotation

GitHub Actions / lint (macos-latest)

var `cdataRegexp` is unused (unused)

func IsADMX(text string) bool {
// We try to unmarshal the string to see if it looks like a valid ADMX policy
Expand All @@ -47,36 +44,23 @@
}

func unmarshal(a string) (admxPolicy, error) {
a, err := convertToXMLString(a)
// We unmarshal into a string to get the CDATA content and decode XML escape characters.
// We wrap the policy in an <admx> tag to ensure it can be unmarshalled by the XML decoder.
var unescaped string
err := xml.Unmarshal([]byte(`<admx>`+a+`</admx>`), &unescaped)
if err != nil {
return admxPolicy{}, fmt.Errorf("converting ADMX policy to XML string: %w", err)
return admxPolicy{}, fmt.Errorf("unmarshalling ADMX policy to string: %w", err)
}
// ADMX policy elements are not case-sensitive. For example: <enabled/> and <Enabled/> are equivalent
// For simplicity, we compare everything in lowercase.
var policy admxPolicy
err = xml.NewDecoder(bytes.NewReader([]byte(a))).Decode(&policy)
err = xml.Unmarshal([]byte(`<admx>`+strings.ToLower(unescaped)+`</admx>`), &policy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't unescaped already include the <admx/> tags? Is it intentional to wrap in those <admx/> again here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unescaped looks like: <enabled/><data id="Item" value="foo"/>

I don't know how to unmarshal it without adding a root XML element.

<admx> is just a random throw-away name. It could be <bozo>

if err != nil {
return admxPolicy{}, fmt.Errorf("unmarshalling ADMX policy: %w", err)
}
return policy, nil
}

func convertToXMLString(a string) (string, error) {
matches := cdataRegexp.FindAllStringSubmatch(a, -1)
if len(matches) > 1 {
return "", errors.New("multiple CDATA matches found in ADMX policy")
}
if len(matches) == 1 && len(matches[0]) > 1 {
// If CDATA is present, we extract the content. Otherwise, we use the original string.
a = matches[0][1]
}
a = html.UnescapeString(a)
// ADMX policy elements are not case-sensitive. For example: <enabled/> and <Enabled/> are equivalent
// For simplicity, we compare everything in lowercase.
a = strings.ToLower(a)
// We wrap the policy in a <policy> tag to ensure it can be unmarshalled by the XML decoder
a = `<policy>` + a + `</policy>`
return a, nil
}

type admxPolicy struct {
Enabled xml.Name `xml:"enabled,omitempty"`
Disabled xml.Name `xml:"disabled,omitempty"`
Expand Down
44 changes: 39 additions & 5 deletions server/mdm/microsoft/admx/admx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ func TestIsADMX(t *testing.T) {
assert.False(t, IsADMX("not an ADMX policy"))
assert.False(t, IsADMX(`<![CDATA[bozo]]>`))
assert.False(t, IsADMX(`<![CDATA[<bozo/>]]>`))
assert.False(t, IsADMX(`<![CDATA[<bozo]]>`))
assert.True(t, IsADMX(`<![CDATA[<enabled/>]]>`))
assert.True(t, IsADMX(`<![CDATA[<disabled/>]]>`))
assert.True(t, IsADMX(`<![CDATA[<data id="id" value="value"/>]]>`))
Expand All @@ -21,6 +22,13 @@ func TestIsADMX(t *testing.T) {
]]>`))
assert.True(t,
IsADMX("&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;"))
assert.True(t, IsADMX(
`&lt;Enabled/&gt;
<![CDATA[<data id="ExecutionPolicy" value="AllSigned"/>]]>
<![CDATA[<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`))
}

func TestEqual(t *testing.T) {
Expand Down Expand Up @@ -69,6 +77,18 @@ func TestEqual(t *testing.T) {
equal: true,
errorContains: "",
},
{
name: "enabled policies with data and nonstandard format",
a: `&lt;Enabled/&gt;
<![CDATA[<data id="ExecutionPolicy" value="AllSigned"/>]]>
<![CDATA[<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: true,
errorContains: "",
},
{
name: "disabled policies with data",
a: `<![CDATA[<disabled/>
Expand All @@ -79,18 +99,32 @@ func TestEqual(t *testing.T) {
errorContains: "",
},
{
name: "unparsable policy",
name: "unparsable policy a 1",
a: "<bozo",
b: "",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unparsable policy a 2",
a: "&lt;bozo",
b: "",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unparsable policy b 1",
a: "",
b: "<bozo",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "multiple CDATA",
a: "<![CDATA[<enabled/>]]><![CDATA[<bozo>]]>",
b: "",
name: "unparsable policy b 2",
a: "",
b: "&lt;bozo",
equal: false,
errorContains: "multiple CDATA matches found",
errorContains: "unmarshalling ADMX policy",
},
{
name: "unequal policies with missing enable",
Expand Down
Loading