diff --git a/changes/16316-windows-xml-validation b/changes/16316-windows-xml-validation
new file mode 100644
index 0000000000..def14d8ae5
--- /dev/null
+++ b/changes/16316-windows-xml-validation
@@ -0,0 +1,5 @@
+* Improved the validation of Windows profiles to prevent errors when the
+ profiles are delivered to the hosts. If you need to embed a nested XML
+ structure (for example for Wi-Fi profiles) you can either:
+ - Escape the XML
+ - Use a wrapping `` element
diff --git a/server/fleet/windows_mdm.go b/server/fleet/windows_mdm.go
index ed456e3e88..d03feb76af 100644
--- a/server/fleet/windows_mdm.go
+++ b/server/fleet/windows_mdm.go
@@ -5,6 +5,7 @@ import (
"encoding/xml"
"errors"
"fmt"
+ "io"
"strings"
"time"
@@ -46,7 +47,15 @@ type MDMWindowsConfigProfile struct {
// It checks that all top-level elements are and none of the
// elements within are reserved URIs.
//
+// It also performs basic checks for XML well-formedness as defined in the [W3C
+// Recommendation section 2.8][1], as required by the [MS-MDM spec][2].
+//
+// Note that we only need to check for well-formedness, but validation is not required.
+//
// Returns an error if these conditions are not met.
+//
+// [1]: http://www.w3.org/TR/2006/REC-xml-20060816
+// [2]: https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-MDM/%5bMS-MDM%5d.pdf
func (m *MDMWindowsConfigProfile) ValidateUserProvided() error {
if len(bytes.TrimSpace(m.SyncML)) == 0 {
return errors.New("The file should include valid XML.")
@@ -56,44 +65,68 @@ func (m *MDMWindowsConfigProfile) ValidateUserProvided() error {
return fmt.Errorf("Profile name %q is not allowed.", m.Name)
}
- var validator struct {
- SyncBody
- NonProtocolElements []interface{} `xml:",any,omitempty"`
- }
- wrappedProfile := fmt.Sprintf("%s", m.SyncML)
- if err := xml.Unmarshal([]byte(wrappedProfile), &validator); err != nil {
- return fmt.Errorf("The file should include valid XML: %w", err)
- }
+ dec := xml.NewDecoder(bytes.NewReader(m.SyncML))
+ // use strict mode to check for a variety of common mistakes like
+ // unclosed tags, etc.
+ dec.Strict = true
- // might be valid XML, but start with something other than
- if mdm.GetRawProfilePlatform(m.SyncML) != "windows" {
- return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.")
- }
+ // keep track of certain elements to perform Fleet-validations.
+ //
+ // NOTE: since we're only checking for well-formedness
+ // we don't need to validate the required nesting
+ // structure (Target>Item>LocURI) so we don't need to track all the tags.
+ var inReplace bool
+ var inLocURI bool
- if len(validator.Add) != 0 ||
- len(validator.Alert) != 0 ||
- len(validator.Atomic) != 0 ||
- len(validator.Delete) != 0 ||
- len(validator.Exec) != 0 ||
- len(validator.Get) != 0 ||
- len(validator.Results) != 0 ||
- len(validator.Status) != 0 ||
- len(validator.NonProtocolElements) != 0 {
- return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.")
- }
-
- for _, cmd := range validator.Replace {
- for _, item := range cmd.Items {
- // intentionally skipping any further validation if we
- // don't get a target per product decision.
- if item.Target == nil {
- continue
- }
-
- if err := validateFleetProvidedLocURI(*item.Target); err != nil {
- return err
+ for {
+ tok, err := dec.Token()
+ if err != nil {
+ if err != io.EOF {
+ return fmt.Errorf("The file should include valid XML: %w", err)
}
+ // EOF means no more tokens to process
+ break
}
+
+ switch t := tok.(type) {
+ // no processing instructions allowed ()
+ // see #16316 for details
+ case xml.ProcInst:
+ return errors.New("The file should include valid XML: processing instructions are not allowed.")
+
+ case xml.StartElement:
+ switch t.Name.Local {
+ case "Replace":
+ inReplace = true
+ case "LocURI":
+ if !inReplace {
+ return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.")
+ }
+ inLocURI = true
+
+ default:
+ if !inReplace {
+ return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.")
+ }
+ }
+
+ case xml.EndElement:
+ switch t.Name.Local {
+ case "Replace":
+ inReplace = false
+ case "LocURI":
+ inLocURI = false
+ }
+
+ case xml.CharData:
+ if inLocURI {
+ if err := validateFleetProvidedLocURI(string(t)); err != nil {
+ return err
+ }
+ }
+
+ }
+
}
return nil
diff --git a/server/fleet/windows_mdm_test.go b/server/fleet/windows_mdm_test.go
index 5ec06ae7a8..3e407afdd5 100644
--- a/server/fleet/windows_mdm_test.go
+++ b/server/fleet/windows_mdm_test.go
@@ -11,7 +11,7 @@ func TestValidateUserProvided(t *testing.T) {
tests := []struct {
name string
profile MDMWindowsConfigProfile
- wantErr bool
+ wantErr string
}{
{
name: "Valid XML with Replace",
@@ -24,7 +24,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: false,
+ wantErr: "",
},
{
name: "Invalid Platform",
@@ -39,10 +39,10 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
- name: "Invalid XML Structure",
+ name: "Invalid top level element",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
@@ -52,7 +52,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements.",
},
{
name: "Reserved LocURI",
@@ -65,7 +65,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Custom configuration profiles can't include BitLocker settings.",
},
{
name: "Reserved LocURI with implicit ./Device prefix",
@@ -78,7 +78,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Custom configuration profiles can't include BitLocker settings.",
},
{
name: "XML with Multiple Replace Elements",
@@ -96,14 +96,14 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: false,
+ wantErr: "",
},
{
name: "Empty XML",
profile: MDMWindowsConfigProfile{
SyncML: []byte(``),
},
- wantErr: true,
+ wantErr: "The file should include valid XML",
},
{
name: "XML with Multiple Replace Elements, One with Reserved LocURI",
@@ -121,7 +121,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Custom configuration profiles can't include BitLocker settings",
},
{
name: "XML with Mixed Replace and Add",
@@ -139,7 +139,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Alert",
@@ -157,7 +157,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Atomic",
@@ -175,7 +175,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Delete",
@@ -193,7 +193,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Exec",
@@ -211,7 +211,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Get",
@@ -229,7 +229,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Results",
@@ -247,7 +247,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with Replace and Status",
@@ -265,7 +265,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
name: "XML with elements not defined in the protocol",
@@ -283,10 +283,10 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "Only supported as a top level element. Make sure you don't have other top level elements",
},
{
- name: "invalid XML",
+ name: "invalid XML with mismatched tags",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
@@ -296,7 +296,71 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: true,
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "invalid XML with unclosed root tag",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+ Custom/URI
+
+`),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "invalid XML with unclosed nested tag",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+ Custom/URI
+
+`),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "invalid XML with overlapping elements",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+ Custom/URI
+
+
+`),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "invalid XML with duplicate attributes",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+ Custom/URI
+
+
+
+`),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "invalid XML with special chars",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+ Custom/URI
+ Invalid & Data
+
+
+`)},
+ wantErr: "The file should include valid XML",
},
{
name: "empty LocURI",
@@ -309,7 +373,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: false,
+ wantErr: "",
},
{
name: "item without target",
@@ -321,7 +385,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: false,
+ wantErr: "",
},
{
name: "no items in Replace",
@@ -331,7 +395,7 @@ func TestValidateUserProvided(t *testing.T) {
`),
},
- wantErr: false,
+ wantErr: "",
},
{
name: "Valid XML with reserved name",
@@ -339,15 +403,139 @@ func TestValidateUserProvided(t *testing.T) {
Name: mdm.FleetWindowsOSUpdatesProfileName,
SyncML: []byte(`Custom/URI`),
},
- wantErr: true,
+ wantErr: `Profile name "Windows OS Updates" is not allowed`,
+ },
+ {
+ name: "XML with top level comment",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+
+ Custom/URI
+
+
+ `),
+ },
+ wantErr: "",
+ },
+ {
+ name: "XML with nested root element in data",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+ -
+
+ Custom/URI
+
+
+
+
+
+
+
+ `),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "XML with nested root element under Replace",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+ -
+
+ Custom/URI
+
+
+
+
+
+
+ `),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "XML with root element above Replace",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+ -
+
+ Custom/URI
+
+
+
+
+
+
+ `),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "XML with root element inside Target",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+
+ Custom/URI
+
+
+
+
+
+ `),
+ },
+ wantErr: "The file should include valid XML",
+ },
+ {
+ name: "XML with CDATA used to embed xml",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+ Custom/URI
+
+
+
+ ]]>
+
+
+
+ `),
+ },
+ wantErr: "",
+ },
+ {
+ name: "XML escaped with nested root element",
+ profile: MDMWindowsConfigProfile{
+ SyncML: []byte(`
+
+
+ Custom/URI
+
+ <?xml version="1.0"?>
+ <name>Wireless Network</name>
+
+
+
+ `),
+ },
+ wantErr: "",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.profile.ValidateUserProvided()
- if tt.wantErr {
- require.Error(t, err)
+ if tt.wantErr != "" {
+ require.ErrorContains(t, err, tt.wantErr)
} else {
require.NoError(t, err)
}
diff --git a/server/service/mdm_test.go b/server/service/mdm_test.go
index c9237e4fa7..707fa83fb7 100644
--- a/server/service/mdm_test.go
+++ b/server/service/mdm_test.go
@@ -1022,7 +1022,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) {
wantErr string
}{
{"empty profile", 0, "", true, "The file should include valid XML."},
- {"plist data", 0, string(mcBytesForTest("Foo", "Bar", "UUID")), true, "Only supported as a top level element."},
+ {"plist data", 0, string(mcBytesForTest("Foo", "Bar", "UUID")), true, "The file should include valid XML: processing instructions are not allowed."},
{"random non-xml data", 0, "\x00\x01\x02", true, "The file should include valid XML:"},
{"valid windows profile", 0, ``, true, ""},
{"mdm not enabled", 0, ``, false, "Windows MDM isn't turned on."},
@@ -1033,7 +1033,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) {
{"Windows updates profile", 0, `- ./Device/Vendor/MSFT/Policy/Config/Update/ConfigureDeadlineNoAutoRebootForFeatureUpdates
`, true, "Custom configuration profiles can't include Windows updates settings."},
{"team empty profile", 1, "", true, "The file should include valid XML."},
- {"team plist data", 1, string(mcBytesForTest("Foo", "Bar", "UUID")), true, "Only supported as a top level element."},
+ {"team plist data", 1, string(mcBytesForTest("Foo", "Bar", "UUID")), true, "The file should include valid XML: processing instructions are not allowed."},
{"team random non-xml data", 1, "\x00\x01\x02", true, "The file should include valid XML:"},
{"team valid windows profile", 1, ``, true, ""},
{"team mdm not enabled", 1, ``, false, "Windows MDM isn't turned on."},