Closed Bug 1441336 Opened 4 years ago Closed 4 years ago

PerformanceObserver CORS restrictions in WebExtensions Content scripts despite appropriate Host permissions

Categories

(WebExtensions :: General, defect, P2)

58 Branch
defect

Tracking

(firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified

People

(Reporter: mozilla+bugzilla, Assigned: zombie)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180208175058

Steps to reproduce:

Created fresh profile. Added WebExtension:

manifest.json:
----
"permissions": [
  "<all_urls>"
],
"content_scripts": [
  {
    "matches": ["<all_urls>"],
    "js": ["contentscript.js"],
    "run_at": "document_start",
    "all_frames": true
  }
]
----

contentscript.js
----
const observer = new PerformanceObserver((list) => {
  list.getEntries().map(entry => {
    if (!entry.name.startsWith('https://jsfiddle.net')) {
      console.log(entry.transferSize);
    }
  });
});
observer.observe({entryTypes: ['resource']});
---

Now navigate to https://jsfiddle.net/


Actual results:

The console logs only 0


Expected results:

The console should log the actual transferSize


Note:
This is probably because of the CORS restrictions for some properties provided by PerformanceObserver, including transferSize: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/transferSize

Since Host permissions should overwrite CORS restrictions I would assume that in this case the actual transferSize is provided.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: General
Ever confirmed: true
Priority: -- → P2
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Attachment #8965418 - Flags: review?(kmaglione+bmo)
Attachment #8965884 - Flags: review?(kmaglione+bmo)
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review240418

::: dom/performance/PerformanceResourceTiming.h:76
(Diff revision 5)
> -    return mTimingData && mTimingData->ShouldReportCrossOriginRedirect()
> -        ? mTimingData->RedirectStartHighRes(mPerformance)
> -        : 0;
> +    bool allowed = mTimingData && 
> +        (mTimingData->ShouldReportCrossOriginRedirect() ||
> +          AllowedForCaller(aSubjectPrincipal));

This is repeated a lot. Maybe add a private helper method for it?

Also, please remove trailing space :)

