Closed Bug 1049583 Opened 5 years ago Closed 5 years ago

Screensharing allowed_domains needs limited wildcard matching.

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: ehugg, Assigned: pkerr)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 2 obsolete files)

Based on comments in other bugs and some email exchanges this is what I think we need for wildcard matching of domain names in media.getusermedia.screensharing.allowed_domains

We will only allow the wildcard at the beginning of the string and only when followed by a '.'.  Also there must be at least one other '.' in the string to avoid allowing an entire TLD.  The wildcard can then match any number of subdomains on the front.

In other words these uses would not be allowed:
 - "*"
 - "*.foo"
 - "*foo.bar"
 - "foo.*.bar"

And these would work:
 - "*.foo.bar"
 - "*.foo.bar.baz"
Blocks: 1049087
Depends on: 1037424
Paul - can you take this one?

Also (though it's likely ill-formed anyways) *..foo should also fail, and *.foo. (which due to vagaries of DNS *might* resolve to *.foo)
Flags: needinfo?(pkerr)
Instead of just refusing *.TLD, it looks like we should be checking this file:
netwerk/dns/effective_tld_names.dat

It was created in bug 342314 for preventing abuse of cookies.
(In reply to Randell Jesup [:jesup] from comment #1)
> Paul - can you take this one?
> 
> Also (though it's likely ill-formed anyways) *..foo should also fail, and
> *.foo. (which due to vagaries of DNS *might* resolve to *.foo)

Yes I can. I already have a partial implementation of wildcard domain expansion about which Martin and I exchanged comments.
Flags: needinfo?(pkerr)
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
Can we use public suffix + 1?
(In reply to Martin Thomson [:mt] from comment #4)
> Can we use public suffix + 1?

Ethan, what is your reason for matching more that one level of sub-domains? What Martin is proposing is similar to certificate validation. (see comment https://bugzilla.mozilla.org/show_bug.cgi?id=1037424#c16)
Flags: needinfo?(ethanhugg)
Version with single level wildcard implemented.
Attachment #8468934 - Flags: feedback?(ethanhugg)
Comment on attachment 8468934 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

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

::: dom/media/MediaManager.cpp
@@ +155,5 @@
> +        NS_WARNING("Ill-formed domain pattern");
> +        return false;
> +      }
> +      /*
> +        FIXME(PRK) determine how many sub-domains are allowed to pass for a wildcard.

I have not received the official word yet but I would like to say that this will work for us as I have not seen any of our sites with multiple sub-domains.  I'm inclined to go with this for now.
Attachment #8468934 - Flags: feedback?(ethanhugg) → feedback+
One level will probably work for us but I haven't got all info from our side yet.   

Does this patch satisfy the Mozilla's security needs or will you need to add checking of the public suffix list?
Flags: needinfo?(ethanhugg)
Comment on attachment 8468934 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

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

::: dom/media/MediaManager.cpp
@@ +110,5 @@
>  using dom::SupportedAudioConstraints;
>  using dom::SupportedVideoConstraints;
>  
> +static bool
> +HostInDomain(const nsACString &aHost, const nsACString &aDomain)

aDomain is the pattern?  Call it aPattern then.

This function could be a lot shorter and easier to read.  We don't need to validate the inputs here.  One has already been validated as a URL and if the other one doesn't match that, then it's merely ineffectual as a method for enabling something.

if (aPattern.Length() > 2 && aPattern[0] == '*' && aPattern[1] == '.') {
  int32_t dot = aHost.FindChar('.');
  if (dot <= 0) { // < 0 would be enough, but the <= is harmless
    return false;
  }

  host = Substring(aHost, dot);
  pattern = Substring(aPattern, 2);
} else {
  host = aHost;
  pattern = aPattern;
}

// have you verified that the pref is converted in ACE form before this point
// and that it's safe to do so on a name that is essentially invalid
// (i.e., one starting with '*' ?  Because people do enter prefs by hand, and
// people don't understand the finer points on different unicode normalization
// forms so that they get their input exactly correct.

return host == pattern;
(In reply to Martin Thomson [:mt] from comment #9)
 
> This function could be a lot shorter and easier to read.  We don't need to
> validate the inputs here.  One has already been validated as a URL and if
> the other one doesn't match that, then it's merely ineffectual as a method
> for enabling something.
> 
In this case, the URI is taken from the DOM for the document and should be in correct (ASCII) form. Removing the explicit checks of the domain pattern against bad formats mentioned in previous comments, except the "*." leading wildcard and the need to have at least one subdomain, could be handled as you demonstrate. I am comfortable with that approach as we are not able to provide any feedback as to a bad wildcard format in anything beyond a log file; we have no UI other than about:config.

I will double check that the ConvertUTF8toACE service call can handle the '*'.
Simplified version based on feedback. ConvertUTF8ToACE does appear to correctly handle subdomain consisting of only '*'
Attachment #8468934 - Attachment is obsolete: true
Removed now unecessary include of ctype.h
Attachment #8469626 - Attachment is obsolete: true
Attachment #8469634 - Flags: review?(martin.thomson)
Attachment #8469634 - Flags: review?(ethanhugg)
Attachment #8469634 - Flags: review?(martin.thomson) → review+
Comment on attachment 8469634 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

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

lgtm
Attachment #8469634 - Flags: review?(ethanhugg) → review+
Whiteboard: [p=1]
Keywords: checkin-needed
Priority: -- → P1
Target Milestone: --- → mozilla34
Version: unspecified → Trunk
Whiteboard: [p=1] → [p=1][screensharing-uplift]
https://hg.mozilla.org/mozilla-central/rev/becf5a2202d7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-
Comment on attachment 8469634 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

Approval Request Comment
[Feature/regressing bug #]: screensharing whitelist

[User impact if declined]: Have to manually list all relevant subdomains.  Important to the planned use of the feature

[Describe test coverage new/current, TBPL]: manual testing

[Risks and why]: very low risk other than a bug could make it overly permissive or not permissive enough.  

[String/UUID change made/needed]: none
Attachment #8469634 - Flags: approval-mozilla-aurora?
Not that I think this is going to happen (I reviewed the code and it looked fine), but I don't think we should understate the impact of "overly permissive" in this context.  Screensharing, particularly as embodied here, risks destroying cross-origin content protection.  We'd still have a consent dialog, but we've seen (repeatedly) that users are incapable of understanding the implications of a choice to permit screensharing.
(In reply to Martin Thomson [:mt] from comment #17)
> Not that I think this is going to happen (I reviewed the code and it looked
> fine), but I don't think we should understate the impact of "overly
> permissive" in this context.  Screensharing, particularly as embodied here,
> risks destroying cross-origin content protection.  We'd still have a consent
> dialog, but we've seen (repeatedly) that users are incapable of
> understanding the implications of a choice to permit screensharing.

Agreed, though I was referring to "if the code has a bug" it might be overly permissive.  Though the code overall is pretty simple.
Attachment #8469634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [p=1][screensharing-uplift] → [p=1]
Comment on attachment 8469634 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

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

::: dom/media/MediaManager.cpp
@@ +159,3 @@
>       Test each domain name in the comma separated list
>       after converting from UTF8 to ASCII. Each domain
>       must match exactly: no wildcards are used.

You forgot to update this comment, which is now wrong.
You need to log in before you can comment on or make changes to this bug.