Closed Bug 1450989 Opened 2 years ago Closed Last year

Form submission can happen sync while !IsSafeToRunScript() and trigger chrome script

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: bzbarsky, Assigned: edgar)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(2 files, 1 obsolete file)

See the stack in bug 1450171 comment 0.  The really relevant part is this:

#18 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1257:23
....
#21 nsBrowserStatusFilter::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:184:27
#22 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1315:3
....
#25 nsDocLoader::OnStartRequest(nsIRequest*, nsISupports*) src/uriloader/base/nsDocLoader.cpp:456:9
....
#34 nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, char16_t const*, nsTSubstring<char16_t> const&, nsIInputStream*, long, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, nsIPrincipal*) src/docshell/base/nsDocShell.cpp:13743:17
#35 mozilla::dom::HTMLFormElement::SubmitSubmission(mozilla::dom::HTMLFormSubmission*) src/dom/html/HTMLFormElement.cpp:782:23
#36 mozilla::dom::HTMLFormElement::FlushPendingSubmission() src/dom/html/HTMLFormElement.cpp:1577:5
#37 mozilla::dom::HTMLFormElement::BeforeSetAttr(int, nsAtom*, nsAttrValueOrString const*, bool) src/dom/html/HTMLFormElement.cpp:186:9

Ideally we would do those "submit the thing" bits off a scriptrunner or something...  Just need to make sure we do them with the pre-attr-change state.  Or capture things like target state more eagerly in submissions.
Once this is fixed, we should consider reinstating the assertion bug 1450171 is removing.
So the testcase from bug 1450171 also triggers two other asserts.

First, it hits the IsSafeToRunScript() assert in nsDocumentViewer::PermitUnloadInternal, coming from that nsDocShell::OnLinkClickSync call in the satck above.  The only good news I have there is that this is _BeforeSetAttr_, so we haven't started seriously messing with state there yet, I think.

Then it hits a similar assert in EventDispatcher::Dispatch when firing the beforeunload event.  Again, this is kinda sorta safe-ish, maybe.

Andred, do we have any cycles on the DOM team to look into changing this pending submission stuff to a model more like the spec's, where we capture the action and target as part of the form submission creation (which we don't now) and maybe can get rid of these sync "submit it now" calls?
Flags: needinfo?(overholt)
Flags: needinfo?(overholt)
Maybe Henri has time to take a look?
Flags: needinfo?(hsivonen)
Group: core-security
Keywords: sec-high
I'll take a look. Intentionally leaving myself needinfoed still.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> So the testcase from bug 1450171 also triggers two other asserts.

