Closed Bug 1139849 Opened 5 years ago Closed 4 years ago

postMessage to incorrect target domain should print a console security error

Categories

(Core :: DOM: Core & HTML, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: stomlinson-pto, Assigned: jrburke, Mentored)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(4 files, 3 obsolete files)

It would be useful to let a developer know when invoking postMessage with a target that does not match the target window. In this scenario, the postMessage will not occur, but the developer/user will be left in the dark. 

Chrome notifies users via their developer console. It would be nice if we did as well, possibly under the Security section of the console.
I believe this has to be handled in the DOM code. If an error message is generated there, the web console will happily display it.
Component: Developer Tools: Console → DOM
Product: Firefox → Core
Yeah.  For whoever wants to work on this, the idea is that in nsGlobalWindow.cpp in PostMessageEvent::Run, where we do this:

8160     if (!targetPrin->Equals(mProvidedPrincipal)) {
8161       return NS_OK;
8162     }

we should log a console error using nsContentUtils::ReportToConsole.  Presumably one associated with the window id of the incumbent global at the callsite to postMessage (so we'll need to cart along the nsIDocument involved in the PostMessageEvent, and null it out after this point in Run ideally).

I'm happy to mentor anyone who wants to work on this one.
Mentor: bzbarsky
Whiteboard: [lang=c++][good first bug]
+1, would love to see this. cc'ing Stephen as we've been discussing security-related messages in the console.
Hey,
I would like to work on this bug.
Do you have any questions after look through the references in comment 2?
Thanks for the cc Jeff. Looks like Vaibhav has this with bz as mentor, right?

cc'ing Christoph to keep it on his radar.
Hi! I would like to work on this bug. Some naive questions following comment 2:
 - How do I get the nsIDocument involved in the PostMessageEvent?
 - What is meant by "incumbent global"?

Thanks!
Flags: needinfo?(bzbarsky)
> - How do I get the nsIDocument involved in the PostMessageEvent?

You need to pass it in from the code that creates the event.

> - What is meant by "incumbent global"?

The global of the code that made the postMessage call.  See https://html.spec.whatwg.org/multipage/webappapis.html#incumbent-settings-object for the spec bits, but they're not that informative.

For your purposes, if you look at nsGlobalWindow::PostMessageMoz, it has a callerInnerWindow (which may be null) which it gets the callerPrin from.  What you want to do is to get an nsIDocument (via ->GetDoc()) from it as well and pass that to the event.  If callerInnerWin is null, you won't be able to get a document, of course, but that's OK; ReportToConsole can handle a null document.

Does that help?
Flags: needinfo?(bzbarsky)
Attached patch bug1139849.patch (obsolete) — Splinter Review
not a final patch. I just would like to know, if I am on the right path...
Attachment #8582694 - Flags: review?(josh)
Comment on attachment 8582694 [details] [diff] [review]
bug1139849.patch

That's the right idea, but it needs to handle callerInnerWin being null, you want nsCOMPtr, not nsRefPtr, and you want to null out mDocument once you're done with it, to avoid the event entraining it.
Attachment #8582694 - Flags: review?(josh)
Attached patch bug1139849.patch (obsolete) — Splinter Review
There is something not working as expected. I get "undefined" if a run this in the webconsole: 
window.postMessage("BlaBla","http://example.org");
Do I receive the error message only if I have built firefox with the en-US locale?
Attachment #8582694 - Attachment is obsolete: true
Attachment #8585072 - Flags: review?(past)
> Do I receive the error message only if I have built firefox with the en-US locale?

It's possible; I'm not sure localization falls back to the en-US string if it can't find the one in the current langpack.... :(

You should be able to run against a clean profile with no langpacks installed to check, I'd think.

One other note: the GetURI() of a principal might be null, so the code needs to handle that.  And we still want to null out mDocumence once we're done.
Attachment #8585072 - Flags: review?(past)
Hi, my name is Josh. I am studying the Open-Source Development module at Coventry University and would like to work on this bug as my "First Bug".

Would that be okay?

Regards
Yes, absolutely.  Please feel free to mail me directly if you run into any problems!  You probably want to start by picking up the patch that's already in this bug.
James, did you mean to request review from someone?
Flags: needinfo?(jrburke)
I apologize for not asking to work on this bug first. I am new to the Gecko side of things and reviewboard, and was trying a bug out, including pushing to reviewboard, as I saw it in an info meeting about contributing. I was so focused on confirming the right steps for the reviewboard part that it seems to have attached to this bug.

I am happy to obsolete this change though if someone else was working on it, or if I messed up with reviewboard. I will set the r? flag though shortly, forgot to set that up correctly before a push.

This attachment shows how the error appears in my local build, and it matches the word style and placement of the different origins with the Chrome error. This will hopefully make it easier for people searching on the internet for the error message to find better, consolidated results.

Some other notes on the changeset:

* builds on the previous patch bug.
* Moved the code to PostMessageEvent.h/cpp, since the code seemed to have moved since last patch.
* Used nsIScriptError::errorFlag instead of warningFlag, since that also matches better with the Chrome behavior.
* Used origin instead of URI in the message, as that matches to what is being tested, matches the Chrome result too.
* Sets mSourceDocument = nullptr after the principal check is done.
* I tested locally by setting targetOrigin = nullptr to see if it had problems, but it seemed to still work, just printed '' in the error message.
Flags: needinfo?(jrburke)
Comment on attachment 8712977 [details]
MozReview Request: Bug 1139849 - postMessage to incorrect target domain should print a console security error

Setting review flag to :bz as he is a mentor on this bug, but happy to retarget to lessen review load.
Attachment #8712977 - Flags: review?(bzbarsky)
James, no one is actively working on this right now that I know of.  So no worries on that score.  ;)

