Skip to content

Commit 0e6c790

Browse files
authored
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
1 parent 0c2b0d2 commit 0e6c790

File tree

4 files changed

+169
-3
lines changed

4 files changed

+169
-3
lines changed

changes/fix-redirect-url-handling

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* Added additional validation to URL parameter for MS MDM auth endpoint

server/service/integration_mdm_test.go

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7917,7 +7917,8 @@ func (s *integrationMDMTestSuite) TestValidGetAuthRequest() {
79177917
// Checking response content
79187918
resContent := string(resBytes)
79197919
require.Contains(t, resContent, "inputToken.name = 'wresult'")
7920-
require.Contains(t, resContent, "form.action = \"ms-app://windows.immersivecontrolpanel\"")
7920+
// we expect the URL to be escaped
7921+
require.Contains(t, resContent, `form.action = "ms-app:\/\/windows.immersivecontrolpanel"`)
79217922
require.Contains(t, resContent, "performPost()")
79227923

79237924
// Getting token content
@@ -7939,6 +7940,79 @@ func (s *integrationMDMTestSuite) TestInvalidGetAuthRequest() {
79397940
require.Contains(t, resContent, "forbidden")
79407941
}
79417942

7943+
func (s *integrationMDMTestSuite) TestAppruValidationInGetAuthRequest() {
7944+
t := s.T()
7945+
7946+
// Test cases with invalid appru values that should bail due to exiting before auth check
7947+
invalidAppruCases := []struct {
7948+
name string
7949+
appru string
7950+
}{
7951+
{
7952+
name: "javascript injection",
7953+
appru: "%3Bfor%20(var%20key%20in%20localStorage)%7B%20alert(key)%7D%3B%2F%2F",
7954+
},
7955+
{
7956+
name: "javascript protocol",
7957+
appru: "javascript:alert(1)",
7958+
},
7959+
{
7960+
name: "data URI",
7961+
appru: "data:text/html,<script>alert(1)</script>",
7962+
},
7963+
{
7964+
name: "empty scheme",
7965+
appru: "://example.com",
7966+
},
7967+
{
7968+
name: "plain text",
7969+
appru: "not-a-url",
7970+
},
7971+
}
7972+
7973+
for _, tc := range invalidAppruCases {
7974+
t.Run(tc.name, func(t *testing.T) {
7975+
targetEndpointURL := microsoft_mdm.MDE2AuthPath + "?appru=" + tc.appru + "&login_hint=demo%40example.com"
7976+
resp := s.DoRaw("GET", targetEndpointURL, nil, http.StatusInternalServerError)
7977+
7978+
resBytes, err := io.ReadAll(resp.Body)
7979+
resContent := string(resBytes)
7980+
require.NoError(t, err)
7981+
require.NotEmpty(t, resBytes)
7982+
require.Contains(t, resContent, "forbidden")
7983+
7984+
resp.Body.Close()
7985+
})
7986+
}
7987+
7988+
// Also verify valid URLs still work
7989+
validAppruCases := []struct {
7990+
name string
7991+
appru string
7992+
}{
7993+
{
7994+
name: "ms-app scheme",
7995+
appru: "ms-app%3A%2F%2Fwindows.immersivecontrolpanel",
7996+
},
7997+
{
7998+
name: "https scheme",
7999+
appru: "https%3A%2F%2Fexample.com%2Fcallback",
8000+
},
8001+
{
8002+
name: "http scheme",
8003+
appru: "http%3A%2F%2Flocalhost%2Fcallback",
8004+
},
8005+
}
8006+
8007+
for _, tc := range validAppruCases {
8008+
t.Run(tc.name, func(t *testing.T) {
8009+
targetEndpointURL := microsoft_mdm.MDE2AuthPath + "?appru=" + tc.appru + "&login_hint=demo%40example.com"
8010+
resp := s.DoRaw("GET", targetEndpointURL, nil, http.StatusOK)
8011+
resp.Body.Close()
8012+
})
8013+
}
8014+
}
8015+
79428016
func (s *integrationMDMTestSuite) TestValidGetTOC() {
79438017
t := s.T()
79448018

server/service/microsoft_mdm.go

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,14 @@ import (
1111
"errors"
1212
"fmt"
1313
"html"
14+
"html/template"
1415
"io"
1516
"net"
1617
"net/http"
1718
"net/url"
19+
"slices"
1820
"strconv"
1921
"strings"
20-
"text/template"
2122
"time"
2223

2324
"github.com/fleetdm/fleet/v4/pkg/fleetdbase"
@@ -409,7 +410,7 @@ func NewSoapFault(errorType string, origMessage int, errorMessage error) mdm_typ
409410
}
410411
}
411412

412-
// getSTSAuthContent Retuns STS auth content
413+
// getSTSAuthContent Returns STS auth content
413414
func getSTSAuthContent(data string) mdm_types.Errorer {
414415
return MDMAuthContainer{
415416
Data: &data,
@@ -777,6 +778,17 @@ func mdmMicrosoftDiscoveryEndpoint(ctx context.Context, request interface{}, svc
777778
}, nil
778779
}
779780

781+
// isValidAppru validates that appru is a valid URL with an allowed scheme.
782+
// It returns true if appru is a valid URL with http, https, or ms-app scheme.
783+
func isValidAppru(appru string) bool {
784+
parsed, err := url.Parse(appru)
785+
if err != nil {
786+
return false
787+
}
788+
789+
return slices.Contains([]string{"http", "https", "ms-app"}, parsed.Scheme)
790+
}
791+
780792
// mdmMicrosoftAuthEndpoint handles the Security Token Service (STS) implementation
781793
func mdmMicrosoftAuthEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (mdm_types.Errorer, error) {
782794
params := request.(*SoapRequestContainer).Params
@@ -793,6 +805,11 @@ func mdmMicrosoftAuthEndpoint(ctx context.Context, request interface{}, svc flee
793805
return getSTSAuthContent(""), errors.New("expected STS params are empty")
794806
}
795807

808+
// Validate that appru is a valid URL
809+
if !isValidAppru(appru) {
810+
return getSTSAuthContent(""), fmt.Errorf("non-URL appru parameter attempted: %q", appru)
811+
}
812+
796813
// Getting the STS endpoint HTML content
797814
stsAuthContent, err := svc.GetMDMMicrosoftSTSAuthResponse(ctx, appru, loginHint)
798815
if err != nil {

server/service/microsoft_mdm_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/fleetdm/fleet/v4/server/mdm/microsoft/syncml"
1515
"github.com/fleetdm/fleet/v4/server/mock"
1616
"github.com/go-kit/log"
17+
"github.com/stretchr/testify/assert"
1718
"github.com/stretchr/testify/require"
1819
)
1920

@@ -35,6 +36,79 @@ func NewSoapRequest(request []byte) (fleet.SoapRequest, error) {
3536
return req, nil
3637
}
3738

39+
func TestIsValidAppruURL(t *testing.T) {
40+
tests := []struct {
41+
name string
42+
appru string
43+
expected bool
44+
}{
45+
// Valid URLs
46+
{
47+
name: "valid ms-app scheme",
48+
appru: "ms-app://windows.immersivecontrolpanel",
49+
expected: true,
50+
},
51+
{
52+
name: "valid https scheme",
53+
appru: "https://example.com/callback",
54+
expected: true,
55+
},
56+
{
57+
name: "valid http scheme",
58+
appru: "http://localhost/callback",
59+
expected: true,
60+
},
61+
// Invalid URLs - XSS attempts
62+
{
63+
name: "javascript injection",
64+
appru: ";for (var key in localStorage){ alert(key)};//",
65+
expected: false,
66+
},
67+
{
68+
name: "javascript protocol",
69+
appru: "javascript:alert(1)",
70+
expected: false,
71+
},
72+
{
73+
name: "data URI",
74+
appru: "data:text/html,<script>alert(1)</script>",
75+
expected: false,
76+
},
77+
{
78+
name: "empty scheme",
79+
appru: "://example.com",
80+
expected: false,
81+
},
82+
{
83+
name: "plain text",
84+
appru: "not-a-url",
85+
expected: false,
86+
},
87+
{
88+
name: "empty string",
89+
appru: "",
90+
expected: false,
91+
},
92+
{
93+
name: "file scheme",
94+
appru: "file:///etc/passwd",
95+
expected: false,
96+
},
97+
{
98+
name: "ftp scheme",
99+
appru: "ftp://example.com",
100+
expected: false,
101+
},
102+
}
103+
104+
for _, tc := range tests {
105+
t.Run(tc.name, func(t *testing.T) {
106+
result := isValidAppru(tc.appru)
107+
assert.Equal(t, tc.expected, result)
108+
})
109+
}
110+
}
111+
38112
func TestValidSoapResponse(t *testing.T) {
39113
relatesTo := "urn:uuid:0d5a1441-5891-453b-becf-a2e5f6ea3749"
40114
soapFaultMsg := NewSoapFault(syncml.SoapErrorAuthentication, fleet.MDEDiscovery, errors.New("test"))

0 commit comments

Comments
 (0)