Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url(...) style injections in cosmetic filters #668

Closed
8 tasks done
antonok-edm opened this issue Jul 12, 2019 · 2 comments
Closed
8 tasks done

url(...) style injections in cosmetic filters #668

antonok-edm opened this issue Jul 12, 2019 · 2 comments
Labels
bug Something isn't working fixed issue has been addressed

Comments

@antonok-edm
Copy link

Prerequisites

  • I verified that this is not a filter issue
  • This is not a support issue or a question
  • I performed a cursory search of the issue tracker to avoid opening a duplicate issue
    • Your issue may already be reported.
  • I tried to reproduce the issue when...
    • uBlock Origin is the only extension
    • uBlock Origin with default lists/settings
    • using a new, unmodified browser profile
  • I am running the latest version of uBlock Origin
  • I checked the documentation to understand that the issue I report is not a normal behavior

Description

There is a regex in the cosmetic filtering code to invalidate filter rules that contain url(. However, it's possible to bypass this check by using escape codes for any of those characters, e.g. \75 rl(.

A specific URL where the issue occurs

This can be applied to any URL, fetching a resource from any other URL. As a specific example, if the following rule is inserted into the filter list:

example.com##*:style(background: \75 rl("https://raw.githubusercontent.com/gorhill/uBlock/master/doc/img/icon38@2x.png"))

Visiting https://example.com will display the uBlock Origin logo all over the page.

Steps to Reproduce

  1. Open the "My filters" tab in the uBlock Origin dashboard
  2. Insert the following rule into the large input area on a new line:
example.com##*:style(background: \75 rl("https://raw.githubusercontent.com/gorhill/uBlock/master/doc/img/icon38@2x.png"))
  1. Click "Apply changes"
  2. Navigate to https://example.com in a new tab

Expected behavior:

The rule should be treated the same as if it did not use an escape sequence, i.e. ignored.

Actual behavior:

The rule is applied to the page, causing an additional network request to fetch the image resource from a 3rd party (in this case, Github).

Your environment

  • uBlock Origin version: v1.20.0
  • Browser Name and version: Confirmed on Brave 0.66.99 Chromium: 75.0.3770.100, Firefox Nightly 69.0a1
  • Operating System and version: Arch Linux

More info

This particular line contains the existing, insufficient check for the presence of url(: https://github.com/gorhill/uBlock/blob/8d8336ffae6ddfa44faaea77cbe5d476109fd007/src/js/static-ext-filtering.js#L668

@uBlock-user uBlock-user added the bug Something isn't working label Jul 12, 2019
gorhill added a commit to gorhill/uBlock that referenced this issue Jul 13, 2019
@gorhill
Copy link
Member

gorhill commented Jul 13, 2019

Thanks for the report.

For your interest @ameshkov, Adguard also suffers this issue. On my side I just outright banned the use of \ in CSS styles.

@ameshkov
Copy link

@gorhill huh, yep, thanks for mentioning it!

@uBlock-user uBlock-user added the fixed issue has been addressed label Jul 13, 2019
antonok-edm added a commit to brave/adblock-rust that referenced this issue Aug 2, 2019
antonok-edm added a commit to brave/adblock-rust that referenced this issue Aug 2, 2019
antonok-edm added a commit to brave/adblock-rust that referenced this issue Oct 28, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Oct 28, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Oct 28, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Oct 28, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Oct 29, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Nov 18, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
antonok-edm added a commit to brave/adblock-rust that referenced this issue Nov 18, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
AndriusA pushed a commit to brave/adblock-rust that referenced this issue Nov 22, 2019
Rather than pulling in an entire CSS styling library as a dependency,
this ensures that the syntax of supplied CSS selectors is valid, without
concern for the textual content of individual tokens. As new
pseudo-selectors are added to the W3C spec, this implementation should
still function as intended.

add tests for bad CSS selector inputs

protect against urls appearing in `:style()` filters

See uBlockOrigin/uBlock-issues#668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

4 participants