From 2c337a4f17f8b30eaa8b7695894bd47e5bd93789 Mon Sep 17 00:00:00 2001 From: Roberto Dip Date: Wed, 29 Nov 2023 18:13:39 -0300 Subject: [PATCH] improve profile validation to detect invalid XML (#15365) for #15361 --- server/fleet/windows_mdm.go | 60 +++--- server/fleet/windows_mdm_test.go | 322 ++++++++++++++++++++++++------- 2 files changed, 293 insertions(+), 89 deletions(-) diff --git a/server/fleet/windows_mdm.go b/server/fleet/windows_mdm.go index 08b719c7e6..21d9492628 100644 --- a/server/fleet/windows_mdm.go +++ b/server/fleet/windows_mdm.go @@ -2,12 +2,12 @@ package fleet import ( "bytes" + "encoding/xml" "errors" "fmt" "strings" "time" - "github.com/beevik/etree" "github.com/fleetdm/fleet/v4/server/mdm" microsoft_mdm "github.com/fleetdm/fleet/v4/server/mdm/microsoft" ) @@ -45,38 +45,50 @@ type MDMWindowsConfigProfile struct { // // Returns an error if these conditions are not met. func (m *MDMWindowsConfigProfile) ValidateUserProvided() error { + if len(bytes.TrimSpace(m.SyncML)) == 0 { + return errors.New("The file should include valid XML.") + } + if _, ok := microsoft_mdm.FleetReservedProfileNames()[m.Name]; ok { return fmt.Errorf("Profile name %q is not allowed.", m.Name) } - if mdm.GetRawProfilePlatform(m.SyncML) != "windows" { - // it doesn't start with , check if it is still valid XML. - if len(bytes.TrimSpace(m.SyncML)) == 0 { - return errors.New("The file should include valid XML.") - } - doc := etree.NewDocument() - if err := doc.ReadFromBytes(m.SyncML); err != nil { - return fmt.Errorf("The file should include valid XML: %w", err) - } - // valid xml, but does not start with - return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.") + var validator struct { + SyncBody + NonProtocolElements []interface{} `xml:",any,omitempty"` } - - doc := etree.NewDocument() - if err := doc.ReadFromBytes(m.SyncML); err != nil { + 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) } - for _, element := range doc.ChildElements() { - if element.Tag != CmdReplace { - return errors.New("Only supported as a top level element. Make sure you don't have other top level elements.") - } + // 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.") + } - for _, locURI := range element.FindElements("//Item/Target/LocURI") { - if locURI != nil { - if err := validateFleetProvidedLocURI(locURI.Text()); err != nil { - return err - } + 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 } } } diff --git a/server/fleet/windows_mdm_test.go b/server/fleet/windows_mdm_test.go index 0f57042795..d1993244ba 100644 --- a/server/fleet/windows_mdm_test.go +++ b/server/fleet/windows_mdm_test.go @@ -17,12 +17,12 @@ func TestValidateUserProvided(t *testing.T) { name: "Valid XML with Replace", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - Custom/URI - - - `), + + + Custom/URI + + +`), }, wantErr: false, }, @@ -30,14 +30,14 @@ func TestValidateUserProvided(t *testing.T) { name: "Invalid Platform", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - - Custom/URI - - - - `), + + + + Custom/URI + + + +`), }, wantErr: true, }, @@ -45,12 +45,12 @@ func TestValidateUserProvided(t *testing.T) { name: "Invalid XML Structure", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - Custom/URI - - - `), + + + Custom/URI + + +`), }, wantErr: true, }, @@ -58,12 +58,12 @@ func TestValidateUserProvided(t *testing.T) { name: "Reserved LocURI", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - ./Device/Vendor/MSFT/BitLocker/Foo - - - `), + + + ./Device/Vendor/MSFT/BitLocker/Foo + + +`), }, wantErr: true, }, @@ -71,12 +71,12 @@ func TestValidateUserProvided(t *testing.T) { name: "Reserved LocURI with implicit ./Device prefix", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - ./Vendor/MSFT/BitLocker/Foo - - - `), + + + ./Vendor/MSFT/BitLocker/Foo + + +`), }, wantErr: true, }, @@ -84,17 +84,17 @@ func TestValidateUserProvided(t *testing.T) { name: "XML with Multiple Replace Elements", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - Custom/URI1 - - - - - Custom/URI2 - - - `), + + + Custom/URI1 + + + + + Custom/URI2 + + +`), }, wantErr: false, }, @@ -109,17 +109,17 @@ func TestValidateUserProvided(t *testing.T) { name: "XML with Multiple Replace Elements, One with Reserved LocURI", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - Custom/URI - - - - - ./Device/Vendor/MSFT/BitLocker/Bar - - - `), + + + Custom/URI + + + + + ./Device/Vendor/MSFT/BitLocker/Bar + + +`), }, wantErr: true, }, @@ -127,20 +127,212 @@ func TestValidateUserProvided(t *testing.T) { name: "XML with Mixed Replace and Add", profile: MDMWindowsConfigProfile{ SyncML: []byte(` - - - Custom/URI - - - - - Another/URI - - - `), + + + Custom/URI + + + + + Another/URI + + +`), }, wantErr: true, }, + { + name: "XML with Replace and Alert", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Alert/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Atomic", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Atomic/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Delete", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Delete/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Exec", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Exec/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Get", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Get/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Results", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Results/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with Replace and Status", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Replace/URI + + + + + Status/URI + + +`), + }, + wantErr: true, + }, + { + name: "XML with elements not defined in the protocol", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Custom/URI + + + + + Another/URI + + +`), + }, + wantErr: true, + }, + { + name: "invalid XML", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Custom/URI + + +`), + }, + wantErr: true, + }, + { + name: "empty LocURI", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + + + +`), + }, + wantErr: false, + }, + { + name: "item without target", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + + +`), + }, + wantErr: false, + }, + { + name: "no items in Replace", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + +`), + }, + wantErr: false, + }, { name: "Valid XML with reserved name", profile: MDMWindowsConfigProfile{