Add flag to protocol handler to distinguish that trust domain is the whole spec

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

As sum. This is needed to remove ugly code branching "imap", "mailbox", "news" to compare whole spec as a trust domain in SecurityComapreURIs() function.
Posted patch mozilla-central patch (obsolete) — Splinter Review
This patch adds the new flag and changes NS_SecurityCompareURIs to use it.
Posted patch comm-central patch (obsolete) — Splinter Review
This patch is applicable on the comm-central repository and adds the new flag to be reported by all three protocols described at the top. This also changes nntp URIs to be compared by the whole spec.
Attachment #338364 - Flags: review?(bzbarsky)
Attachment #338365 - Flags: review?(bzbarsky)
Is there a reason to not use the NS_NetUtil function for checking URI flags here?

The flag value is wrong; << 12 is already in use.  Want to reorder the flags to actually be in order so that's easier to catch?

>+     * This flag tells the security checkers to compare the whole URI
>+     * spec to check same origin between two URIs. Examples are
>+     * "imap", "mailbox" and "news".

How about:

  If this flag is set, then the origin for this protocol is the full URI spec,
  not just the scheme + host + port.

And call the flag ORIGIN_IS_FULL_SPEC, maybe?
Attachment #338364 - Flags: review?(bzbarsky) → review-
Comment on attachment 338365 [details] [diff] [review]
comm-central patch