Thank you for working on it!  I'll do the review tomorrow morning (US Eastern time).
Comment on attachment 8712977 [details]
MozReview Request: Bug 1139849 - postMessage to incorrect target domain should print a console security error

https://reviewboard.mozilla.org/r/32731/#review29609

Sorry for the lag; some urgent things came up this morning.  :(

::: dom/base/PostMessageEvent.cpp:98
(Diff revision 1)
> +      nsresult rv = targetPrin->GetOrigin(targetOrigin);

Instead of using nsIPrincipal::GetOrigin, which is an internal thing that does quite interesting things in some situations that would be confusing in an error message targeting web developers, I think this code should use nsContentUtils::GetUTFOrigin, which gets what the HTML spec defines as the "origin".

So like this:

  nsAutoString providedOrigin, targetOrigin;
  nsresult rv = nsContentUtils::GetUTFOrigin(targetPrin, targetOrigin);
  
And yes, I think you should call the thing you get from mProvidedPrincipal "providedOrigin".

Then you won't need to do extra UTF8-to-UTF16 conversions either.

::: dom/base/nsGlobalWindow.cpp:7972
(Diff revision 1)
> +                         !callerInnerWin

Probably better to reverse the sense of this check.  Up above it's the way it is because of the IsCallerChrome() bit in the boolean or, but here we've just got a simple condition, and starting it with ! is a bit odd.

r=me with those two nits fixed, and thank you for picking this up!

I'm not sure what the current status of mozreview is in terms of being able to push to it.  Once you have the updated patch, if you can't push it to mozreview please just attach it to the bug directly and I'll get it landed for you.
Attachment #8712977 - Flags: review?(bzbarsky) → review+
Attachment #8585072 - Attachment is obsolete: true
Assignee: nobody → jrburke
Status: NEW → ASSIGNED
Comment on attachment 8713483 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

I had some issue with MozReview/my local clone so ended up with this patch.

It includes the two changes around nsContentUtils::GetUTFOrigin and reversing the callerInnerWin? tertiary logic to place the positive case first.

Built and tested locally, the message appears the same as in the previously uploaded screenshot.
Flags: needinfo?(bzbarsky)
Comment on attachment 8713483 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

Actually, I just realized one more issue: If the principal check _fails_ we never null out mSourceDocument.

What we should probably do is put it in a stack variable at the top of Run(), null it out, and use the stack variable for the ReportToConsole call...
Ah right, I can do that, can post a fresh patch tomorrow.
Flags: needinfo?(bzbarsky)
Attachment #8713483 - Attachment is obsolete: true
Comment on attachment 8713803 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

Updated patch, the diff from the previous patch is just the stack variable for the source document. Built and run locally, still works the same:

diff --git a/dom/base/PostMessageEvent.cpp b/dom/base/PostMessageEvent.cpp
--- a/dom/base/PostMessageEvent.cpp
+++ b/dom/base/PostMessageEvent.cpp
@@ -54,16 +54,22 @@ PostMessageEvent::Run()
              "should have been passed an outer window!");
   MOZ_ASSERT(!mSource || mSource->IsOuterWindow(),
              "should have been passed an outer window!");
 
   AutoJSAPI jsapi;
   jsapi.Init();
   JSContext* cx = jsapi.cx();
 
+  // The document is just used for the principal mismatch error message below.
+  // Use a stack variable so mSourceDocument is not held onto after this method
+  // finishes, regardless of the method outcome.
+  nsIDocument* sourceDocument = mSourceDocument;
+  mSourceDocument = nullptr;
+
   // If we bailed before this point we're going to leak mMessage, but
   // that's probably better than crashing.
 
   RefPtr<nsGlobalWindow> targetWindow;
   if (mTargetWindow->IsClosedOrClosing() ||
       !(targetWindow = mTargetWindow->GetCurrentInnerWindowInternal()) ||
       targetWindow->IsClosedOrClosing())
     return NS_OK;
@@ -98,29 +104,25 @@ PostMessageEvent::Run()
       nsresult rv = nsContentUtils::GetUTFOrigin(targetPrin, targetOrigin);
       NS_ENSURE_SUCCESS(rv, rv);
       rv = nsContentUtils::GetUTFOrigin(mProvidedPrincipal, providedOrigin);
       NS_ENSURE_SUCCESS(rv, rv);
 
       const char16_t* params[] = { providedOrigin.get(), targetOrigin.get() };
 
       nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
-        NS_LITERAL_CSTRING("DOM Window"), mSourceDocument,
+        NS_LITERAL_CSTRING("DOM Window"), sourceDocument,
         nsContentUtils::eDOM_PROPERTIES,
         "TargetPrincipalDoesNotMatch",
         params, ArrayLength(params));
 
       return NS_OK;
     }
   }
 
-  // The document is just used for the principal mismatch check above, now it is
-  // no longer needed.
-  mSourceDocument = nullptr;
-
   ErrorResult rv;
   JS::Rooted<JS::Value> messageData(cx);
   nsCOMPtr<nsPIDOMWindow> window = targetWindow.get();
 
   Read(window, cx, &messageData, rv);
   if (NS_WARN_IF(rv.Failed())) {
     return rv.StealNSResult();
   }
>+  nsIDocument* sourceDocument = mSourceDocument;

That needs to be an nsCOMPtr<nsIDocument> so the object is kept alive.  So something like this:

  nsCOMPtr<nsIDocument> sourceDocument;
  sourceDocument.swap(mSourceDocument);
The tree was open for once, so I just made that change and pushed this.  Thank you again for the patch!
https://hg.mozilla.org/mozilla-central/rev/13d8daa68e24
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.