Remove bluemonday and fix double-escaping

[david-bezero]

Fixes #183

Uses json.Marshal to build a correctly encoded user structure (replacing fmt.Sprintf), removing bluemonday from this part of the code entirely as it is not required.

Also updates the email template code to avoid double-encoding everything, and removes bluemonday as it is not necessary (any tags entered are encoded by the template library anyway, as seen in the test, so there is no risk of injection)

This preserves the whitespace / trimming tidy-up steps for the email data.

[umputun]

I don't understnd how json marshal prevents injection of userID here. Even if it does somehow, it is missing a test demonstrating it.

[david-bezero]

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 src attribute, but then it's up to the consumer to escape it at that point. It could just as easily be passed in to a fetch or 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.

[umputun]

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.

[david-bezero]

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 .src attribute via javascript, or using fetch, 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.

[david-bezero]

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.URL is 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.

[david-bezero]

I have added url.QueryEscape around 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)

[umputun]

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.

[david-bezero]

the sanitize'd values are passed to 2 places:

• a handshake (which doesn't need them to be escaped or sanitised because it just handles them as a string)
• an email template builder (which does its own escaping)

[umputun]

@paskal - can you take a look pls? To me, it looks like the Sender called after html/template was applied, and it can be safe to avoid sanitizing here. However, as far as I recall, this code was added to address some potential vulnerabilities, and I don't recall many details about it anymore.

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?

[david-bezero]

if you decide double escaping is needed here (though I can't imagine why that would be), it can be achieved entirely though template.HTMLEscapeString without bluemonday being involved.

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 site parameter did not go through the extra process (so was only escaped once).

[david-bezero]

I did a little digging in to how these checks came about and found your comment @umputun here:

#119 (comment)

That explains why you were previously vulnerable to this attack, but are now double-encoding. The text -> html template change was the only required change in the end - it made the rest of the changes obsolete, and this PR effectively undoes those parts.

[umputun]

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.

[david-bezero]

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 badUser to badData because I added it to the site parameter as well (which previously wasn't being tested for how it handles malicious data)

[umputun]

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.

[david-bezero]

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.

[david-bezero]

I'm not sure what to make of the linting error but it's not related to anything that changed in this PR:

G101: Potential hardcoded credentials: RSA private key (gosec)

(file: provider/apple_test.go)

[paskal]

@david-bezero, please rebase from the master to fix tests. Are there any changes expected in this PR based on previous comments, or it's in the final state for the review?

[paskal]

@david-bezero, gentle ping for an answer.

[david-bezero]

hey. Sorry I don't have time lately to update this PR, but to answer your question as far as I'm aware this is in a good state to review.

I can maybe rebase it some time next week.

[paskal]

Everything makes sense to me as-is.
Apologies, I messed up something with rebasing against the master, so commits are not accidentally co-signed by me.

[umputun]

Could you pls resolve the conflict? I'm going to merge it in and release a new version as soon as it done

[david-bezero]

Sorry for the delay; I've been away over the holidays. Rebased and force-pushed.

[coveralls]

Pull Request Test Coverage Report for Build 7463747212

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 82.746%

---

Totals
Change from base Build 7265392742:	-0.2%
Covered Lines:	2561
Relevant Lines:	3095

---

💛 - Coveralls