Broken abort-current-inline-script overrides

[agiammarchi]

Environment

ABP 3.5.1 test build
Google Chrome 75 (Canary)
macOS 10.12

Summary

Current abort-current-inline-script snippet cannot be used multiple times within the same document.

How to reproduce

Add these two filters:

127.0.0.1#$#abort-current-inline-script document.write atob
127.0.0.1#$#abort-current-inline-script document.write btoa

Spin any static web server with the attached test.html, open a new tab, open DevTools and in the Network section throttle to a slow connection ("Slow 3G" on Chrome and "Regular 2G" on Firefox), and load the test page.

Observed behaviour

Only one of the filters is applied (check console panel, it should show both) and the end of the document still shows "btoa: dGhpcyBpcyBhIGJ1Zw=="

Expected behaviour

Both filters should be applied; the end of the document should not show "btoa: dGhpcyBpcyBhIGJ1Zw=="

Additional notes

Since there is no mechanism to know if the same method was previously overwritten, the last time wrapPropertyAccess would return true is the only time the callback will be intercepted and its search the only one performed.

This works as expected with multiple iframes, but only because these share nothing with other frames.

[mjethani]

Can you update the "How to reproduce" section with an example?

[agiammarchi]

Added test case, hope that's enough. It's also pretty obvious in the code since any previous check would be overwritten and not stored anyhow.

We could set a special property in the method/property we overwrite but that'd be easily discoverable by anti-ab scripts.

[hfiguiere]

In the current code, the second snippet never overrides because we already have (property is not configurable anymore).

[agiammarchi]

Object.defineProperty does not change configurability unless explicitly set, but the issue is that the code doesn't allow two different checks for same property/accessor

[hfiguiere]

Replying to agiammarchi:

Object.defineProperty does not change configurability unless explicitly set, but the issue is that the code doesn't allow two different checks for same property/accessor

configurable is set to false by default. So unless I set to to true the property won't be configurable afterwards.

Yes I know what the original bug is about. I'm writing tests first to make sure we get everything right and commenting here.

[agiammarchi]

Replying to hfiguiere:

configurable is set to false by default. So unless I set to to true the property won't be configurable afterwards.

that works only if the property is not own, but we don't check for that, and yet, that's not the whole issue.

const obj = {test: 123};
Object.defineProperty(obj, 'test', {value: 456});
obj.test; // 456
Object.defineProperty(obj, 'test', {value: 789});
obj.test; // 789

That is what I meant. We should have both own or not own checks, or explicit configurable: false if that's what we expect, and yet we cannot indeed override same property twice becasue we don't store the previous thing to check anywhere.

I keep believing we need persistency in these cases, but the ticket I've opened is not getting the attention I hoped it deserved.

[mjethani]

Replying to agiammarchi:

I keep believing we need persistency in these cases, but the ticket I've opened is not getting the attention I hoped it deserved.

I have a long triage queue unfortunately but I hope I can get around to it at some point.

In terms of priorities in general, if a theoretical issue is also a practical problem that someone is running into, it gets a priority; if it is merely a theoretical issue that someone might run into in the future, by default it stays with no priority assigned in the issue tracker so we can reference it from other tickets and go back to it when it becomes a real concern. I think it is always good to log such tickets.

[mjethani]

@agiammarchi could you please add a reproducible test case and also mention the environment (ABP, browser, and OS versions)?

I'm not sure how you are running into this issue because it is really hard to reproduce it for the following reasons:

1. Inline <script>s are executed before our wrapper has a chance to wrap the properties
2. abort-current-inline-script does not work with setTimeout() because ​document.currentScript is null in a timeout handler (or any asynchronous code)
3. abort-current-inline-script does not work with external scripts because document.currentScript.textContent is blank

[mjethani]

Replying to mjethani:

@agiammarchi could you please add a reproducible test case and also mention the environment (ABP, browser, and OS versions)?

I'm not sure how you are running into this issue because it is really hard to reproduce it for the following reasons [...]

To make it even worse, abort-current-inline-script doesn't even work with inline scripts that are executed after the code has been injected, because document.currentScript points to the injected <script> element, at least on Canary (version 75).

I am not sure how this snippet ever worked.

[mjethani]

I have filed #7421 and #7422 for the issues mentioned in the preceding comments.

[agiammarchi]

Replying to mjethani:

I'm not sure how you are running into this issue

