Remove bluemonday and fix double-escaping #184
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,6 @@ import ( | |
|
|
||
| "github.com/go-pkgz/rest" | ||
| "github.com/golang-jwt/jwt" | ||
|
|
||
| "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&[email protected] | ||
| func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) { | ||
|
|
||
| user, address, site := r.URL.Query().Get("user"), r.URL.Query().Get("address"), r.URL.Query().Get("site") | ||
|
There was a problem hiding this comment. this one is unclear to me either. The PR comment says "bluemonday as it is not necessary (any tags entered are encoded by the template library anyway, so there is no risk of injection)" but this method is a handler, and nothing prevents bad actors to pass anything in. There was a problem hiding this comment. the
There was a problem hiding this comment. @paskal - can you take a look pls? To me, it looks like the This sanitization was added by me on 06/03/22 "sanitize both user and address in verified provider". Do you recall if it was the result of some finding in remark42 around the same time? There was a problem hiding this comment. if you decide double escaping is needed here (though I can't imagine why that would be), it can be achieved entirely though But note that if double escaping is required for some reason, that means there is an attack vector in the existing code because the free-valued There was a problem hiding this comment. I did a little digging in to how these checks came about and found your comment @umputun here: That explains why you were previously vulnerable to this attack, but are now double-encoding. The |
||
|
|
||
| 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: 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: trim(user), | ||
| Address: trim(address), | ||
| Token: tkn, | ||
| 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 trim(inp string) string { | ||
| res := strings.ReplaceAll(inp, "\n", "") | ||
| res = strings.TrimSpace(res) | ||
| if len(res) > 128 { | ||
| return res[:128] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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_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() | ||
| badData := "<html><script>nasty stuff</script><escaped></html>" | ||
|
There was a problem hiding this comment. I don't get why this test changed this way and the user is url-escaped as a part of the test. It looks like you don't expect badUser to be passed in, but there is nothing preventing it. There was a problem hiding this comment. I mostly changed this to make it easier to reason about what's going on. The old test value was very long, and had its own hard-coded URL encoding, so to keep the test simple I replaced it with a shorter demonstration of the attack it's trying to prevent, and used the built-in function to automatically URL-encode it. I renamed it from There was a problem hiding this comment. I am almost sure this long and ugly input resulted from some vulnerability report we tried to replicate and address. I have asked @paskal to check it out and cross-reference, I will take a closer look too. There was a problem hiding this comment. The previous "badUser" input is nothing more than a URL encoded copy of a phishing message: if it is allowed to render raw, the user sees the phishing content before the real content of the message, and is encouraged to enter their password into an input field (in a real attack this would presumably submit somewhere). As such, it's more of a demonstration of how this vulnerability could be used in a meaningful attack, than a carefully crafted string which triggers a vulnerability in itself. For the purpose of a test, I'd argue that keeping things simple is better. The much shorter input I swapped it with still tests the same attack vector, just without a full demonstration of the impact. |
||
| req, err := http.NewRequest("GET", "/[email protected]&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, "[email protected]", emailer.to) | ||
| expectedEscaped := "<html><script>nasty stuff</script>&lt;escaped&gt;</html>" | ||
| assert.Contains(t, emailer.text, expectedEscaped+" [email protected] "+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) | ||
| // not escaped in these fields as they are not rendered as HTML | ||
| assert.Equal(t, badData+"::[email protected]", tkn.Handshake.ID) | ||
| assert.Equal(t, badData, tkn.Audience) | ||
| assert.True(t, tkn.ExpiresAt > tkn.NotBefore) | ||
|
|
||
| assert.Equal(t, "test", e.Name()) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understnd how json marshal prevents injection of userID here. Even if it does somehow, it is missing a test demonstrating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't prevent html tags - it prevents backslashes and quotes (backslashes weren't caught with the old approach, so any ID ending with a backslash would break the JSON syntax)
the point is that HTML tags don't need to be escaped here: none of this stuff ends up as HTML (the picture might be rendered in a
srcattribute, but then it's up to the consumer to escape it at that point. It could just as easily be passed in to afetchor similar, which would need different escaping semantics, so escaping it here is the wrong location.for tests, this is covered by the existing tests, but a new one could be added with a backslash in the ID to prevent future regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds scary and means we are introducing a real attack vector to all the existing users who will have to handle potential injection via avatar's URL on some level. Avatars are here for only one reason - displaying them on web pages, and it is our responsibility to prevent possible attacks here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a possible option which preserves the "safety net" for users who forget to escape things in HTML themselves, without compromising functionality for users who do the right thing, would be to URL-encode the value (excluding
/but being sure to include",',<, and>). This will prevent any injection attacks if rendered as a raw string into HTML, without breaking use-cases where the value is used in non-html-source ways (e.g. passing to a.srcattribute via javascript, or usingfetch, or using a server-side request for the image)I'd personally argue that this safety net isn't necessary, as users who forget to escape the value will almost certainly have plenty of other vulnerabilities in their own code anyway, but it's available as an option which maintains correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering this further: since the user ID is being passed in as a query parameter in the avatar URL, it should be URL encoded anyway just for correctness. And since
c.URLis controlled by the developer, that part of the URL does not carry any risk.I'll add URL encoding for the userID parameter in this URL (which was missing before) to make it more correct, which will have a side-effect of making it safe even for users who forget to escape the string when injecting into raw HTML. I'll also add a test for this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added
url.QueryEscapearound the userID and added a test to confirm it handles the important special characters. Note that this fixes an existing issue where user IDs containing&would be interpreted incorrectly in this URL (though I don't think it was possible to use that fact for a full exploit; just breakage)