search

kheina-com / Blue-Blocker

Blocks all Twitter Blue verified users on twitter.com
Mozilla Public License 2.0
343 stars 28 forks source link

Rewrite EscapeHTML func to be safer #295

Closed rougetimelord closed 2 months ago

rougetimelord commented 4 months ago

Mozilla kinda had a point, we set the href attribute of an anchor tag to the escaped string. Therefore, we need to make sure to escape single and double quotes.

Definitely could be minimized slightly, but I want to make sure the reviewer understands what's happening fully. This is safe, it destroys tags, and does not allow you to escape the confines of an attribute in order to add tag level event handlers.

See #294 for more details.

  1. [x] ensure src/manifest.ts and package.json have the correct version number
  2. [x] use makefile to generate zips (make chrome, make firefox)
    • [x] chrome should be tested with npm run build
    • [ ] test firefox locally using zip
  3. [ ] merge to main
cooljeanius commented 4 months ago

I'll leave this one for @kheina to review; don't feel qualified to review it myself

bkh555 commented 3 months ago

Is there any update on this? Content Security Policies renders my basic XSS attempts useless though I don't see Mozilla budging on this since it's a fairly common issue that has easy fixes. In fact their doc page states that using innerHTML, like is done on pages/queue/index.ts, is likely to have your add-on rejected. https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML#security_considerations

cooljeanius commented 3 months ago

Is there any update on this? Content Security Policies renders my basic XSS attempts useless though I don't see Mozilla budging on this since it's a fairly common issue that has easy fixes. In fact their doc page states that using innerHTML, like is done on pages/queue/index.ts, is likely to have your add-on rejected. developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML#security_considerations

Well do you think that this PR would be an adequate solution for it? I'll approve it if you think it looks good

bkh555 commented 3 months ago

Well do you think that this PR would be an adequate solution for it? I'll approve it if you think it looks good

Since Mozilla stated that merely sanitizing the data will satisfy them, I think starting with that is a good idea. I'm not a maintainer, just wanted to throw out more information to help get this thing along. I would refer to https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html to be on the safe side.

OWASP recommends DOMPurify for HTML Sanitization.

If going the route of a dep is too much effort then this commit would do as well. I don't believe anything else is not escaped in this context other than quotes.

rougetimelord commented 3 months ago

Imo it's a non issue... but if we wanted to be extra sure they let us through, we could use DOMPurify within the modal creation function.

In terms of what this change does, it's basically the same as the old version but more explicit and removes attribute escapes.

On Wed, Jun 19, 2024 at 14:46 Brandon @.***> wrote:

Well do you think that this PR would be an adequate solution for it? I'll approve it if you think it looks good

Since Mozilla stated that merely sanitizing the data will satisfy them, I think starting with that is a good idea. I'm not a maintainer, just wanted to throw out more information to help get this thing along. I would refer to https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html to be on the safe side.

OWASP recommends DOMPurify https://github.com/cure53/DOMPurify for HTML Sanitization.

— Reply to this email directly, view it on GitHub https://github.com/kheina-com/Blue-Blocker/pull/295#issuecomment-2179493535, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADLFRYWUJ2FWM7DK7DOU55TZIH32NAVCNFSM6AAAAABIQFGG5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGQ4TGNJTGU . You are receiving this because you authored the thread.Message ID: @.***>

cooljeanius commented 3 months ago

Imo it's a non issue... but if we wanted to be extra sure they let us through, we could use DOMPurify within the modal creation function.

So do you want to do that in this PR, or save it for a separate one?

kheina commented 3 months ago

I've held off from pulling in third party libraries to avoid bloating the size of the addon. if we rewrite it we should just use the 3-4 line div method they provide.

that being said, I don't want to do either since it's unnecessary, but I will acquiesce if yall would like to change it for firefox approval reasons

cooljeanius commented 3 months ago

I've held off from pulling in third party libraries to avoid bloating the size of the addon. if we rewrite it we should just use the 3-4 line div method they provide.

that being said, I don't want to do either since it's unnecessary, but I will acquiesce if yall would like to change it for firefox approval reasons

I would like to change it for Firefox approval reasons (assuming that it's correct)

bkh555 commented 3 months ago

I would like to change it for Firefox approval reasons (assuming that it's correct)

The fix satisfies the conditions provided by OWASP, so I agree.

Encoding Type: HTML Entity Encoding Mechanism: Convert & to &amp;, Convert < to &lt;, Convert > to &gt;, Convert " to &quot;, Convert ' to &#x27;

(&#039; is appropriate as well, here)

cooljeanius commented 3 months ago

2. [ ] use makefile to generate zips (make chrome, make firefox)

...actually wait, I'm getting some errors when I try to do this step:

$ make chrome
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
$ make firefox
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
cooljeanius commented 3 months ago
  1. [ ] use makefile to generate zips (make chrome, make firefox)

...actually wait, I'm getting some errors when I try to do this step:

$ make chrome
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
$ make firefox
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.

This still happens; is that expected?

kheina commented 3 months ago
  1. [ ] use makefile to generate zips (make chrome, make firefox)

...actually wait, I'm getting some errors when I try to do this step:

$ make chrome
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
$ make firefox
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.

This still happens; is that expected?

I'll take a look after work

kheina commented 3 months ago
  1. [ ] use makefile to generate zips (make chrome, make firefox)

...actually wait, I'm getting some errors when I try to do this step:

$ make chrome
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
$ make firefox
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.

This still happens; is that expected?

this is not expected, are you running npm run build first?

cooljeanius commented 3 months ago
  1. [ ] use makefile to generate zips (make chrome, make firefox)

...actually wait, I'm getting some errors when I try to do this step:

$ make chrome
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.
$ make firefox
cat: build/manifest.json: No such file or directory
makefile:6: *** Extension version mismatch. manifest: , package.json: "0.4.3".  Stop.

This still happens; is that expected?

this is not expected, are you running npm run build first?

Ah ok, now it works...

cooljeanius commented 3 months ago

Zips from this PR: blue-blocker-chrome-0.4.3.zip blue-blocker-firefox-0.4.3.zip

rougetimelord commented 2 months ago

Going ahead with the merge so we can hopefully get 0.4.3 out soon 🤞🏻