Improve Windows profile LocURI content validation (#49715)
**Related issue:** Resolves fleetdm/confidential#16881 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [ ] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [ ] Confirmed that the fix is not expected to adversely impact load test results - [ ] Alerted the release DRI if additional load testing is needed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Windows MDM validation for `LocURI`, ensuring full values are considered before checks. * Rejects empty or whitespace-only `LocURI` entries. * Strengthens `LocURI` validation for Fleet-reserved, SCEP-specific, and BitLocker-related formats after complete assembly. * **Tests** * Added new test cases for malformed BitLocker `LocURI` values split across CDATA and XML comment boundaries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -0,0 +1 @@
|
||||
- Improved LocURI validation in Windows profile handling to canonicalize element content before checking.
|
||||
+44
-19
@@ -72,6 +72,11 @@ type windowsProfileValidator struct {
|
||||
// close tag fires; used to reject `<LocURI></LocURI>` which a real Windows device returns status 400 for.
|
||||
locURIHasContent bool
|
||||
|
||||
// Accumulates all CharData fragments within a <LocURI> 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 <LocURI></LocURI> 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 <LocURI></LocURI> 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("<LocURI> 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
|
||||
// <Exec> 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
|
||||
|
||||
@@ -957,6 +957,32 @@ func TestValidateUserProvided(t *testing.T) {
|
||||
},
|
||||
wantErr: "",
|
||||
},
|
||||
{
|
||||
name: "BitLocker LocURI split across CDATA boundary is rejected",
|
||||
profile: MDMWindowsConfigProfile{
|
||||
SyncML: []byte(`
|
||||
<Replace>
|
||||
<Item>
|
||||
<Target><LocURI>./Device/Vendor/MSFT/Bit<![CDATA[Locker]]>/RequireDeviceEncryption</LocURI></Target>
|
||||
</Item>
|
||||
</Replace>
|
||||
`),
|
||||
},
|
||||
wantErr: syncml.DiskEncryptionProfileRestrictionErrMsg,
|
||||
},
|
||||
{
|
||||
name: "BitLocker LocURI split across XML comment boundary is rejected",
|
||||
profile: MDMWindowsConfigProfile{
|
||||
SyncML: []byte(`
|
||||
<Replace>
|
||||
<Item>
|
||||
<Target><LocURI>./Device/Vendor/MSFT/Bit<!--x-->Locker/RequireDeviceEncryption</LocURI></Target>
|
||||
</Item>
|
||||
</Replace>
|
||||
`),
|
||||
},
|
||||
wantErr: syncml.DiskEncryptionProfileRestrictionErrMsg,
|
||||
},
|
||||
{
|
||||
name: "BitLocker LocURI allowed when custom disk encryption is enabled",
|
||||
profile: MDMWindowsConfigProfile{
|
||||
|
||||
Reference in New Issue
Block a user