gUM persistent permissions must not work for http

VERIFIED FIXED in Firefox 30

Status

()

defect
P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: abr, Assigned: florian)

Tracking

({sec-high})

30 Branch
Firefox 31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox29 unaffected, firefox30+ verified, firefox31+ verified, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.4 unaffected)

Details

(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa+])

Attachments

(5 attachments, 3 obsolete attachments)

Bug 804611 introduced a mechanism for granting persistent permissions to the camera and microphone, but did not prevent it from being available for sites retrieved over insecure connections. The required behavior in this regard is well documented in the RTCWEB security specification:

>  Because HTTP origins cannot be securely established against network
>  attackers, implementations MUST NOT allow the setting of permanent
>  access permissions for HTTP origins.  Implementations MAY also opt to
>  refuse all permissions grants for HTTP origins, but it is RECOMMENDED
>  that currently they support one-time camera/microphone access.

This is a truly major security vulnerability, allowing remote sites to access users' cameras without verifying that the intended site has been reached; and it is already in the hands of our Aurora users.

We need to fast-track and uplift a fix for this immediately -- without waiting for the broader security review (Bug 983158) -- before we see a real-world exploit of this behavior.
Blocks: 983158
Florian: per bug 983158 (which now seems to be complete), we need to resolve the HTTPS restriction ASAP or yank this until it is.