Looks like I don't have access to the bug or the test case.
Flags: needinfo?(bzbarsky)
Just cc'd you :)
Flags: needinfo?(bzbarsky)
Henri, it's been a while. Do you think you'll find the time soon or should we ask someone else?
Edgar, can you take a look at this (while Henri's on PTO)?
Flags: needinfo?(hsivonen) → needinfo?(echen)
(In reply to Andrew Overholt [:overholt] from comment #8)
> Edgar, can you take a look at this (while Henri's on PTO)?

Sure, I'll take a look. I don't have access to the bug 1450171, could you cc me there?
Flags: needinfo?(overholt)
(In reply to Edgar Chen [:edgar] from comment #9)
> Sure, I'll take a look. I don't have access to the bug 1450171, could you cc
> me there?

Done.
Flags: needinfo?(overholt)
Attached patch patch, v1 (obsolete) — Splinter Review
Flags: needinfo?(echen)
Assignee: nobody → echen
Attachment #8994128 - Flags: review?(bzbarsky)
Comment on attachment 8994128 [details] [diff] [review]
patch, v1

>+  if (!actionURL) {
>+    return NS_ERROR_FAILURE;

Won't this get propagated all the way out?

Specifically, if someone calls submit() on a form which is not in a composed doc, or on a form with an empty "action" attribute not in an HTML document, or whatnot then we will throw an exception where we used to just silently do nothing, no?

Similar in cases when GetActionURL returns various errors (e.g. CSP blocks the load, or the script security manager does).

Please make sure that these various cases (all the cases when GetActionURL can return failure for non-OOM reasons and all the cases when it can return success and a null actionURL) are tested by web platform tests and that we pass those tests.

>+    return mActionURL.get();

You don't need the .get()

r=me with the above bits fixed, though it's also worth checking whether the other places where we call FlushPendingSubmission correspond to anything in the spec....
Attachment #8994128 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Comment on attachment 8994128 [details] [diff] [review]
> patch, v1
> 
> >+  if (!actionURL) {
> >+    return NS_ERROR_FAILURE;
> 
> Won't this get propagated all the way out?
> 
> Specifically, if someone calls submit() on a form which is not in a composed
> doc, or on a form with an empty "action" attribute not in an HTML document,
> or whatnot then we will throw an exception where we used to just silently do
> nothing, no?

You are right. Returning an error here make submit() throw an exception, though I have a hard time to find an example that trigger getActionURL returns success with a null actionURL [1]
- For the not in a composed doc case, it bails out earlier in [1].
- We won't run into the checks for HTML document [2] since the action will not be an empty string (it will use document's URI if the action attribute is empty).

But we better not change the behavior, I will update a new patch to fix this.

> 
> Similar in cases when GetActionURL returns various errors (e.g. CSP blocks
> the load, or the script security manager does).

No behavior changes for the case that GetActionURL returns various errors, we used to propagate the errors out [3].

[1] https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/html/HTMLFormElement.cpp#554
[2] https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/html/HTMLFormElement.cpp#1671-1674
[3] https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/html/HTMLFormElement.cpp#690
> But we better not change the behavior, I will update a new patch to fix this.

OK, thanks.

If we have checks in there (like the HTML document checks) that are basically dead code, followups to remove them would be great!

> No behavior changes for the case that GetActionURL returns various errors, we used to propagate the errors out [3].

Great.  Could you file a followup for this case to update it to follow the spec?   Pretty sure per spec doing a submit() that gets blocked by CSP should not throw an exception from the submit() call and that we currently do.
Attached patch patch, v2Splinter Review
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> Great.  Could you file a followup for this case to update it to follow the
> spec?   Pretty sure per spec doing a submit() that gets blocked by CSP
> should not throw an exception from the submit() call and that we currently
> do.

Filed bug 1478395.
Comment on attachment 8994878 [details] [diff] [review]
patch, v2

[Security approval request comment]

How easily could an exploit be constructed based on the patch?
A: It's not easy to exploit from the fix.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A: None.

Which older supported branches are affected by this flaw?
A: All supported branches.

If not all supported branches, which bug introduced the flaw?
A: None.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
A: No. It is not too complex, but it seems to me that it is not necessary to backport to older branches.

How likely is this patch to cause regressions; how much testing does it need?
A: It pass all existing tests.
Attachment #8994878 - Flags: sec-approval?
sec-approval+ for trunk. We'll want Beta and ESR560 patches made and nominated as well.
Attachment #8994878 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08753fa05058
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #8994128 - Attachment is obsolete: true
Please nominate this for Beta and ESR60 approval. It grafts cleanly to both branches as-landed.
Flags: needinfo?(echen)
Comment on attachment 8994878 [details] [diff] [review]
patch, v2

Approval Request Comment
[Feature/Bug causing the regression]:
None

[User impact if declined]:
Something insecure or unsafe could happen during form submission.

[Is this code covered by automated tests?]:
There are tests for form submission.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
The testcase from bug 1450171.
 
[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
The change is small and it has been in Nightly for a while.

[String changes made/needed]:
None
Flags: needinfo?(echen)
Attachment #8994878 - Flags: approval-mozilla-beta?
Patch for ESR60
Comment on attachment 8998102 [details] [diff] [review]
[ESR60] patch, v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

User impact if declined: 
Something insecure or unsafe could happen during form submission.

Fix Landed on Version:
63

Risk to taking this patch (and alternatives if risky): 
Low. The change is small and it has been in Nightly for a while.

String or UUID changes made by this patch:
None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8998102 - Flags: approval-mozilla-esr60?
Comment on attachment 8994878 [details] [diff] [review]
patch, v2

Fixes a sec-high. Approved for 62.0b16 and ESR 60.2.
Attachment #8994878 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8998102 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> it's also worth checking whether the
> other places where we call FlushPendingSubmission correspond to anything in
> the spec....

I will check this in bug 1477286.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.