Elgato_dark/fleet
1
0
Fork 0
mirror of https://github.com/fleetdm/fleet synced 2026-05-23 17:08:53 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 83

improve validation of windows profiles (#16563)

Browse source 
for #16316, this improves the XML validation of Windows profiles and
ensures we support two ways of embedding XML:

- Escape the XML
- Use a wrapping `<![CDATA[ ... ]]>` element
This commit is contained in:
Roberto Dip 2024-02-05 09:49:55 -03:00 committed by GitHub
parent 5360029d67
commit e35d1dacbd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 289 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
changes/16316-windows-xml-validation Normal file
View file

@ -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 `<![CDATA[ ... ]]>` element

101
server/fleet/windows_mdm.go
View file

@ -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 <Replace> and none of the <LocURI>
// elements within <Target> 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("<SyncBody>%s</SyncBody>", 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 <Replace>
if mdm.GetRawProfilePlatform(m.SyncML) != "windows" {
return errors.New("Only <Replace> 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 <Replace> 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 (<?target inst?>)
// 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 <Replace> 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 <Replace> 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

242
server/fleet/windows_mdm_test.go
View file

@ -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) {
</Replace>
`),
},
wantErr: false,
wantErr: "",
},
{
name: "Invalid Platform",
@ -39,10 +39,10 @@ func TestValidateUserProvided(t *testing.T) {
</SyncML>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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(`
<Add>
@ -52,7 +52,7 @@ func TestValidateUserProvided(t *testing.T) {
</Add>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Replace>
`),
},
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) {
</Replace>
`),
},
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) {
</Replace>
`),
},
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) {
</Replace>
`),
},
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) {
</Add>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Alert>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Atomic>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Delete>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Exec>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Get>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Results>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Status>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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) {
</Foo>
`),
},
wantErr: true,
wantErr: "Only <Replace> 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(`
<Replace>
@ -296,7 +296,71 @@ func TestValidateUserProvided(t *testing.T) {
</Add>
`),
},
wantErr: true,
wantErr: "The file should include valid XML",
},
{
name: "invalid XML with unclosed root tag",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target><LocURI>Custom/URI</LocURI></Target>
</Item>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "invalid XML with unclosed nested tag",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target><LocURI>Custom/URI</LocURI></Target>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "invalid XML with overlapping elements",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target><LocURI>Custom/URI</Target></LocURI>
</Item>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "invalid XML with duplicate attributes",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target><LocURI>Custom/URI</Target></LocURI>
<Data attr="1" attr="2"></Data>
</Item>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "invalid XML with special chars",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target><LocURI>Custom/URI</Target></LocURI>
<Data>Invalid & Data</Data>
</Item>
</Replace>
`)},
wantErr: "The file should include valid XML",
},
{
name: "empty LocURI",
@ -309,7 +373,7 @@ func TestValidateUserProvided(t *testing.T) {
</Replace>
`),
},
wantErr: false,
wantErr: "",
},
{
name: "item without target",
@ -321,7 +385,7 @@ func TestValidateUserProvided(t *testing.T) {
</Replace>
`),
},
wantErr: false,
wantErr: "",
},
{
name: "no items in Replace",
@ -331,7 +395,7 @@ func TestValidateUserProvided(t *testing.T) {
</Replace>
`),
},
wantErr: false,
wantErr: "",
},
{
name: "Valid XML with reserved name",
@ -339,15 +403,139 @@ func TestValidateUserProvided(t *testing.T) {
Name: mdm.FleetWindowsOSUpdatesProfileName,
SyncML: []byte(`<Replace><Target><LocURI>Custom/URI</LocURI></Target></Replace>`),
},
wantErr: true,
wantErr: `Profile name "Windows OS Updates" is not allowed`,
},
{
name: "XML with top level comment",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<!-- this is a comment -->
<Replace>
<Target>
<LocURI>Custom/URI</LocURI>
</Target>
</Replace>
`),
},
wantErr: "",
},
{
name: "XML with nested root element in data",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Item>
<Target>
<LocURI>Custom/URI</LocURI>
<Data>
<?xml version="1.0"?>
<Foo></Foo>
</Data>
</Target>
</Item>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "XML with nested root element under Replace",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<?xml version="1.0"?>
<Item>
<Target>
<LocURI>Custom/URI</LocURI>
<Data>
<Foo></Foo>
</Data>
</Target>
</Item>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "XML with root element above Replace",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<?xml version="1.0"?>
<Replace>
<Item>
<Target>
<LocURI>Custom/URI</LocURI>
<Data>
<Foo></Foo>
</Data>
</Target>
</Item>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "XML with root element inside Target",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Target>
<?xml version="1.0"?>
<LocURI>Custom/URI</LocURI>
<Data>
<Foo></Foo>
</Data>
</Target>
</Replace>
`),
},
wantErr: "The file should include valid XML",
},
{
name: "XML with CDATA used to embed xml",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Target>
<LocURI>Custom/URI</LocURI>
<Data>
<![CDATA[
<?xml version="1.0"?>
<Foo></Foo>
]]>
</Data>
</Target>
</Replace>
`),
},
wantErr: "",
},
{
name: "XML escaped with nested root element",
profile: MDMWindowsConfigProfile{
SyncML: []byte(`
<Replace>
<Target>
<LocURI>Custom/URI</LocURI>
<Data>
&lt;?xml version=&quot;1.0&quot;?&gt;
&lt;name&gt;Wireless Network&lt;/name&gt;
</Data>
</Target>
</Replace>
`),
},
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)
}

4
server/service/mdm_test.go
View file

@ -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 <Replace> 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, `<Replace></Replace>`, true, ""},
{"mdm not enabled", 0, `<Replace></Replace>`, false, "Windows MDM isn't turned on."},
@ -1033,7 +1033,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) {
{"Windows updates profile", 0, `<Replace><Item><Target><LocURI> ./Device/Vendor/MSFT/Policy/Config/Update/ConfigureDeadlineNoAutoRebootForFeatureUpdates </LocURI></Target></Item></Replace>`, 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 <Replace> 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, `<Replace></Replace>`, true, ""},
{"team mdm not enabled", 1, `<Replace></Replace>`, false, "Windows MDM isn't turned on."},