Closed
Bug 451081
Opened 16 years ago
Closed 8 years ago
Add flag to protocol handler to distinguish that trust domain is the whole spec
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(3 files, 6 obsolete files)
2.65 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
2.86 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
3.25 KB,
patch
|
mayhemer
:
review+
mayhemer
:
checkin+
|
Details | Diff | Splinter Review |
As sum. This is needed to remove ugly code branching "imap", "mailbox", "news" to compare whole spec as a trust domain in SecurityComapreURIs() function.
![]() |
Assignee | |
Comment 1•15 years ago
|
||
This patch adds the new flag and changes NS_SecurityCompareURIs to use it.
![]() |
Assignee | |
Comment 2•15 years ago
|
||
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.
![]() |
Assignee | |
Updated•15 years ago
|
Attachment #338364 -
Flags: review?(bzbarsky)
![]() |
Assignee | |
Updated•15 years ago
|
Attachment #338365 -
Flags: review?(bzbarsky)
![]() |
||
Comment 3•15 years ago
|
||
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?
![]() |
||
Updated•15 years ago
|
Attachment #338364 -
Flags: review?(bzbarsky) → review-
![]() |
||
Comment 4•15 years ago
|
||
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+
![]() |
Assignee | |
Comment 5•15 years ago
|
||
Boris: you gave r- for the mozilla-central patch but w/o any comment, mistake?
![]() |
Assignee | |
Comment 6•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 7•15 years ago
|
||
I rearranged and changed name of the flag.
Attachment #338364 -
Attachment is obsolete: true
Attachment #360351 -
Flags: review?(bzbarsky)
![]() |
Assignee | |
Comment 8•15 years ago
|
||
Updated the flag name.
Attachment #338365 -
Attachment is obsolete: true
Attachment #360352 -
Flags: review?(dmose)
Attachment #338365 -
Flags: review?(dmose)
![]() |
||
Comment 9•15 years ago
|
||
The question about the nsNetUtil function (NS_URIChainHasFlags) remains, no? Was there a reason to not use it?
![]() |
Assignee | |
Comment 10•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 11•15 years ago
|
||
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)
![]() |
||
Updated•15 years ago
|
Attachment #360410 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 12•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 13•15 years ago
|
||
Changed as suggested. Ready to land patch.
Attachment #360410 -
Attachment is obsolete: true
Attachment #360411 -
Flags: review+
![]() |
Assignee | |
Comment 14•15 years ago
|
||
Dan, will you get time to review this? It gets old, thanks.
Status: NEW → ASSIGNED
Comment 15•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 16•15 years ago
|
||
(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•13 years ago
|
||
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)
![]() |
Assignee | |
Comment 18•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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+
![]() |
Assignee | |
Comment 21•13 years ago
|
||
(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.
![]() |
Assignee | |
Comment 22•13 years ago
|
||
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?
![]() |
Assignee | |
Comment 23•13 years ago
|
||
Any consensus on landing this? This grows old...
![]() |
||
Comment 24•13 years ago
|
||
Comment 22 sounds fine to me, fwiw.
![]() |
Assignee | |
Comment 25•13 years ago
|
||
http://hg.mozilla.org/comm-central/rev/3ffc5d736389
Attachment #551429 -
Flags: review+
![]() |
Assignee | |
Comment 26•13 years ago
|
||
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.
![]() |
Assignee | |
Comment 27•13 years ago
|
||
This is the patch to land on m-c.
Attachment #551433 -
Flags: review+
Comment 28•13 years ago
|
||
So does the second half of this bug need to land now? Honza, is there some code change you're waiting for?
![]() |
Assignee | |
Comment 29•13 years ago
|
||
(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•13 years ago
|
||
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•12 years ago
|
||
Yep, you're fine landing the m-c patch now.
Comment 32•12 years ago
|
||
Honza, I'm not going to mark this checkin-needed because I know you generally like to check stuff in yourself--go for it.
![]() |
Assignee | |
Comment 33•12 years ago
|
||
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]
![]() |
Assignee | |
Updated•12 years ago
|
Whiteboard: [inbound]
Comment 34•12 years ago
|
||
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
![]() |
Assignee | |
Comment 35•9 years ago
|
||
Boris, does this still make sense to be finished? I completely forgot about this bug.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 36•9 years ago
|
||
If we want extensions to be able to implement schemes with this security property, then this makes sense, yes.
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 37•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73db502c94a
Attachment #360411 -
Attachment is obsolete: true
Attachment #551433 -
Attachment is obsolete: true
Attachment #8708421 -
Flags: review+
![]() |
Assignee | |
Comment 38•8 years ago
|
||
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)
Comment 39•8 years ago
|
||
(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~~~'
![]() |
Assignee | |
Comment 40•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 41•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 42•8 years ago
|
||
Comment on attachment 8708421 [details] [diff] [review] mozilla-central patch, v2.2 [merged] https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfbc6368c93
Attachment #8708421 -
Flags: checkin+
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bbfbc6368c93
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•