dveditz's concerns in the review about the UI are relevant, but are either large meta-issues or specific concerns in general with existing UI outside of this bug.
Flags: needinfo?(florian)
(In reply to Randell Jesup [:jesup] from comment #1)
> Florian: per bug 983158 (which now seems to be complete),

I've asked for some clarifications there.

> we need to resolve
> the HTTPS restriction ASAP or yank this until it is.

I'm planning to have a look at this next week. I think we need to resolve it before 2014-04-28 (when 30 merges to beta) on both central and aurora.

Is there any reason why this bug is marked security-sensitive, for an issue that's been discussed in the public bug 983158 and originally mentioned on bug 804611 comment 47?
Flags: needinfo?(florian)
Flags: firefox-backlog?
This is the security document in question:

http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-09

See section 5.1 and 5.2 (partially quoted above).
Florian:  We really need to address this as soon as possible.

From the initial comment

>We need to fast-track and uplift a fix for this immediately -- without waiting for the broader security review (Bug 983158) -- before we see a real-world exploit of this behavior.

Per above, this needs to be fixed or disabled (or backed out) ASAP, not just before uplift.  If it can't be fixed ASAP, we should disable for now and you can fix it afterwards.  I was under the impression before this landed that it would never reach Aurora this way (and that it would be disabled-by-default on Nightly after landing, probably that wasn't clear in IRC, but it should have been in the bug).  I missed your summary in the bug of the IRC conversation (bugmail avalanche, and I wasn't reviewing the UI portion).  My apologies; I assumed the statements that it *must* be https-loaded to allow this behavior were clear.

The disable can be simply ghosting the entry I assume.  If you think it will be a while to fix and want to allow experimentation, we can consider a pref to enable it (off by default).

Thanks
(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> Is there any reason why this bug is marked security-sensitive, for an issue
> that's been discussed in the public bug 983158 and originally mentioned on
> bug 804611 comment 47?

It's SS because we're discussing an exploitable bug that's in the hands of all Aurora users. At this point, it would be tempting for an attacker to attempt to insert themselves in the place of any web site that requests camera permissions, under the assumption that some non-zero number of Aurora users have allowed that site to ask for the camera without prompting. The lure of remote controlling another users' camera is not small, and the privacy implications are pretty profound.

If you want to argue for making 938158 and 804611 SS, I'm on board with that. Randell?
Flags: needinfo?(rjesup)
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> I'm planning to have a look at this next week.

I don't think that's okay, given that this is a real security issue in the hands of Aurora users right now. I'm also perplexed about why we can't do this right now, given that the solution is very small. In short:

Adjust this code so that the "secondaryActions" don't include the "Always" option by default; and insert it into the array after a conditional check that the URI scheme is HTTPS:
http://dxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm#141

Add a new conditional to this "else" clause that checks whether the URI  has a scheme of https, and which skips the clause if not:
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?from=MediaManager.cpp#1415

This looks like maybe 10 lines of code, on the outside, along with a test that verifies the new behaviors (i.e., persistent permissions aren't offered and aren't honored on http pages). Given that this is a real and emergent security hole with a relatively small fix, I don't think "next week" is the timeframe we're looking for here.
(Addendum: you'll also need to either remove "never" from the menu as well, or evaluate "never" for non-https URLs in MediaManager.cpp)
Flags: needinfo?(rjesup)
Posted patch Run tests over https (obsolete) — Splinter Review
(In reply to Randell Jesup [:jesup] from comment #4)
> Florian:  We really need to address this as soon as possible.

Is there any evidence that this is more important today than it was last week? This has been publicly visible and waiting for input from Security for almost a month, I don't understand the sudden rush here.

This feature is barely discoverable, so only a tiny portion of the aurora user base (how large is it?) is likely affected currently.

If you think there's a significant number of people currently at actual risk and want to protect them ASAP, you can land today the media manager change to just ignore permissions for http urls (do you have the answer to my question in bug 983158 comment 4?) and I'll figure out the UI part and the test later.

Currently the tests run over http. You'll need the patch I'm attaching to keep the tests working if you add the restriction to media manager.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Created attachment 8404913 [details] [diff] [review]
> Run tests over https
> 
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Florian:  We really need to address this as soon as possible.
> 
> Is there any evidence that this is more important today than it was last
> week? This has been publicly visible and waiting for input from Security for
> almost a month, I don't understand the sudden rush here.

It's not more important than last week. We shouldn't have landed it
then either. It's that Adam, I (and perhaps Jesup) just found
out that it did anyway.


> If you think there's a significant number of people currently at actual risk
> and want to protect them ASAP, you can land today the media manager change
> to just ignore permissions for http urls (do you have the answer to my
> question in bug 983158 comment 4?) and I'll figure out the UI part and the
> test later.

This seems like a good idea.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Created attachment 8404913 [details] [diff] [review]
> Run tests over https
> 
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Florian:  We really need to address this as soon as possible.
> 
> Is there any evidence that this is more important today than it was last
> week? This has been publicly visible and waiting for input from Security for
> almost a month, I don't understand the sudden rush here.

As EKR points out, the rush is because we just found out about this. This should never have landed in m-c in the first place, and certainly should not have ridden the train out to Aurora. The rush is because we're catching this weeks after it should have been caught.

> This feature is barely discoverable, so only a tiny portion of the aurora
> user base (how large is it?) is likely affected currently.

It's not a matter how how many people are exposed: one exploit is too many. People do truly freak out about this kind of thing; see http://arstechnica.com/tech-policy/2010/02/school-under-fire-for-spying-on-kid-via-webcam-at-home/

In terms of what part of the page has to load over HTTP: by my thinking, all we need to worry about is the actual document that's calling getUserMedia, rather than any document that may wrap it in an iframe (but I'd like Daniel to weigh in on the topic). In any case, even if you end up wanting additional checks, doing that is better than doing nothing, so I'd recommend getting that protection in place as quickly as possible, and then enhancing it (if necessary) afterward.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> ...you can land today the media manager change...

Unfortunately, I have an awful lot going on right now, so all I have time to do is back out the entire feature. If you don't have the time to address this in the next couple of days, I'd be happy to take care of backing it out for you.

Do you think you can you get these changes in the tree and ready for Aurora uplift by the end of Monday?
In case we need to back the feature out, this patch should do the job (still needs to be run through try to ensure I didn't botch the unrot)
I'll look to see if I can disable it easily.  I have a bunch of other landings to do
Comment on attachment 8404952 [details] [diff] [review]
gUM persistent permission grant backout

A full backout wouldn't be a good idea at this point, we shouldn't touch localizable strings on aurora. If we run out of time and need to remove the feature on aurora, we should backout the changes to dom/media/MediaManager and just hide the UI/disable the test.
Attachment #8404952 - Flags: review-
(In reply to Adam Roach [:abr] from comment #11)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)
> > ...you can land today the media manager change...

> Do you think you can you get these changes in the tree and ready for Aurora
> uplift by the end of Monday?

The simple media manager change, yes (unless jesup does it before me of course!). The rest will take some more time, but should be fixed on both central and aurora by the end of next week.
Change is just putting an if() {...} around a block of permissions code in MediaManager.cpp, plus disabling entries in UI
Attachment #8406306 - Flags: review?(florian)
Flags: firefox-backlog? → firefox-backlog+
Attachment #8404913 - Attachment is obsolete: true
Attachment #8406388 - Flags: review?(rjesup)
Comment on attachment 8406306 [details] [diff] [review]
disable persistent permissions until https checking is done

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

This would break the test at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_devices_get_user_media.js in a few different ways. If you actually think we will need to land this disabling, you'll need to comment out a few parts of this test file.

::: browser/modules/webrtcUI.jsm
@@ +139,5 @@
>    };
>  
>    let secondaryActions = [
> +/*
> + * To be re-enabled soon, bug 993495

This will break the test "getUserMedia prompt: Always/Never Share", and will also break other tests because kActionDeny would now be 1.

::: dom/media/MediaManager.cpp
@@ +1462,2 @@
>      nsresult rv;
> +    if (false /*Preferences::GetBool("media.navigator.permission.insecure_persistent_allowed")*/) {

This would break the "getUserMedia without prompt: use persistent permissions" test.
Attachment #8406306 - Flags: review?(florian) → review-
This adds to Florian's patch to make it not save perms on an http site (his patch made it not load them).  Together, this should cover it until he can land the real solution later this week.
Randell, do you know how this behaves with mixed content?
Comment on attachment 8406388 [details] [diff] [review]
MediaManager part

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

There's a little duplication of code that isn't needed, but that's fixed in my followup patch.

This patch alone would have a lesser issue: saving perms on an http site would appear to not work, but would cause the same site loaded from https to use the perms you saved.  My followup patch plugs this for now, until Florian can land the full solution (and tests for http) later this week.

r+ing with addition of my patch.
Attachment #8406388 - Flags: review?(rjesup) → review+
Attachment #8406555 - Flags: review?(florian)
(In reply to Eric Rescorla (:ekr) from comment #21)
> Randell, do you know how this behaves with mixed content?

There's no explicit test for mixed content here.

The normal mixed-content blocking will be active.
Comment on attachment 8406555 [details] [diff] [review]
disable saving preference on http-loaded sites

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

::: browser/modules/webrtcUI.jsm
@@ +159,5 @@
>        label: stringBundle.getString("getUserMedia.never.label"),
>        accessKey: stringBundle.getString("getUserMedia.never.accesskey"),
>        callback: function () {
>          denyRequest(aCallID);
> +        if (aSecure) {

Is this intentional? I don't see a reason to prevent the user from saving that he doesn't want to be bothered again by the permission prompt on an http website.
Comment on attachment 8406555 [details] [diff] [review]
disable saving preference on http-loaded sites

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

Looks good with comment 24 addressed.
Attachment #8406555 - Flags: review?(florian) → review+
Status: NEW → ASSIGNED
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa?]
Comment on attachment 8406555 [details] [diff] [review]
disable saving preference on http-loaded sites

>   } else {
>+    nsCOMPtr<nsIURI> docURI = aWindow->GetDocumentURI();
>+    NS_ENSURE_TRUE(docURI, NS_ERROR_FAILURE);
>+    bool isHTTPS;
>+    nsresult rv = docURI->SchemeIs("https", &isHTTPS);
>+    NS_ENSURE_SUCCESS(rv, rv);
I think we should use uri from principal here, 
but looks like we'd need to fix other stuff too.

We're not consistent.
We do for example
permManager->TestExactPermissionFromPrincipal(
  aWindow->GetExtantDoc()->NodePrincipal(), "microphone", &audioPerm);
but then we check https from GetDocumentURI

So if we at some point give the permission, then
about:blank created from the right https: site can get user media without
explicit permission, but if there first isn't that permission,
about:blank can't be used to create the persistent permission.

http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm?rev=b5bf95a06118&mark=123-123,163-163,165-165#120
uses documentURI when storing the permission.

And http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/permissions.js?rev=244ff7612e87&mark=32-32#30
when showing the permission UI.
This all is nicely inconsistent.

So, explicit https: check is fine for now, given that we have inconsistencies in so many places
>   } else {
>+    nsCOMPtr<nsIURI> docURI = aWindow->GetDocumentURI();
>+    NS_ENSURE_TRUE(docURI, NS_ERROR_FAILURE);
>+    bool isHTTPS;
>+    nsresult rv = docURI->SchemeIs("https", &isHTTPS);
>+    NS_ENSURE_SUCCESS(rv, rv);
I would make this throwing.
So, 
bool isHttps = false;
nsIURI* docURI = aWindow->GetDocumentURI();
if (docURI) {
  docURI->SchemeIs("https", &isHTTPS);
}
Attachment #8406555 - Flags: review?(bugs) → review+
We'll need to handle smaug's other comments about document vs principal in the followup.  Per florian's review and discussion, removed the disable of Never on http sites - let users stop an http site from bombarding them with popups.
Attachment #8406555 - Attachment is obsolete: true
Randell:

Looking at this patch, it looks like we still display the "always/never" UI for insecure pages, but simply treat it as "yes but only this time" or "no but only this time". That seems like a really surprising user experience. Am I reading the code wrong?
Comment on attachment 8407073 [details] [diff] [review]
disable saving preference on http-loaded sites

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Fairly easily, though actually making use of this requires getting a user to save permissions and/or execute a MITM or injection attack.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  "isSecure" and https checks make it pretty clear what's going on.

Which older supported branches are affected by this flaw?  30

If not all supported branches, which bug introduced the flaw? 30

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backport should be easy; I'll merge the patch to 30 in preparation.

How likely is this patch to cause regressions; how much testing does it need?  Low likelihood of regressions.  Basic testing of with-and-without, and save and then try on http, etc.



[Approval Request Comment]
Bug caused by (feature/regressing bug #): 804611

User impact if declined: Major privacy risk, especially for MITM attacks

Testing completed (on m-c, etc.): ready to land (see sec-approval)
Note: we'll need to land a follow-up patch to clean up the UI and tests

Risk to taking this patch (and alternatives if risky): low

String or IDL/UUID changes made by this patch: none
Attachment #8407073 - Flags: sec-approval?
Attachment #8407073 - Flags: review+
Attachment #8407073 - Flags: approval-mozilla-aurora?
Tagging Randell for an answer to my question in Comment 28.
Flags: needinfo?(rjesup)
(In reply to Adam Roach [:abr] from comment #28)
> Randell:
> 
> Looking at this patch, it looks like we still display the "always/never" UI
> for insecure pages, but simply treat it as "yes but only this time" or "no
> but only this time". That seems like a really surprising user experience. Am
> I reading the code wrong?

No, that's correct.  Florian is going to work on a followup patch to disable Always and add tests tomorrow. Per requests in this bug, trying to get something landed to plug the hole ASAP
Comment on attachment 8407073 [details] [diff] [review]
disable saving preference on http-loaded sites

sec-approval+ for trunk and I've given Aurora approval as well.
Attachment #8407073 - Flags: sec-approval?
Attachment #8407073 - Flags: sec-approval+
Attachment #8407073 - Flags: approval-mozilla-aurora?
Attachment #8407073 - Flags: approval-mozilla-aurora+
Matt, can you please investigate what testing this needs before the end of the current iteration? The iteration ends two weeks from yesterday.
QA Contact: mwobensmith
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa?] → p=3 s=it-31c-30a-29b.3 [qa+]
Target Milestone: Firefox 30 → Firefox 31
Posted patch UI changes (obsolete) — Splinter Review
I still need to add tests for this new behavior.
Attachment #8407673 - Attachment is obsolete: true
Attachment #8408318 - Flags: review?(felipc)
Comment on attachment 8408318 [details] [diff] [review]
UI changes and test

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

Patch looks good. It's pretty easy to understand the problem by looking at the patch/test. Is this a concern? I guess not since it didn't make it beyond Aurora.
Attachment #8408318 - Flags: review?(felipc) → review+
Comment on attachment 8408318 [details] [diff] [review]
UI changes and test

Note: I pushed this patch to try (https://tbpl.mozilla.org/?tree=Try&rev=9bbc71bca399) and want to see a green test run on all OSes before landing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 804611
User impact if declined: Broken UI, now that we have landed a change in the back-end to plug the hole.
Testing completed (on m-c, etc.): this patch includes a test for both this UI change and the MediaManager change that landed previously.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: none.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Easily, but not more easily than from the patch previously landed here.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, the test makes it obvious, but the patch already landed here also made it obvious.

Which older supported branches are affected by this flaw? 30

If not all supported branches, which bug introduced the flaw? bug 804611

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be easy.

How likely is this patch to cause regressions; how much testing does it need? Basic Q/A verification of the fix.


To help QA, here's an URL using gUM on https (https://apprtc.webrtc.org/) and one on http (http://mozilla.github.io/webrtc-landing/gum_test.html).
Attachment #8408318 - Flags: sec-approval?
Attachment #8408318 - Flags: approval-mozilla-aurora?
Comment on attachment 8408318 [details] [diff] [review]
UI changes and test

sec-approval+ for trunk and approval for Aurora. Please land it on trunk and see it go green before landing it on Aurora.
Attachment #8408318 - Flags: sec-approval?
Attachment #8408318 - Flags: sec-approval+
Attachment #8408318 - Flags: approval-mozilla-aurora?
Attachment #8408318 - Flags: approval-mozilla-aurora+
QA Contact: mwobensmith → drno
I tested this on 30.0a2 (2014-04-17):

To make things more interesting I used http://mozilla.github.io/webrtc-landing/gum_test.html and the same page but with HTTPS: https://mozilla.github.io/webrtc-landing/gum_test.html

I tested 'Always share', 'Never share' and 'Don't share'. It seems to work like expected for the most part, except the few points below.
- 'Always share' is persistent for HTTPS for sessions and restarts
- 'Always share' from HTTPS does not propagate to HTTP (the HTTP URL triggers a new door hanger dialog)
- 'Always share' is not persistent for HTTP (neither session or restart)

But I have a few questions/concerns (none of these prevent this from shipping and can be addressed in separate tickets if needed):
1) Why do we show the most dangerous option 'Always share' at the top of the drop-down menu?
   I understand that the options are right now in the order 'Share' (the default button), 'Always share', 'Don't share', Never share'. But I think if the user opens the drop-down menu it is most likely that he accidentally will hit the 'Always share' option. I would prefer the order 'Share', 'Don't share', 'Never share', 'Always share'.
2) Is is intentional that 'Never share' can only be removed via about:permissions?
   If I ever clicked 'Never share' (for HTTP or HTTPS) it gets persistently remembered. But if I visit that page again and changed my mind I failed to find an option to change it right there in that tab. Probably an edge case and don't need to be solved for this ticket, but we should think about an easier way to remove the persistent 'Never share' option.
3) The 'Never share' option from the HTTP URL over writes the 'Always share' option from the same HTTPS URL.
   Again an edge case which does not need to be solved right here. But if I visited the HTTPS URL and chose 'Always share' this gets properly stored. If I then visit the same URL via a HTTP URL and chose 'Never share' that option over writes my previous choice for the HTTPS URL and from now on I always get error messages if I visit the HTTPS URL again. Is this intentional?
4) As 'Always share' does not get stored for HTTP URL's, why do we offer that option at all?
   The always share does not even work within the same session. So 'Always share' has exactly the same functionality as the default 'Share' button for HTTP URL's. Would it be possible to not show 'Always share' in case of HTTP URL's?
https://hg.mozilla.org/mozilla-central/rev/87dd13ef285c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Nils Ohlmeier [:drno] from comment #42)
> I tested this on 30.0a2 (2014-04-17):
> 
> To make things more interesting I used
> http://mozilla.github.io/webrtc-landing/gum_test.html and the same page but
> with HTTPS: https://mozilla.github.io/webrtc-landing/gum_test.html
> 
> I tested 'Always share', 'Never share' and 'Don't share'. It seems to work
> like expected for the most part, except the few points below.
> - 'Always share' is persistent for HTTPS for sessions and restarts
> - 'Always share' from HTTPS does not propagate to HTTP (the HTTP URL
> triggers a new door hanger dialog)
> - 'Always share' is not persistent for HTTP (neither session or restart)

Good.
 
> But I have a few questions/concerns (none of these prevent this from
> shipping and can be addressed in separate tickets if needed):
> 1) Why do we show the most dangerous option 'Always share' at the top of the
> drop-down menu?
>    I understand that the options are right now in the order 'Share' (the
> default button), 'Always share', 'Don't share', Never share'. But I think if
> the user opens the drop-down menu it is most likely that he accidentally
> will hit the 'Always share' option. I would prefer the order 'Share', 'Don't
> share', 'Never share', 'Always share'.

A reasonable suggestion.

> 2) Is is intentional that 'Never share' can only be removed via
> about:permissions?
>    If I ever clicked 'Never share' (for HTTP or HTTPS) it gets persistently
> remembered. But if I visit that page again and changed my mind I failed to
> find an option to change it right there in that tab. Probably an edge case
> and don't need to be solved for this ticket, but we should think about an
> easier way to remove the persistent 'Never share' option.

I agree - that bit me too.  about:permissions is totally not discoverable.

When we auto-deny permission via Never, I suggest the doorhanger-exists icon (or one similar) in the URLbar could appear, and clicking it would open a doorhanger with "Access to your microphone and camera by site foo has been blocked" and a button "Open about:permissions" that pops open about:permissions, scrolled to the site.

> 3) The 'Never share' option from the HTTP URL over writes the 'Always share'
> option from the same HTTPS URL.
>    Again an edge case which does not need to be solved right here. But if I
> visited the HTTPS URL and chose 'Always share' this gets properly stored. If
> I then visit the same URL via a HTTP URL and chose 'Never share' that option
> over writes my previous choice for the HTTPS URL and from now on I always
> get error messages if I visit the HTTPS URL again. Is this intentional?

I think it's intentional, and correct.  IMHO.

> 4) As 'Always share' does not get stored for HTTP URL's, why do we offer
> that option at all?
>    The always share does not even work within the same session. So 'Always
> share' has exactly the same functionality as the default 'Share' button for
> HTTP URL's. Would it be possible to not show 'Always share' in case of HTTP
> URL's?

We shouldn't offer it; my understanding was that the "final" patch would either remove it from http sites, or disable the entry.  (This works even better with your #1 above).  This is something we should look to do a followup on soon and uplift.
(In reply to Nils Ohlmeier [:drno] from comment #42)
> I tested this on 30.0a2 (2014-04-17):

> But I have a few questions/concerns (none of these prevent this from
> shipping and can be addressed in separate tickets if needed):
> 1) Why do we show the most dangerous option 'Always share' at the top of the
> drop-down menu?
>    I understand that the options are right now in the order 'Share' (the
> default button), 'Always share', 'Don't share', Never share'. But I think if
> the user opens the drop-down menu it is most likely that he accidentally
> will hit the 'Always share' option. I would prefer the order 'Share', 'Don't
> share', 'Never share', 'Always share'.

I think if you want to make changes there, it's something that should be discussed in a separate bug with UX people, not in a hidden security bug. Personally I'm more concerned that people who need the "Always share" option may never discover the drop down and switch to another browser (that more aggressively adds persistent permissions) out of tiredness of clicking "Share" on the prompt.

> 2) Is is intentional that 'Never share' can only be removed via
> about:permissions?