::: dom/performance/PerformanceResourceTiming.h:95
(Diff revision 5)
> +    return allowed ? mTimingData->RedirectEndHighRes(mPerformance) : 0;
>    }
>  
> -  DOMHighResTimeStamp DomainLookupStart() const {
> -    return mTimingData && mTimingData->TimingAllowed()
> -        ? mTimingData->DomainLookupStartHighRes(mPerformance)
> +  DOMHighResTimeStamp DomainLookupStart(Maybe<nsIPrincipal*>& aSubjectPrincipal) const {
> +    bool allowed = mTimingData && 
> +        (mTimingData->TimingAllowed() || AllowedForCaller(aSubjectPrincipal));

Probably make this `TimingAllowed(aSubjectPrincipal)`

::: dom/performance/PerformanceResourceTiming.cpp:36
(Diff revision 5)
>    , mTimingData(Move(aPerformanceTiming))
>    , mPerformance(aPerformance)
>  {
>    MOZ_ASSERT(aPerformance, "Parent performance object should be provided");
> +
> +  NS_NewURI(getter_AddRefs(mURI), aName);

Can only do this on the main thread. But we don't have principals on worker threads, anyway, so maybe just check IsMainThread, and leave mURI null otherwise.
Attachment #8965418 - Flags: review?(kmaglione+bmo)
Comment on attachment 8965884 [details]
Bug 1441336 - Test addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234708/#review240420

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:20
(Diff revision 1)
> +      }],
> +    },
> +    files: {
> +      "cs.js"() {
> +        let obs = new window.PerformanceObserver(list => {
> +          list.getEntries().map(e => {

Nit: s/map/forEach/

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:30
(Diff revision 1)
> +
> +        let w = window;
> +        w.eval(`
> +          let b = document.createElement("link");
> +          b.rel = "stylesheet";
> +          b.href = "http://b.example.com/file_download.txt";

No eval. `w.wrappedJSObject.href = "...";`;

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:32
(Diff revision 1)
> +        w.eval(`
> +          let b = document.createElement("link");
> +          b.rel = "stylesheet";
> +          b.href = "http://b.example.com/file_download.txt";
> +          document.head.appendChild(b);
> +        

Nit: Trailing space.
Attachment #8965884 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8965884 [details]
Bug 1441336 - Test addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234708/#review240420

> No eval. `w.wrappedJSObject.href = "...";`;

Nice, didn't know that works.
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review240436

Thanks.

This also needs review from a DOM peer, of course.

::: dom/performance/PerformanceResourceTiming.h:174
(Diff revision 7)
>  
>    size_t
>    SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
>  
> +  bool
> +  TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller, bool aRedirect = false) const

Please document this, particularly the aRedirect parameter.

::: dom/performance/PerformanceResourceTiming.h:180
(Diff revision 7)
> +    if ((aRedirect && mTimingData->ShouldReportCrossOriginRedirect()) ||
> +        (!aRedirect && mTimingData->TimingAllowed())) {

This should probably be a ternery or a nested if `aRedirect ? ShouldReport() : TimingAllowed()`

::: dom/performance/PerformanceResourceTiming.cpp:36
(Diff revision 7)
>    , mTimingData(Move(aPerformanceTiming))
>    , mPerformance(aPerformance)
>  {
>    MOZ_ASSERT(aPerformance, "Parent performance object should be provided");
> +  if (NS_IsMainThread()) {
> +    NS_NewURI(getter_AddRefs(mURI), aName);

Please add a comment about why it's OK for this to be null off-main-thread.
Attachment #8965418 - Flags: review?(kmaglione+bmo) → review+
Attachment #8965418 - Flags: review?(bzbarsky)
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review240678

::: dom/performance/PerformanceResourceTiming.h:75
(Diff revision 8)
>          : 0;
>    }
>  
> -  DOMHighResTimeStamp RedirectStart() const {
> +  DOMHighResTimeStamp RedirectStart(Maybe<nsIPrincipal*>& aSubjectPrincipal) const {
>      // We have to check if all the redirect URIs had the same origin (since
>      // there is no check in RedirectEndHighRes())

This comment should be talking about RedirectStartHighRes (preexisting, I know).

::: dom/performance/PerformanceResourceTiming.h:189
(Diff revision 8)
> +
>    nsString mInitiatorType;
>    UniquePtr<PerformanceTimingData> mTimingData;
>    RefPtr<Performance> mPerformance;
> +
> +  nsCOMPtr<nsIURI> mURI;

What is this member, exactly?  Which URI?

::: dom/performance/PerformanceResourceTiming.cpp:38
(Diff revision 8)
>  {
>    MOZ_ASSERT(aPerformance, "Parent performance object should be provided");
> +  if (NS_IsMainThread()) {
> +    // Used to check if an addon content script has access to this timing.
> +    // We don't need it in workers, and ignore mURI if null.
> +    NS_NewURI(getter_AddRefs(mURI), aName);

OK, so this looks like the initial pre-all-redirects URI, right?

::: dom/performance/PerformanceResourceTiming.cpp:106
(Diff revision 8)
> +      : mTimingData->TimingAllowed()) {
> +    return true;
> +  }
> +
> +  // Check if the addon has permission to access the cross-origin resource.
> +  return mURI && aCaller.isSome() &&

Why do we not need to check that the addon can access _all_ URIs in the redirect chain?

::: dom/webidl/PerformanceResourceTiming.webidl:20
(Diff revision 8)
>  {
>    readonly attribute DOMString initiatorType;
>    readonly attribute DOMString nextHopProtocol;
>  
>    readonly attribute DOMHighResTimeStamp workerStart;
> +  readonly attribute DOMHighResTimeStamp fetchStart;

Please don't change the order here.  The old order matched the spec IDL, and we should continue doing so.  If nothing else, changing the order here leads to web-observably different enumeration behavior!
Attachment #8965418 - Flags: review?(bzbarsky)
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review241046

::: dom/performance/PerformanceResourceTiming.cpp:38
(Diff revision 8)
>  {
>    MOZ_ASSERT(aPerformance, "Parent performance object should be provided");
> +  if (NS_IsMainThread()) {
> +    // Used to check if an addon content script has access to this timing.
> +    // We don't need it in workers, and ignore mURI if null.
> +    NS_NewURI(getter_AddRefs(mURI), aName);

Yes, per the spec for the `name` property, that's `originalURI` from:

https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#56-64

I can rename to make that more explicit.

::: dom/performance/PerformanceResourceTiming.cpp:106
(Diff revision 8)
> +      : mTimingData->TimingAllowed()) {
> +    return true;
> +  }
> +
> +  // Check if the addon has permission to access the cross-origin resource.
> +  return mURI && aCaller.isSome() &&

This needs a longer explanation:

(from my reading and current understanding of Performance Timing code)

There is a big mismatch between the current Performance Timing implementation, and what we would want for this bug.  Understandably, all of cross-origin decisions are performed upon creation of `PerformanceTimingData`[1], which is later passed to any and all observer callbacks.  And in the world without extensions, this makes sense, since all performance observers that can see a specific TimingData run with the same origin.

Extension content scripts obviously mess with this assumption, and since they can be added at any point, that whole approach is invalid, and to get the exact correct behavior would require a bigger refactoring of performance timing code -- something that would be above my head (or at least, which would take me too much time to be worth it).

So I figured, perhaps naively, since this is only exposing redirects _timing_ data, and for resources that an extension would otherwise have access to according to the originalURI, this would be acceptable (and extensions might have other ways to get those timings anyway).

I can drop that logic for redirect timings if you disagree with that, or if you see an easy way to work around this problem (without major rewrites), I'd be happy to do it.


1) https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#208-213

::: dom/webidl/PerformanceResourceTiming.webidl:20
(Diff revision 8)
>  {
>    readonly attribute DOMString initiatorType;
>    readonly attribute DOMString nextHopProtocol;
>  
>    readonly attribute DOMHighResTimeStamp workerStart;
> +  readonly attribute DOMHighResTimeStamp fetchStart;

Oops, sorry.
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review241046

> Yes, per the spec for the `name` property, that's `originalURI` from:
> 
> https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#56-64
> 
> I can rename to make that more explicit.

Documenting in addition to that would be even better.

> This needs a longer explanation:
> 
> (from my reading and current understanding of Performance Timing code)
> 
> There is a big mismatch between the current Performance Timing implementation, and what we would want for this bug.  Understandably, all of cross-origin decisions are performed upon creation of `PerformanceTimingData`[1], which is later passed to any and all observer callbacks.  And in the world without extensions, this makes sense, since all performance observers that can see a specific TimingData run with the same origin.
> 
> Extension content scripts obviously mess with this assumption, and since they can be added at any point, that whole approach is invalid, and to get the exact correct behavior would require a bigger refactoring of performance timing code -- something that would be above my head (or at least, which would take me too much time to be worth it).
> 
> So I figured, perhaps naively, since this is only exposing redirects _timing_ data, and for resources that an extension would otherwise have access to according to the originalURI, this would be acceptable (and extensions might have other ways to get those timings anyway).
> 
> I can drop that logic for redirect timings if you disagree with that, or if you see an easy way to work around this problem (without major rewrites), I'd be happy to do it.
> 
> 
> 1) https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#208-213

> and for resources that an extension would otherwise have access to according to the originalURI

That's the question.  How do extensions handle access there when redirects are involved?
God, I hate the quoting behavior of mozreview, and the fact that there's no way to make it sane.  :(
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> That's the question.  How do extensions handle access there when redirects
> are involved?

In general, for each phase of the request, we check permissions against the final channel URI for that phase, and also against the loading principal. So, even if they don't have permissions for previous phases, they can see phases they do have permissions for.
OK.  So the point is webextensions try to hide information about the no-permissions phases, and this change would allow them for timing information for those phases...

I would prefer we not expose stuff to not exposing too much.
Comment on attachment 8965418 [details]
Bug 1441336 - Use addon permissions for PerformanceTiming properties

https://reviewboard.mozilla.org/r/234176/#review241502

::: dom/performance/PerformanceResourceTiming.h:176
(Diff revisions 8 - 9)
> -   * @return  True if the caller should have access to this timing data.
> -   */
>    bool
> -  TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller, bool aCheckRedirects = false) const;
> +  TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller) const;
> +
> +  bool

This should probably be documented somewhat in terms of what it's trying to enforce.
Attachment #8965418 - Flags: review?(bzbarsky) → review+
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c294177f4ec9
Use addon permissions for PerformanceTiming properties r=bz,kmag
https://hg.mozilla.org/integration/autoland/rev/0f1e4fe1eb4c
Test addon permissions for PerformanceTiming properties r=kmag
https://hg.mozilla.org/mozilla-central/rev/c294177f4ec9
https://hg.mozilla.org/mozilla-central/rev/0f1e4fe1eb4c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Verified as fixed in latest Nightly. I will attach a before(FF59.02)\after(61.0a1) screenshot where the fix can be seen
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.