Closed Bug 1150703 Opened 5 years ago Closed 5 years ago

Add a flag to prevent content-privileged about URIs from being linked to

Categories

(Core :: Networking, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main38-])

Attachments

(1 file)

See bug 1147597 for context.

Basically, right now about: pages can pick between being privileged or being linked to. Sometimes they want neither. We should add a flag that can be used in conjunction with SAFE_FOR_UNTRUSTED_CONTENT to achieve this.
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite?
Duplicate of this bug: 1150862
Makes sense to me.
Attached patch Patch v0.1 (obsolete) — Splinter Review
I don't know if/how it's possible to write a test for this, but as this is sec-sensitive it should probably be another patch anyway.

Also, accepting suggestions for a better name for this flag if people think that's necessary.
Thanks for taking this Gijs. Best to move the browser/components changes to bug 1150862, I think.
Group: core-security
Attached patch Patch v0.2Splinter Review
Attachment #8588037 - Flags: review?(sworkman)
Comment on attachment 8588037 [details] [diff] [review]
Patch v0.2

A little bird tells me sworkman isn't around at the moment... Patrick/Jonas, can you help?
Attachment #8588037 - Flags: review?(mcmanus)
Attachment #8588037 - Flags: review?(jonas)
Going to assume this is sec-want as well per bug 1150862.
Keywords: sec-want
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment on attachment 8588037 [details] [diff] [review]
Patch v0.2

Review of attachment 8588037 [details] [diff] [review]:
-----------------------------------------------------------------

I apologize for the delay - I was hoping someone more familiar with your project would step up.. I did my best and this makes sense to me.

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp
@@ +33,5 @@
> +  uint32_t flags;
> +  nsresult rv = aModule->GetURIFlags(aURI, &flags);
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  return (flags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) != 0 && (flags & nsIAboutModule::MAKE_UNLINKABLE) == 0;

nit.. iirc the styleguide discourages comparing against null or 0.. so just
return (one) && !(two)
Attachment #8588037 - Flags: review?(mcmanus) → review+
Attachment #8588037 - Flags: review?(sworkman)
Attachment #8588037 - Flags: review?(jonas)
w/ nit:

remote:   https://hg.mozilla.org/integration/fx-team/rev/e416125133da
Flags: in-testsuite? → in-testsuite-
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e416125133da
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
Comment on attachment 8588037 [details] [diff] [review]
Patch v0.2

Approval Request Comment:
[Feature/regressing bug #]: Reader mode is safer with this.
[User impact if declined]: content pages can link to reader mode which is a Bad Idea (defense in depth)
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: pretty low
[String/UUID change made/needed]: no; there's an IDL adjustment but it's only an added flag which is defined on the IDL and therefore doesn't need UUID adjustment nor does it change binary compat, AIUI.
Attachment #8588037 - Flags: approval-mozilla-beta?
Attachment #8588037 - Flags: approval-mozilla-aurora?
Should be in 38 beta 5.
Attachment #8588037 - Flags: approval-mozilla-beta?
Attachment #8588037 - Flags: approval-mozilla-beta+
Attachment #8588037 - Flags: approval-mozilla-aurora?
Attachment #8588037 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main38-]
You need to log in before you can comment on or make changes to this bug.