My understanding of about:permissions is that it's not finished and not meant to be discoverable yet.

There are 2 other ways to remove the 'Never share' persistent permission that are both reachable with a few clicks:
- the identity panel when clicking the padlock or the globe icon in the URL bar
- the Permissions tab of the Page Info dialog (Either "View Page Info" in the context menu, "View Page Info" from the "Tools" menu, or the "More Information…" button from the identity panel).

> 3) The 'Never share' option from the HTTP URL over writes the 'Always share'
> option from the same HTTPS URL.
>    Again an edge case which does not need to be solved right here. But if I
> visited the HTTPS URL and chose 'Always share' this gets properly stored. If
> I then visit the same URL via a HTTP URL and chose 'Never share' that option
> over writes my previous choice for the HTTPS URL and from now on I always
> get error messages if I visit the HTTPS URL again. Is this intentional?

Mostly intentional. I can't think of a real-life case where a user would want to always allow gUM on the https version of a site and automatically deny it on the http version. I think security-conscious users would want to never load the http version; not to get rid of the gUM prompt when they are using http.

> 4) As 'Always share' does not get stored for HTTP URL's, why do we offer
> that option at all?

We don't. The nightly you tested doesn't contain the UI patch that landed yesterday.
(In reply to Florian Quèze [:florian] [:flo] from comment #45)
> (In reply to Nils Ohlmeier [:drno] from comment #42)
> > I tested this on 30.0a2 (2014-04-17):
> 
> I think if you want to make changes there, it's something that should be
> discussed in a separate bug with UX people, not in a hidden security bug.
> Personally I'm more concerned that people who need the "Always share" option
> may never discover the drop down and switch to another browser (that more
> aggressively adds persistent permissions) out of tiredness of clicking
> "Share" on the prompt.

I just tried to write down my findings/concerns from testing this in one central place, rather then spreading them in 4 new tickets (which each would have to be judge if security relevant or not).

With that being said: I totally agree with your statement there as well. Lets open another ticket to discuss this further.

> Mostly intentional. I can't think of a real-life case where a user would
> want to always allow gUM on the https version of a site and automatically
> deny it on the http version. I think security-conscious users would want to
> never load the http version; not to get rid of the gUM prompt when they are
> using http.

First of all it is easier said then done to prevent you from loading the http version. But I would actually see as a feature that I could set the HTTPS version of lets say Facebook to have gUM access all the time, but set the HTTP flavor to block so that I get an error message if someone happens to get me to click on an HTTP link, or Facebook decides to move me from my initial HTTPS to HTTP because of a bug, or...
Currently you can set it "only" to have access to gUM via HTTPS and have the door hanger pop up when you happen to hit the HTTP URL.
Maybe the Permissions tab of the Page Info page would be a nice place to more granular give control over this for users who are interested in this.
But it is an edge case.
 
> > 4) As 'Always share' does not get stored for HTTP URL's, why do we offer
> > that option at all?
> 
> We don't. The nightly you tested doesn't contain the UI patch that landed
> yesterday.

Well as I wrote at the top I tested Aurora. I just confirmed that 'Always share' is gone from Nightly. Thanks for that :-)
But in todays Aurora (30.0a2 (2014-04-18)) it is still present.
Hi Nils, the desktop iteration ends on Monday April 28 and wanted to know if this bug be marked as verified or if further testing is required?
Flags: needinfo?(drno)
Hi Marco,
I set the two versions which I manually verified to verified. So from my perspective this ticket is verified and good to ship (my open concerns can be better handled in a new ticket). Did I forget to set another flag or whiteboard marker?
Flags: needinfo?(drno)
Hi Nils, thanks very much for the update - much appreciated.
Status: RESOLVED → VERIFIED
Group: core-security
Component: Security → Device Permissions
You need to log in before you can comment on or make changes to this bug.