From 0e6c790803d1b4407c5b4b41a67a37864a3d3573 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Sat, 10 Jan 2026 20:04:27 -0600 Subject: [PATCH] Add additional validation to mdmMicrosoftAuthEndpoint (#38147) - [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) ## Testing - [x] Added/updated automated tests - [ ] QA'd all new/changed functionality manually --- changes/fix-redirect-url-handling | 1 + server/service/integration_mdm_test.go | 76 +++++++++++++++++++++++++- server/service/microsoft_mdm.go | 21 ++++++- server/service/microsoft_mdm_test.go | 74 +++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 3 deletions(-) create mode 100644 changes/fix-redirect-url-handling diff --git a/changes/fix-redirect-url-handling b/changes/fix-redirect-url-handling new file mode 100644 index 0000000000..8868298686 --- /dev/null +++ b/changes/fix-redirect-url-handling @@ -0,0 +1 @@ +* Added additional validation to URL parameter for MS MDM auth endpoint diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 59c0b30c84..afe081ef50 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -7917,7 +7917,8 @@ func (s *integrationMDMTestSuite) TestValidGetAuthRequest() { // Checking response content resContent := string(resBytes) require.Contains(t, resContent, "inputToken.name = 'wresult'") - require.Contains(t, resContent, "form.action = \"ms-app://windows.immersivecontrolpanel\"") + // we expect the URL to be escaped + require.Contains(t, resContent, `form.action = "ms-app:\/\/windows.immersivecontrolpanel"`) require.Contains(t, resContent, "performPost()") // Getting token content @@ -7939,6 +7940,79 @@ func (s *integrationMDMTestSuite) TestInvalidGetAuthRequest() { require.Contains(t, resContent, "forbidden") } +func (s *integrationMDMTestSuite) TestAppruValidationInGetAuthRequest() { + t := s.T() + + // Test cases with invalid appru values that should bail due to exiting before auth check + invalidAppruCases := []struct { + name string + appru string + }{ + { + name: "javascript injection", + appru: "%3Bfor%20(var%20key%20in%20localStorage)%7B%20alert(key)%7D%3B%2F%2F", + }, + { + name: "javascript protocol", + appru: "javascript:alert(1)", + }, + { + name: "data URI", + appru: "data:text/html,", + }, + { + name: "empty scheme", + appru: "://example.com", + }, + { + name: "plain text", + appru: "not-a-url", + }, + } + + for _, tc := range invalidAppruCases { + t.Run(tc.name, func(t *testing.T) { + targetEndpointURL := microsoft_mdm.MDE2AuthPath + "?appru=" + tc.appru + "&login_hint=demo%40example.com" + resp := s.DoRaw("GET", targetEndpointURL, nil, http.StatusInternalServerError) + + resBytes, err := io.ReadAll(resp.Body) + resContent := string(resBytes) + require.NoError(t, err) + require.NotEmpty(t, resBytes) + require.Contains(t, resContent, "forbidden") + + resp.Body.Close() + }) + } + + // Also verify valid URLs still work + validAppruCases := []struct { + name string + appru string + }{ + { + name: "ms-app scheme", + appru: "ms-app%3A%2F%2Fwindows.immersivecontrolpanel", + }, + { + name: "https scheme", + appru: "https%3A%2F%2Fexample.com%2Fcallback", + }, + { + name: "http scheme", + appru: "http%3A%2F%2Flocalhost%2Fcallback", + }, + } + + for _, tc := range validAppruCases { + t.Run(tc.name, func(t *testing.T) { + targetEndpointURL := microsoft_mdm.MDE2AuthPath + "?appru=" + tc.appru + "&login_hint=demo%40example.com" + resp := s.DoRaw("GET", targetEndpointURL, nil, http.StatusOK) + resp.Body.Close() + }) + } +} + func (s *integrationMDMTestSuite) TestValidGetTOC() { t := s.T() diff --git a/server/service/microsoft_mdm.go b/server/service/microsoft_mdm.go index 1d51745f3e..7e42ee8f30 100644 --- a/server/service/microsoft_mdm.go +++ b/server/service/microsoft_mdm.go @@ -11,13 +11,14 @@ import ( "errors" "fmt" "html" + "html/template" "io" "net" "net/http" "net/url" + "slices" "strconv" "strings" - "text/template" "time" "github.com/fleetdm/fleet/v4/pkg/fleetdbase" @@ -409,7 +410,7 @@ func NewSoapFault(errorType string, origMessage int, errorMessage error) mdm_typ } } -// getSTSAuthContent Retuns STS auth content +// getSTSAuthContent Returns STS auth content func getSTSAuthContent(data string) mdm_types.Errorer { return MDMAuthContainer{ Data: &data, @@ -777,6 +778,17 @@ func mdmMicrosoftDiscoveryEndpoint(ctx context.Context, request interface{}, svc }, nil } +// isValidAppru validates that appru is a valid URL with an allowed scheme. +// It returns true if appru is a valid URL with http, https, or ms-app scheme. +func isValidAppru(appru string) bool { + parsed, err := url.Parse(appru) + if err != nil { + return false + } + + return slices.Contains([]string{"http", "https", "ms-app"}, parsed.Scheme) +} + // mdmMicrosoftAuthEndpoint handles the Security Token Service (STS) implementation func mdmMicrosoftAuthEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (mdm_types.Errorer, error) { params := request.(*SoapRequestContainer).Params @@ -793,6 +805,11 @@ func mdmMicrosoftAuthEndpoint(ctx context.Context, request interface{}, svc flee return getSTSAuthContent(""), errors.New("expected STS params are empty") } + // Validate that appru is a valid URL + if !isValidAppru(appru) { + return getSTSAuthContent(""), fmt.Errorf("non-URL appru parameter attempted: %q", appru) + } + // Getting the STS endpoint HTML content stsAuthContent, err := svc.GetMDMMicrosoftSTSAuthResponse(ctx, appru, loginHint) if err != nil { diff --git a/server/service/microsoft_mdm_test.go b/server/service/microsoft_mdm_test.go index bdd888b5a1..055d830783 100644 --- a/server/service/microsoft_mdm_test.go +++ b/server/service/microsoft_mdm_test.go @@ -14,6 +14,7 @@ import ( "github.com/fleetdm/fleet/v4/server/mdm/microsoft/syncml" "github.com/fleetdm/fleet/v4/server/mock" "github.com/go-kit/log" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -35,6 +36,79 @@ func NewSoapRequest(request []byte) (fleet.SoapRequest, error) { return req, nil } +func TestIsValidAppruURL(t *testing.T) { + tests := []struct { + name string + appru string + expected bool + }{ + // Valid URLs + { + name: "valid ms-app scheme", + appru: "ms-app://windows.immersivecontrolpanel", + expected: true, + }, + { + name: "valid https scheme", + appru: "https://example.com/callback", + expected: true, + }, + { + name: "valid http scheme", + appru: "http://localhost/callback", + expected: true, + }, + // Invalid URLs - XSS attempts + { + name: "javascript injection", + appru: ";for (var key in localStorage){ alert(key)};//", + expected: false, + }, + { + name: "javascript protocol", + appru: "javascript:alert(1)", + expected: false, + }, + { + name: "data URI", + appru: "data:text/html,", + expected: false, + }, + { + name: "empty scheme", + appru: "://example.com", + expected: false, + }, + { + name: "plain text", + appru: "not-a-url", + expected: false, + }, + { + name: "empty string", + appru: "", + expected: false, + }, + { + name: "file scheme", + appru: "file:///etc/passwd", + expected: false, + }, + { + name: "ftp scheme", + appru: "ftp://example.com", + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := isValidAppru(tc.appru) + assert.Equal(t, tc.expected, result) + }) + } +} + func TestValidSoapResponse(t *testing.T) { relatesTo := "urn:uuid:0d5a1441-5891-453b-becf-a2e5f6ea3749" soapFaultMsg := NewSoapFault(syncml.SoapErrorAuthentication, fleet.MDEDiscovery, errors.New("test"))