Looks ok to me, with the new flag name, but dmose should look too.
Attachment #338365 - Flags: review?(dmose)
Attachment #338365 - Flags: review?(bzbarsky)
Attachment #338365 - Flags: review+
Boris: you gave r- for the mozilla-central patch but w/o any comment, mistake?
(In reply to comment #5)
> Boris: you gave r- for the mozilla-central patch but w/o any comment, mistake?

Ups, sorry, missed the email.
Posted patch mozilla-central patch, v2 (obsolete) — Splinter Review
I rearranged and changed name of the flag.
Attachment #338364 - Attachment is obsolete: true
Attachment #360351 - Flags: review?(bzbarsky)
Updated the flag name.
Attachment #338365 - Attachment is obsolete: true
Attachment #360352 - Flags: review?(dmose)
Attachment #338365 - Flags: review?(dmose)
The question about the nsNetUtil function (NS_URIChainHasFlags) remains, no?  Was there a reason to not use it?
(In reply to comment #9)
> The question about the nsNetUtil function (NS_URIChainHasFlags) remains, no? 
> Was there a reason to not use it?

Grr... missed that comment. I'll make a new patch.
Posted patch mozilla-central patch, v2.1 (obsolete) — Splinter Review
I did rather rebuild with that change. Now it looks really better then before.
Attachment #360351 - Attachment is obsolete: true
Attachment #360410 - Flags: review?(bzbarsky)
Attachment #360351 - Flags: review?(bzbarsky)
Attachment #360410 - Flags: review?(bzbarsky) → review+
Comment on attachment 360410 [details] [diff] [review]
mozilla-central patch, v2.1

Let's make that NS_FAILED(...) || hasFlag, so that the failure case fails towards more secure behavior, and looks good.
Posted patch mozilla-central patch, v2.2 (obsolete) — Splinter Review
Changed as suggested. Ready to land patch.
Attachment #360410 - Attachment is obsolete: true
Attachment #360411 - Flags: review+
Dan, will you get time to review this? It gets old, thanks.
Status: NEW → ASSIGNED
Yes, apologies for not getting to it sooner, and thanks for the patch.  There is some other stuff in front of it in my queue, but I will get there.
(In reply to comment #15)
> Yes, apologies for not getting to it sooner, and thanks for the patch.  There
> is some other stuff in front of it in my queue, but I will get there.

This is not urgent, only potential merging may get worse (but not impossible).
Comment on attachment 360352 [details] [diff] [review]
comm-central patch, v2

Honza, it looks like not too much of this has bitrotted (just a small bit of the mozilla-central patch).

I think I can take the review on. However, do you how would I tell that the code is still working? i.e. what could I expect to break if this wasn't working properly?
Attachment #360352 - Flags: review?(dmose) → review?(bugzilla)
Honestly I'm not a big expert to privileges.  This is changing a very very old code.  Probably bz would say better what could happen when this didn't work.

If we bypass the block for imap, mailbox or news scheme, I assume we end up with security checks for those URIs always being false.  What all this may cause in mailnews I have no idea.
If the mailnews schemes did not have ORIGIN_IS_FULL_SPEC, then one message could read the contents of another on the same server, if it could guess its URL.
Comment on attachment 360352 [details] [diff] [review]
comm-central patch, v2

Ok, I can't actually find a way to test this, but from analysis of the changes this looks fine.

When you come to land it, please can you ping me first as we may need to add some ifdefs depending on the state of comm-central.
Attachment #360352 - Flags: review?(bugzilla) → review+
(In reply to comment #20)
> When you come to land it, please can you ping me first as we may need to add
> some ifdefs depending on the state of comm-central.

If we land comm-central patch sooner, that tree will be broken, but we are safe we to not have an unsecure builds out there.  Not sure how you want to ifdef this.

However, please suggest any plan that would not involve temporary tree-redness.
So, to land this without a risk of having an insecure build (nightly) released among people and also redness of the tree, we should:

- update the comm-central patch with changes conditioned with #ifdef IS_ORIGIN_IS_FULL_SPEC_DEFINED and land it

- update the mozilla-central patch with #define IS_ORIGIN_IS_FULL_SPEC_DEFINED and then land it too

Sounds good?
Any consensus on landing this?  This grows old...
Comment 22 sounds fine to me, fwiw.
So, the first part, now inactive under defines has been landed on comm-central.

Please, give me a signal to land the mozilla-central patch, that unlocks the changes and starts using them.
This is the patch to land on m-c.
Attachment #551433 - Flags: review+
So does the second half of this bug need to land now?  Honza, is there some code change you're waiting for?
(In reply to Jason Duell (:jduell) from comment #28)
> So does the second half of this bug need to land now?  Honza, is there some
> code change you're waiting for?

I'm not sure how exactly the comm-central and mozilla-central merging works, but as I'm thinking of it, we probably may land the patch right now.  I wanted to avoid having the new mozilla-central code be in w/o the change in protocol handlers that have to expose the new flag.  Unless I missed something it should be OK to land the rest now too.
Mark--is this correct?

Thunderbird is built using the latest comm-central code and either the current m-c, or an older one

  http://ccgi.standard8.plus.com/blog/archives/501

So as long as you landed the comm-central patch first (which you did) you should be fine to land the 2nd part on m-c now (holding off marking [need-checkin] in case Mark points out something I'm missing here).
Yep, you're fine landing the m-c patch now.
Honza, I'm not going to mark this checkin-needed because I know you generally like to check stuff in yourself--go for it.
Comment on attachment 551433 [details] [diff] [review]
mozilla-central patch, v2.2 [landed, comment 33]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f3e28e0f909f
Attachment #551433 - Attachment description: mozilla-central patch, v2.2 [ready to land] → mozilla-central patch, v2.2 [landed, comment 33]
Whiteboard: [inbound]
Backed out of inbound due to M-oth shutdown crashes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/74e7ea3b71be

See:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=f3e28e0f909f
Whiteboard: [inbound]
Version: 1.9.0 Branch → Trunk
Boris, does this still make sense to be finished?  I completely forgot about this bug.
Flags: needinfo?(bzbarsky)
If we want extensions to be able to implement schemes with this security property, then this makes sense, yes.
Flags: needinfo?(bzbarsky)
Wayne, the patch for mozilla-central introducing the new ORIGIN_IS_FULL_SPEC protocol handler flag is about to land.  This affects few protocols implemented by mailnews.  Portions needed to make it work on c-c has already landed (years ago, comment 25).

Before we land the m-c part, I would like to check it doesn't affect comm-central test coverage.

How can I do that?  I presume I can do a push to try-comm-central with reference to e.g. the try repo/revision from comment 37.  But I don't know how to do it the best way.

Feel free to suggest someone else to answer this.  Thanks.
Flags: needinfo?(vseerror)
(In reply to Honza Bambas (:mayhemer) from comment #38)
> How can I do that?  I presume I can do a push to try-comm-central with
> reference to e.g. the try repo/revision from comment 37.  But I don't know
> how to do it the best way.

The following is the shell script I use to push changes to try-comm-central. The key part is the second if statement. Effectively, you need to add a patch that matches the mozilla-*.patch glob in the top level of comm-central, along with a tweak to some arguments to client.py. Everything else is just other stuff to handle mq phases and set up try syntax.

#!/bin/bash

changed=$(hg stat -m -a -r -d)
if [ -n "$changed" ]; then
  echo "ERROR: You have pending changes. Not doing anything."
  exit 1
fi
if [[ "$1" == "--patch" ]]; then
  echo "Adding mozilla patches"
  mozrepo=$(hg root)/mozilla
  ccrepo=$(hg root)
  i=0
  for patch in $(hg -R $mozrepo qapplied); do
    outpatch=$(printf "$ccrepo/mozilla-%04d-%s.patch" $i $patch)
    hg diff -R $mozrepo -c $patch > $outpatch
    hg add $outpatch
    i=$(($i + 1))
  done

  sed -i -e 's/--hgtool=[^ ]*/--apply-patches &/' $ccrepo/build/client.py-args
  #sed -i -e '/ALWAYS_RUN_CLIENT_PY/d' $ccrepo/mail/config/mozconfigs/win32/*
  shift
fi
if [ -z "$@" ]; then
  syntax="try: -b -do -p all -u all -t none"
else
  syntax="$@"
fi
hg qnew '~~~try~~~' -m "$syntax"
hg push -f ssh://hg.mozilla.org/try-comm-central
#hg phase -f -d qbase:qtip
hg qpop
hg qrm '~~~try~~~'
Comment on attachment 8708421 [details] [diff] [review]
mozilla-central patch, v2.2 [merged]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f4c16a34955b

For reference, slightly modified for win32/MinGW

#!/bin/bash

# run from comm-central (the clone root)

changed=$(hg stat -m -a -r -d)
if [ -n "$changed" ]; then
  echo "ERROR: You have pending changes. Not doing anything."
  exit 1
fi
if [[ "$1" == "--patch" ]]; then
  echo "Adding mozilla patches"
  mozrepo=$(pwd)/mozilla
  ccrepo=$(pwd)
  i=0
  for patch in $(hg -R $mozrepo qapplied); do
    outpatch=$(printf "$ccrepo/mozilla-%04d-%s.patch" $i $patch)
    hg diff -R $mozrepo -c $patch > $outpatch
    hg add $outpatch
    i=$(($i + 1))
  done

  # Doesn't work? -> Permissions on $ccrepo/build must be changed to Full Control for the current user
  sed -i -e 's/--hgtool=[^ ]*/--apply-patches &/' $ccrepo/build/client.py-args
  shift
fi
if [ -z "$@" ]; then
  syntax="try: -b -do -p all -u all -t none"
else
  syntax="$@"
fi
hg qnew '~~~try~~~' -m "$syntax"
hg push -f ssh://hg.mozilla.org/try-comm-central
hg qpop
hg qrm '~~~try~~~'
Flags: needinfo?(vseerror)
So, comm-central has certain tests checking the default protocol flags.  Obviously when we land the m-c patch that turns these flags on (with a #define), those test on c-c are going to fail.

I'll land the m-c parts, file an orange bug to fix the tests on c-c.
Depends on: 1241238
https://hg.mozilla.org/mozilla-central/rev/bbfbc6368c93
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1340677
You need to log in before you can comment on or make changes to this bug.