diff --git a/changes/windows-locuri-validation.md b/changes/windows-locuri-validation.md new file mode 100644 index 0000000000..01f68f2753 --- /dev/null +++ b/changes/windows-locuri-validation.md @@ -0,0 +1 @@ +- Improved LocURI validation in Windows profile handling to canonicalize element content before checking. diff --git a/server/fleet/windows_mdm.go b/server/fleet/windows_mdm.go index aa4dba9687..cc64d1e981 100644 --- a/server/fleet/windows_mdm.go +++ b/server/fleet/windows_mdm.go @@ -72,6 +72,11 @@ type windowsProfileValidator struct { // close tag fires; used to reject `` which a real Windows device returns status 400 for. locURIHasContent bool + // Accumulates all CharData fragments within a element so the complete value is validated as a whole. This + // prevents bypasses where a forbidden substring (e.g. "BitLocker") is split across CDATA or comment boundaries so + // that no single CharData token contains the full reserved string. + locURIAccumulator strings.Builder + // When true, custom BitLocker (disk encryption) LocURIs are allowed instead of being rejected. allowCustomDiskEncryption bool @@ -219,13 +224,43 @@ func (v *windowsProfileValidator) handleEndElement(el xml.EndElement) error { v.currentTopLevelElement = "" } - // An empty produces no CharData token, so we catch it here when the close tag fires before any - // content. Whitespace-only content is rejected in validateLocURIFormat. + // An empty or whitespace-only LocURI produces no non-whitespace CharData, so locURIHasContent + // stays false and we reject here before any further validation runs. if elementName == "LocURI" && !v.locURIHasContent { - v.currentElement = "" return errors.New(" can't be empty.") } + // When leaving a LocURI element, validate the fully accumulated content so that forbidden substrings split across + // CDATA or comment boundaries are caught. + if elementName == "LocURI" { + locURI := v.locURIAccumulator.String() + v.locURIAccumulator.Reset() + + // Check for Fleet-reserved LocURIs (e.g. BitLocker, Windows Updates). Runs first so users + // get the specific "managed by Fleet" error instead of a generic format error. + if err := validateFleetProvidedLocURI(locURI, v.allowCustomDiskEncryption); err != nil { + return err + } + + // Validate structural format rules (must start with "./", no invalid characters, etc.) + // that real Windows devices enforce with status 400. + if err := validateLocURIFormat(locURI); err != nil { + return err + } + + // Validate SCEP-specific constraints depending on whether this LocURI is inside an + // command (certificate operations) or a non-Exec command (Add/Replace). + if v.isInExec() { + if err := v.scepValidator.validateExecLocURI(locURI); err != nil { + return err + } + } else { + if err := v.scepValidator.validateLocURI(locURI); err != nil { + return err + } + } + } + v.currentElement = "" v.locURIHasContent = false return nil @@ -237,25 +272,15 @@ func (v *windowsProfileValidator) handleCharData(el xml.CharData) error { return nil } - locURI := string(el) - if strings.TrimSpace(locURI) != "" { + fragment := string(el) + if strings.TrimSpace(fragment) != "" { v.locURIHasContent = true } - // Surface Fleet-reserved URI errors (BitLocker, Windows updates) before the generic format check so users get the more - // specific message. - if err := validateFleetProvidedLocURI(locURI, v.allowCustomDiskEncryption); err != nil { - return err - } - - if err := validateLocURIFormat(locURI); err != nil { - return err - } - - if v.isInExec() { - return v.scepValidator.validateExecLocURI(locURI) - } - return v.scepValidator.validateLocURI(locURI) + // Accumulate CharData fragments; actual validation happens in handleEndElement when the full LocURI value is known. + // This prevents bypasses where a forbidden substring is split across CDATA or comment boundaries. + v.locURIAccumulator.WriteString(fragment) + return nil } // validateLocURIFormat rejects LocURI values that real Windows MDM devices reject with status 400 (empirically verified diff --git a/server/fleet/windows_mdm_test.go b/server/fleet/windows_mdm_test.go index 51f64ca904..56bd8d6534 100644 --- a/server/fleet/windows_mdm_test.go +++ b/server/fleet/windows_mdm_test.go @@ -957,6 +957,32 @@ func TestValidateUserProvided(t *testing.T) { }, wantErr: "", }, + { + name: "BitLocker LocURI split across CDATA boundary is rejected", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + ./Device/Vendor/MSFT/Bit/RequireDeviceEncryption + + +`), + }, + wantErr: syncml.DiskEncryptionProfileRestrictionErrMsg, + }, + { + name: "BitLocker LocURI split across XML comment boundary is rejected", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + ./Device/Vendor/MSFT/BitLocker/RequireDeviceEncryption + + +`), + }, + wantErr: syncml.DiskEncryptionProfileRestrictionErrMsg, + }, { name: "BitLocker LocURI allowed when custom disk encryption is enabled", profile: MDMWindowsConfigProfile{