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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Honza Bambas (:mayhemer)]

As sum. This is needed to remove ugly code branching "imap", "mailbox", "news" to compare whole spec as a trust domain in SecurityComapreURIs() function.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch

This patch adds the new flag and changes NS_SecurityCompareURIs to use it.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: comm-central patch

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.

Comment 3

[Boris Zbarsky [: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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338365 [details] [diff] [review]
comm-central patch

Looks ok to me, with the new flag name, but dmose should look too.

Comment 5

[Honza Bambas (:mayhemer)]

Boris: you gave r- for the mozilla-central patch but w/o any comment, mistake?

Comment 6

[Honza Bambas (:mayhemer)]

(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.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch, v2

I rearranged and changed name of the flag.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: comm-central patch, v2

Updated the flag name.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The question about the nsNetUtil function (NS_URIChainHasFlags) remains, no?  Was there a reason to not use it?

Comment 10

[Honza Bambas (:mayhemer)]

(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.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch, v2.1

I did rather rebuild with that change. Now it looks really better then before.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch, v2.2

Changed as suggested. Ready to land patch.

Comment 14

[Honza Bambas (:mayhemer)]

Dan, will you get time to review this? It gets old, thanks.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 16

[Honza Bambas (:mayhemer)]

(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 17

[Mark Banner (:standard8)]

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?

Comment 18

[Honza Bambas (:mayhemer)]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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 20

[Mark Banner (:standard8)]

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.

Comment 21

[Honza Bambas (:mayhemer)]

(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.

Comment 22

[Honza Bambas (:mayhemer)]

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?

Comment 23

[Honza Bambas (:mayhemer)]

Any consensus on landing this?  This grows old...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment 22 sounds fine to me, fwiw.

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: comm-central patch, v2 [as landed, comment 25]

http://hg.mozilla.org/comm-central/rev/3ffc5d736389

Comment 26

[Honza Bambas (:mayhemer)]

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.

Comment 27

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch, v2.2 [landed, comment 33]

This is the patch to land on m-c.

Comment 28

[Jason Duell]

So does the second half of this bug need to land now?  Honza, is there some code change you're waiting for?

Comment 29

[Honza Bambas (:mayhemer)]

(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.

Comment 30

[Jason Duell]

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).

Comment 31

[Mark Banner (:standard8)]

Yep, you're fine landing the m-c patch now.

Comment 32

[Jason Duell]

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 33

[Honza Bambas (:mayhemer)]

Comment on attachment 551433 [details] [diff] [review]
mozilla-central patch, v2.2 [landed, comment 33]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f3e28e0f909f

Comment 34

[Ed Morley [:emorley]]

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

Comment 35

[Honza Bambas (:mayhemer)]

Boris, does this still make sense to be finished?  I completely forgot about this bug.

Comment 36

[Boris Zbarsky [:bzbarsky]]

If we want extensions to be able to implement schemes with this security property, then this makes sense, yes.

Comment 37

[Honza Bambas (:mayhemer)]

Attachment: mozilla-central patch, v2.2 [merged]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73db502c94a

Comment 38

[Honza Bambas (:mayhemer)]

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.

Comment 39

[Joshua Cranmer [:jcranmer]]

(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 40

[Honza Bambas (:mayhemer)]

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~~~'

Comment 41

[Honza Bambas (:mayhemer)]

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.

Comment 42

[Honza Bambas (:mayhemer)]

Comment on attachment 8708421 [details] [diff] [review]
mozilla-central patch, v2.2 [merged]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfbc6368c93

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbfbc6368c93