The issue is that, as described in the ticket, if we set for the same site two check for the document.write only the first works (without any guarantees of success, since we don't check own property).

function wrapPropertyAccess(object, property, descriptor)
{
  // if it's own descriptor and is not configurable
  // meaning: if we set a wrap already, next time this will return false
  // and nothing will happen
  let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
  if (currentDescriptor && !currentDescriptor.configurable)
    return false;

  Object.defineProperty(object, property, descriptor);
  return true;
}

That rule wraps document.write and aborts only if, as example, atob is part of the text.

However, if another script has document.write and btoa in its content, the snippet, for that site, will ignore the second check because the wrap will set own write and the loop will silently return.

The reason me and Amr encountered this issue is that the same site was using different techniques and in some case they were using atob but in some other they were using btoa.

It is true that we could use a RegExp instead, but the snippet is not explicitly described as one that can be used only once per property.

The document.write is not an own property of the document, and us setting it via a descriptor that doesn't explicit the configurable: true are shooting our own foot making the snippet unable to override anything, that is not own property and configurable, twice.

Is this any more clear now?

If this is expected we're asking filters maintainers to coordinates to never use the same target, i.e. document.write, twice.

Is this intended?

[agiammarchi]

@mjethani as counter verification of what I am saying, this is basically our code, and the override is only one (I'd expect two).

You can copy and paste this on any browser console:

function abortCurrentInlineScript(api, search = null)
{
  let object = window;
  let path = api.split(".");
  let name = path.pop();

  for (let node of path)
  {
    object = object[node];

    if (!object || !(typeof object == "object" || typeof object == "function"))
      return;
  }

  let currentValue = object[name];
  let descriptor = {
    get()
    {
      return currentValue;
    },
    set(value)
    {
      currentValue = value;
    }
  };

  if (wrapPropertyAccess(object, name, descriptor))
    overrideOnError();
}

function wrapPropertyAccess(object, property, descriptor)
{
  let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
  if (currentDescriptor && !currentDescriptor.configurable)
    return false;

  Object.defineProperty(object, property, descriptor);
  return true;
}

let overrides = 0;
function overrideOnError()
{
  overrides++;
}

// test it for real
abortCurrentInlineScript('document.write', 'atob');
abortCurrentInlineScript('document.write', 'btoa');
console.assert(overrides === 2, `Expected 2 overrides, got: ${overrides}`);

[mjethani]

I understand, but we need a standalone test case to be able to verify the fix at least.

[agiammarchi]

Replying to mjethani:

I understand, but we need a standalone test case to be able to verify the fix at least.

The stand alone test case is that if we encounter this:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
</head>
<body>
  <script>
  document.write('<p>btoa: ' + btoa('this is a bug') + '</p>');
  </script>
</body>
</html>

but we set this:

127.0.0.1#$#abort-current-inline-script document.write atob
127.0.0.1#$#abort-current-inline-script document.write btoa

the second check should not hit because the get/setter is already set for atob which is not present.

I'd expect it to hit. Should I change the ticket code to reflect this?

[agiammarchi]

Unrelated, but worth replying:

Replying to mjethani:

In terms of priorities in general, if a theoretical issue is also a practical problem that someone is running into, it gets a priority; if it is merely a theoretical issue that someone might run into in the future, by default it stays with no priority assigned in the issue tracker so we can reference it from other tickets and go back to it when it becomes a real concern. I think it is always good to log such tickets.

The issue is so practical we keep patching via N function wrappers instead of using a persistent state. This very same bug, the fetch query string snippet, and others we're patching these days, have all the same pattern: we don't know if we operated already in the page, so we wrap functions just for the sake of it, causing possible side effects if the page also wrapped what we wrap (i.e. they can detect us or we trigger callbacks we don't own).

Maybe I should expand in there how important this is, but my main concern is that if we keep ignoring it, the refactoring we'll need once we'll hit such issue, will be 10x more painful.

As summary: we have already use cases for persistent data, and we have sites easily detecting, or circumventing us, through setters and wrappers, or even poisoning the DOM env (i.e. overwriting the textContent of the DOM to avoid us testing content).

These are most reasons I wish we'd find already a better way to both secure our own code, and have a persistent, and private, state.

[mjethani]

Replying to agiammarchi:

The stand alone test case is that if we encounter this [...]

As I explained in comment:15 and comment:16, I am unable to reproduce the issue with this test case. Are you actually able to reproduce the issue with your standalone test case? Please don't post test cases that you have not tried yourself. In the current test case in the description, neither of the snippet filters work, as I explained in comment:16.

If you are able to reproduce the issue, please add your platform details (as I also asked in comment:15) to the "Environment" section of the issue description.

[agiammarchi]

Replying to mjethani:

Please don't post test cases that you have not tried yourself.

My test case is a meta example, and it cannot work because AFAIK our code runs after that content.

However, in the real world, I've been able to reproduce it.

Whatever way we have in place already to test the snippet (i.e. regression tests) we should use the exact same test just adding same property check with a different content before the one that is currently working.

I am, unfortunately, not super confident in where we keep these regression tests, otherwise I would've posted that one.

[hfiguiere]

Replying to mjethani:

I understand, but we need a standalone test case to be able to verify the fix at least.

the test included with the patch (in review) isn't enough?

[Ross]

Replying to mjethani:

You're correct with those observations. However I think the snippet is sometimes in place before the inline scripts as my local test case (below) works half of the time (seems race condition-y). But I was under the impression I was supposed to expect that sort of behaviour between inline scripts and snippets.

One other thing I also noticed is that it really depends on what the inline script does or calls and how busy the page is. If the inline script is just document.querySelector("#foo"); on a empty page, that will almost always run before the snippet. However if the script does slightly more, like below, and there is more to the page (like on the testpages), the snippet works as expected.

<p id="target">Not run yet</p>

<script>
  let target = document.getElementById("target");
  target.innerHTML = "Script was aborted";
  target.className = "target-green";

  document.querySelector("#foo");

  target.innerHTML = "Script was not aborted";
  target.className = "target-red";
</script>

[mjethani]

It seems #7421 is not a bug; either it was a temporary issue in Canary or I messed something else up locally.

I have added instructions to reproduce this issue reliably now.

[abpbot]

A commit referencing this issue has landed:
​Issue 7373 - Allow abort-current-inline-script overrides

[Ross]

Fixed. Working as described.

(Except on Edge, where the snippet doesn't work, which is known issue #7420)

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2