diff --git a/go.mod b/go.mod index ec65ad56..a7666c9c 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,6 @@ require ( github.com/go-pkgz/repeater v1.1.3 github.com/go-pkgz/rest v1.18.2 github.com/golang-jwt/jwt v3.2.2+incompatible - github.com/microcosm-cc/bluemonday v1.0.26 github.com/nullrocks/identicon v0.0.0-20180626043057-7875f45b0022 github.com/stretchr/testify v1.8.4 go.etcd.io/bbolt v1.3.8 @@ -21,12 +20,10 @@ require ( require ( cloud.google.com/go/compute v1.23.3 // indirect cloud.google.com/go/compute/metadata v0.2.3 // indirect - github.com/aymerick/douceur v0.2.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/golang/snappy v0.0.4 // indirect github.com/google/uuid v1.1.1 // indirect - github.com/gorilla/css v1.0.1 // indirect github.com/klauspost/compress v1.17.4 // indirect github.com/montanaflynn/stats v0.7.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect @@ -43,7 +40,6 @@ require ( github.com/xdg-go/stringprep v1.0.4 // indirect github.com/youmark/pkcs8 v0.0.0-20201027041543-1326539a0a0a // indirect golang.org/x/crypto v0.17.0 // indirect - golang.org/x/net v0.19.0 // indirect golang.org/x/sync v0.5.0 // indirect golang.org/x/sys v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect diff --git a/go.sum b/go.sum index 6b83dce8..ce6d6407 100644 --- a/go.sum +++ b/go.sum @@ -7,8 +7,6 @@ github.com/ajg/form v1.5.1 h1:t9c7v8JUKu/XxOGBU0yjNpaMloxGEJhUkqFRq0ibGeU= github.com/ajg/form v1.5.1/go.mod h1:uL1WgH+h2mgNtvBq0339dVnzXdBETtL2LeUXaIv25UY= github.com/andybalholm/brotli v1.0.4 h1:V7DdXeJtZscaqfNuAdSRuRFzuiKlHSC/Zh3zl9qY3JY= github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig= -github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= -github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -61,8 +59,6 @@ github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY= -github.com/gorilla/css v1.0.1 h1:ntNaBIghp6JmvWnxbZKANoLyuXTPZ4cAMlo6RyhlbO8= -github.com/gorilla/css v1.0.1/go.mod h1:BvnYkspnSzMmwRK+b8/xgNPLiIuNZr6vbZBTPQ2A3b0= github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc= github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= @@ -82,8 +78,6 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/mattn/go-colorable v0.1.7/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU= -github.com/microcosm-cc/bluemonday v1.0.26 h1:xbqSvqzQMeEHCqMi64VAs4d8uy6Mequs3rQ0k/Khz58= -github.com/microcosm-cc/bluemonday v1.0.26/go.mod h1:JyzOCs9gkyQyjs+6h10UEVSe02CGwkhd72Xdqh78TWs= github.com/montanaflynn/stats v0.0.0-20171201202039-1bf9dbcd8cbe/go.mod h1:wL8QJuTMNUDYhXwkmfOly8iTdp5TEcJFWZD2D7SIkUc= github.com/montanaflynn/stats v0.7.1 h1:etflOAAHORrCC44V+aR6Ftzort912ZU+YLiSTuV8eaE= github.com/montanaflynn/stats v0.7.1/go.mod h1:etXPPgVO6n31NxCd9KQUMvCM+ve0ruNzt6R8Bnaayow= diff --git a/provider/custom_server.go b/provider/custom_server.go index ac49f029..0ca145e3 100644 --- a/provider/custom_server.go +++ b/provider/custom_server.go @@ -2,6 +2,7 @@ package provider import ( "context" + "encoding/json" "fmt" "html/template" "net" @@ -12,7 +13,6 @@ import ( "time" goauth2 "github.com/go-oauth2/oauth2/v4/server" - "github.com/microcosm-cc/bluemonday" "golang.org/x/oauth2" "github.com/go-pkgz/auth/avatar" @@ -160,16 +160,19 @@ func (c *CustomServer) handleUserInfo(w http.ResponseWriter, r *http.Request) { } userID := ti.GetUserID() - p := bluemonday.UGCPolicy() - ava := p.Sanitize(fmt.Sprintf(c.URL+"/avatar?user=%s", userID)) - res := fmt.Sprintf(`{ - "id": "%s", - "name":"%s", - "picture":"%s" - }`, userID, userID, ava) + user := token.User{ + ID: userID, + Name: userID, + Picture: fmt.Sprintf(c.URL+"/avatar?user=%s", url.QueryEscape(userID)), + } + res, err := json.Marshal(user) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } w.Header().Set("Content-Type", "application/json; charset=utf-8") - if _, err := w.Write([]byte(res)); err != nil { + if _, err := w.Write(res); err != nil { w.WriteHeader(http.StatusInternalServerError) return } diff --git a/provider/custom_server_test.go b/provider/custom_server_test.go index 527e1905..3083920f 100644 --- a/provider/custom_server_test.go +++ b/provider/custom_server_test.go @@ -43,6 +43,9 @@ func TestCustomProvider(t *testing.T) { L: logger.Std, } + var loginUsername string + var capturedUser token.User + sopts := CustomServerOpt{ URL: "http://127.0.0.1:9096", L: logger.Std, @@ -61,7 +64,7 @@ func TestCustomProvider(t *testing.T) { jar.SetCookies(u, r.Cookies()) form := url.Values{} - form.Add("username", "admin") + form.Add("username", loginUsername) form.Add("password", "pwd1234") req, err := http.NewRequest("POST", "", strings.NewReader(form.Encode())) @@ -87,9 +90,7 @@ func TestCustomProvider(t *testing.T) { claims, err := params.JwtService.Parse(resp.Cookies()[0].Value) assert.NoError(t, err) - assert.Equal(t, token.User{Name: "admin", ID: "admin", - Picture: "http://127.0.0.1:9096/avatar?user=admin", IP: ""}, *claims.User) - + capturedUser = *claims.User }, } @@ -120,6 +121,7 @@ func TestCustomProvider(t *testing.T) { client := &http.Client{Jar: jar, Timeout: time.Second * 10} // check non-admin, permanent + loginUsername = "admin" resp, err := client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site") require.Nil(t, err) assert.Equal(t, 200, resp.StatusCode) @@ -127,6 +129,8 @@ func TestCustomProvider(t *testing.T) { assert.NoError(t, err) t.Logf("resp %s", string(body)) t.Logf("headers: %+v", resp.Header) + assert.Equal(t, token.User{Name: "admin", ID: "admin", + Picture: "http://127.0.0.1:9096/avatar?user=admin", IP: ""}, capturedUser) // check avatar resp, err = client.Get("http://127.0.0.1:9096/avatar?user=dev_user") @@ -137,6 +141,18 @@ func TestCustomProvider(t *testing.T) { assert.Equal(t, 960, len(body)) t.Logf("headers: %+v", resp.Header) + // check malicious user ID + loginUsername = "attack" + resp, err = client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site") + require.Nil(t, err) + assert.Equal(t, 200, resp.StatusCode) + body, err = io.ReadAll(resp.Body) + assert.NoError(t, err) + t.Logf("resp %s", string(body)) + t.Logf("headers: %+v", resp.Header) + // user ID in picture URL is encoded + assert.Equal(t, "http://127.0.0.1:9096/avatar?user=none%26attack%3Dvalue%22%3E%3Cscript%3Enasty+stuff%3C%2Fscript%3E", capturedUser.Picture) + // check default login page prov.LoginPageHandler = nil resp, err = client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site") @@ -196,10 +212,13 @@ func initGoauth2Srv(t *testing.T) *goauth2.Server { if r.ParseForm() != nil { return "", fmt.Errorf("no username and password in request") } - if r.Form.Get("username") != "admin" || r.Form.Get("password") != "pwd1234" { - return "", fmt.Errorf("wrong creds") + if r.Form.Get("username") == "admin" && r.Form.Get("password") == "pwd1234" { + return "admin", nil + } + if r.Form.Get("username") == "attack" && r.Form.Get("password") == "pwd1234" { + return "none&attack=value\">", nil } - return "admin", nil + return "", fmt.Errorf("wrong creds") }) srv.SetInternalErrorHandler(func(err error) (re *errors.Response) { diff --git a/provider/verify.go b/provider/verify.go index 23da8263..8b0a03dd 100644 --- a/provider/verify.go +++ b/provider/verify.go @@ -11,7 +11,6 @@ import ( "github.com/go-pkgz/rest" "github.com/golang-jwt/jwt" - "github.com/microcosm-cc/bluemonday" "github.com/go-pkgz/auth/avatar" "github.com/go-pkgz/auth/logger" @@ -134,9 +133,7 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { // GET /login?site=site&user=name&address=someone@example.com func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) { - user, address := r.URL.Query().Get("user"), r.URL.Query().Get("address") - user = e.sanitize(user) - address = e.sanitize(address) + user, address, site := r.URL.Query().Get("user"), r.URL.Query().Get("address"), r.URL.Query().Get("site") if user == "" || address == "" { rest.SendErrorJSON(w, r, e.L, http.StatusBadRequest, fmt.Errorf("wrong request"), "can't get user and address") @@ -150,7 +147,7 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) }, SessionOnly: r.URL.Query().Get("session") != "" && r.URL.Query().Get("session") != "0", StandardClaims: jwt.StandardClaims{ - Audience: e.sanitize(r.URL.Query().Get("site")), + Audience: site, ExpiresAt: time.Now().Add(30 * time.Minute).Unix(), NotBefore: time.Now().Add(-1 * time.Minute).Unix(), Issuer: e.Issuer, @@ -179,10 +176,10 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) Token string Site string }{ - User: user, - Address: address, + User: trim(user), + Address: trim(address), Token: tkn, - Site: r.URL.Query().Get("site"), + Site: site, } buf := bytes.Buffer{} if err = emailTmpl.Execute(&buf, tmplData); err != nil { @@ -212,14 +209,8 @@ Confirmation for {{.User}} {{.Address}}, site {{.Site}} Token: {{.Token}} ` -func (e VerifyHandler) sanitize(inp string) string { - p := bluemonday.UGCPolicy() - res := p.Sanitize(inp) - res = template.HTMLEscapeString(res) - res = strings.ReplaceAll(res, "&", "&") - res = strings.ReplaceAll(res, """, "\"") - res = strings.ReplaceAll(res, "'", "'") - res = strings.ReplaceAll(res, "\n", "") +func trim(inp string) string { + res := strings.ReplaceAll(inp, "\n", "") res = strings.TrimSpace(res) if len(res) > 128 { return res[:128] diff --git a/provider/verify_test.go b/provider/verify_test.go index 47796725..291c8928 100644 --- a/provider/verify_test.go +++ b/provider/verify_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -59,7 +60,7 @@ func TestVerifyHandler_LoginSendConfirm(t *testing.T) { assert.Equal(t, "test", e.Name()) } -func TestVerifyHandler_LoginSendConfirmRejected(t *testing.T) { +func TestVerifyHandler_LoginSendConfirmEscapesBadInput(t *testing.T) { emailer := mockSender{} e := VerifyHandler{ @@ -77,20 +78,22 @@ func TestVerifyHandler_LoginSendConfirmRejected(t *testing.T) { handler := http.HandlerFunc(e.LoginHandler) rr := httptest.NewRecorder() - badUser := "%3C%21DOCTYPE%20html%3E%20%0A%3Chtml%3E%20%0A%3Chead%3E%0A%3Cmeta%20name%3D%22viewport%22%20content%3D%22width%3Ddevice-width%2C%20initial-scale%3D1%22%3E%0A%3Ctitle%3E%20Login%20Page%20%3C%2Ftitle%3E%0A%3Cstyle%3E%20%0ABody%20%7B%0A%20%20font-family%3A%20Calibri%2C%20Helvetica%2C%20sans-serif%3B%0A%20%20background-color%3A%20pink%3B%0A%7D%0Abutton%20%7B%20%0A%20%20%20%20%20%20%20background-color%3A%20%234CAF50%3B%20%0A%20%20%20%20%20%20%20width%3A%20100%25%3B%0A%20%20%20%20%20%20%20%20color%3A%20orange%3B%20%0A%20%20%20%20%20%20%20%20padding%3A%2015px%3B%20%0A%20%20%20%20%20%20%20%20margin%3A%2010px%200px%3B%20%0A%20%20%20%20%20%20%20%20border%3A%20none%3B%20%0A%20%20%20%20%20%20%20%20cursor%3A%20pointer%3B%20%0A%20%20%20%20%20%20%20%20%20%7D%20%0A%20form%20%7B%20%0A%20%20%20%20%20%20%20%20border%3A%203px%20solid%20%23f1f1f1%3B%20%0A%20%20%20%20%7D%20%0A%20input%5Btype%3Dtext%5D%2C%20input%5Btype%3Dpassword%5D%20%7B%20%0A%20%20%20%20%20%20%20%20width%3A%20100%25%3B%20%0A%20%20%20%20%20%20%20%20margin%3A%208px%200%3B%0A%20%20%20%20%20%20%20%20padding%3A%2012px%2020px%3B%20%0A%20%20%20%20%20%20%20%20display%3A%20inline-block%3B%20%0A%20%20%20%20%20%20%20%20border%3A%202px%20solid%20green%3B%20%0A%20%20%20%20%20%20%20%20box-sizing%3A%20border-box%3B%20%0A%20%20%20%20%7D%0A%20button%3Ahover%20%7B%20%0A%20%20%20%20%20%20%20%20opacity%3A%200.7%3B%20%0A%20%20%20%20%7D%20%0A%20%20.cancelbtn%20%7B%20%0A%20%20%20%20%20%20%20%20width%3A%20auto%3B%20%0A%20%20%20%20%20%20%20%20padding%3A%2010px%2018px%3B%0A%20%20%20%20%20%20%20%20margin%3A%2010px%205px%3B%0A%20%20%20%20%7D%20%0A%20%20%20%20%20%20%0A%20%20%20%0A%20.container%20%7B%20%0A%20%20%20%20%20%20%20%20padding%3A%2025px%3B%20%0A%20%20%20%20%20%20%20%20background-color%3A%20lightblue%3B%0A%20%20%20%20%7D%20%0A%3C%2Fstyle%3E%20%0A%3C%2Fhead%3E%20%20%0A%3Cbody%3E%20%20%0A%20%20%20%20%3Ccenter%3E%20%3Ch1%3E%20Student%20Login%20Form%20%3C%2Fh1%3E%20%3C%2Fcenter%3E%20%0A%20%20%20%20%3Cform%3E%0A%20%20%20%20%20%20%20%20%3Cdiv%20class%3D%22container%22%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Clabel%3EUsername%20%3A%20%3C%2Flabel%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22text%22%20placeholder%3D%22Enter%20Username%22%20name%3D%22username%22%20required%3E%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Clabel%3EPassword%20%3A%20%3C%2Flabel%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22password%22%20placeholder%3D%22Enter%20Password%22%20name%3D%22password%22%20required%3E%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cbutton%20type%3D%22submit%22%3ELogin%3C%2Fbutton%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22checkbox%22%20checked%3D%22checked%22%3E%20Remember%20me%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cbutton%20type%3D%22button%22%20class%3D%22cancelbtn%22%3E%20Cancel%3C%2Fbutton%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20Forgot%20%3Ca%20href%3D%22%23%22%3E%20password%3F%20%3C%2Fa%3E%20%0A%20%20%20%20%20%20%20%20%3C%2Fdiv%3E%20%0A%20%20%20%20%3C%2Fform%3E%20%20%20%0A%3C%2Fbody%3E%20%20%20%0A%3C%2Fhtml%3E%0A%0A%20%0A" - req, err := http.NewRequest("GET", "/login?address=blah@user.com&user="+badUser+"&site=remark42", http.NoBody) + badData := "<escaped>" + req, err := http.NewRequest("GET", "/login?address=blah@user.com&user="+url.QueryEscape(badData)+"&site="+url.QueryEscape(badData), http.NoBody) require.NoError(t, err) handler.ServeHTTP(rr, req) assert.Equal(t, 200, rr.Code) assert.Equal(t, "blah@user.com", emailer.to) - assert.Contains(t, emailer.text, "Password : blah@user.com remark42 token:") + expectedEscaped := "<html><script>nasty stuff</script>&lt;escaped&gt;</html>" + assert.Contains(t, emailer.text, expectedEscaped+" blah@user.com "+expectedEscaped+" token:") tknStr := strings.Split(emailer.text, " token:")[1] tkn, err := e.TokenService.Parse(tknStr) assert.NoError(t, err) t.Logf("%s %+v", tknStr, tkn) - assert.Equal(t, "<h1> Student Login Form </h1> <div> Username : Password : ::blah@user.com", tkn.Handshake.ID) - assert.Equal(t, "remark42", tkn.Audience) + // not escaped in these fields as they are not rendered as HTML + assert.Equal(t, badData+"::blah@user.com", tkn.Handshake.ID) + assert.Equal(t, badData, tkn.Audience) assert.True(t, tkn.ExpiresAt > tkn.NotBefore) assert.Equal(t, "test", e.Name())