Page MenuHomePhabricator
Differential D14232

Bug 1513445 - Disallow web documents loaded with the SystemPrincipal
AbandonedPublic
Actions

Authored by freddyb on Dec 12 2018, 5:39 AM.

Details

Reviewers
ckerschb
Bugzilla Bug ID
1513445
Summary

Bug 1513445 - Disallow web documents loaded with the SystemPrincipal

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Branch
bug-1513445-assert-for-sub-and-document
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 39122
Build 50226: arc lint + arc unit

Event Timeline

freddyb created this revision.Dec 12 2018, 5:39 AM
Herald added a project: secure-revision. · View Herald TranscriptDec 12 2018, 5:39 AM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 12 2018, 5:40 AM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
Harbormaster completed remote builds in B26593: Diff 43478.Dec 12 2018, 5:40 AM
freddyb requested review of this revision.Dec 12 2018, 5:40 AM
reviewbot added a subscriber: reviewbot.Dec 12 2018, 5:59 AM

Code analysis found 3 defects in this patch:

  • 3 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -p path/to/file.cpp (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects: https://queue.taskcluster.net/v1/task/JWjxec5kSGi2cKE0TC4jHg/runs/0/artifacts/public/results/clang-format-PHID-DIFF-xlxy5qrfjz7jk5bbcj4q.diff (use it in your repository with hg import or git apply)

If you see a problem in this automated review, please report it here: https://bit.ly/2IyNRy2

dom/security/nsContentSecurityManager.cpp
797–798 ↗(On Diff #43478)

Warning: Incorrect coding style [clang-format]

800–806 ↗(On Diff #43478)

Warning: Incorrect coding style [clang-format]

808 ↗(On Diff #43478)

Warning: Incorrect coding style [clang-format]

ckerschb requested changes to this revision.Dec 12 2018, 10:33 AM

I don't understand what this patch is supposed to be doing. The comment talks about web documents, but then you check if the type is of TYPE_SCRIPT (twice)?
What do you want to assert? Or put differently, what should never happen here?

This revision now requires changes to proceed.Dec 12 2018, 10:33 AM
freddyb updated this revision to Diff 43512.Dec 12 2018, 10:39 AM

Now checking correct content type

Harbormaster completed remote builds in B26615: Diff 43512.Dec 12 2018, 10:39 AM
reviewbot added a comment.Dec 12 2018, 10:53 AM

Code analysis found 3 defects in this patch:

  • 3 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -p path/to/file.cpp (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects: https://queue.taskcluster.net/v1/task/DJoE6WEQQRSAjRdp3xawjg/runs/0/artifacts/public/results/clang-format-PHID-DIFF-j6nmsigd6l4obqjrrvz7.diff (use it in your repository with hg import or git apply)

If you see a problem in this automated review, please report it here: https://bit.ly/2IyNRy2

dom/security/nsContentSecurityManager.cpp
797–798 ↗(On Diff #43512)

Warning: Incorrect coding style [clang-format]

800–806 ↗(On Diff #43512)

Warning: Incorrect coding style [clang-format]

808 ↗(On Diff #43512)

Warning: Incorrect coding style [clang-format]

freddyb updated this revision to Diff 43518.Dec 12 2018, 10:56 AM

Now, with clang-format nits fixed

Harbormaster completed remote builds in B26619: Diff 43518.Dec 12 2018, 10:56 AM
ckerschb requested changes to this revision.Dec 12 2018, 11:43 AM
ckerschb added inline comments.
dom/security/nsContentSecurityManager.cpp
808 ↗(On Diff #43512)

we should structure that in a better way:

  • I guess we only want that for debug builds, right? If so, please use #ifdef DEBUG
  • That whole if/and/or construct seems overly complicated, why not have several nested ifs using comments explaining what we look out for. e.g. you could start with: if scheme is of type http or http then we dig deeper, within that if you can do, if type is doc or subdoc, then dig deeper, ...
  • Why do we want that not in automation? I guess we should do a try run and then look at the tests that are failing and potentially annotate the failing tests instead of just doing !IsInAutomation.
  • nit: please use a more descriptive string than 'bad channel'

While I think that could work for loads of type sub document, I am not so sure about type document loads because those don't have a valid loadingPrincipal, do they?

This revision now requires changes to proceed.Dec 12 2018, 11:43 AM
freddyb updated this revision to Diff 43521.Dec 12 2018, 11:59 AM
freddyb marked 8 inline comments as done.

Turn logical-and conditionals into nested ifs for improved readability.
Improved wording on assert message.

Harbormaster completed remote builds in B26622: Diff 43521.Dec 12 2018, 11:59 AM
ckerschb requested changes to this revision.Dec 12 2018, 2:59 PM

As discussed on slack, we should

  • remove the isInAutomation bits
  • check what the loadingPrincipal is in the top-level doc cases
  • evaluate a TRY Push by things that are failing and see if we can/should fine grain the assertion, and then ultimately how we can annotate those tests
  • finally, I guess we should ifdef the entire assertion and only run in debug builds

Once all that happened I am happy to review again!

This revision now requires changes to proceed.Dec 12 2018, 2:59 PM
freddyb updated this revision to Diff 53947.Jan 22 2019, 2:49 PM

adding carve-out to not break browser-mochitests.
changed ordering of if's to do most performance-sensitive checks last.

Harbormaster completed remote builds in B34176: Diff 53947.Jan 22 2019, 2:49 PM
ckerschb requested changes to this revision.Jan 22 2019, 3:42 PM

Please look at my previous suggestions, none of them seem to have it made into this version.
Now we are back to whitelisting everything which runs on TRY, no?
Also, please run ./mach clang-format before requesting r?

What we should do is, carve out only the tests we know will fail and then also have the assertion on when running TRY. Covering everything into |#ifdef DEBUG| is also important for this initial version of the assertion.

This revision now requires changes to proceed.Jan 22 2019, 3:42 PM
freddyb updated this revision to Diff 60521.Feb 11 2019, 10:12 AM

This moves the logic a bit around for performance (i.e., making most costly checks last)
and adds test.

Harbormaster completed remote builds in B39122: Diff 60521.Feb 11 2019, 10:12 AM
freddyb abandoned this revision.Feb 11 2019, 11:32 AM

Due to tooling problems, this revision had to be abandoned.
It has been superseded by https://phabricator.services.mozilla.com/D19351

dom/security/nsContentSecurityManager.cpp
808 ↗(On Diff #43512)
  1. I think we want this in all builds. Is there a reason we would want to enable this in DEBUG only?
  2. I agree, the nesting is a bit unfortunate. Will change
  3. Tests in automation appear like they are hosted on example.com (see build/pgo/server-locations.txt for more domain names). But of course, we shouldn't break them.
  4. Right on. Will do.
freddyb added a comment.Feb 11 2019, 11:33 AM

Actually, https://phabricator.services.mozilla.com/D19350 is first - then https://phabricator.services.mozilla.com/D19351

sigh.

Revision Contents
Changeset List

Diff 60521

View Options

dom/security/test/general/chrome.ini

Loading...
View Options

dom/security/test/general/file_assert_systemprincipal_documents.html

Loading...
View Options

dom/security/test/general/test_assert_systemprincipal_documents.xul

Loading...
Privacy · Cookies · Legal