1049583 - Screensharing allowed_domains needs limited wildcard matching.

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

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"

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 2

[Ethan Hugg [:ehugg]]

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.

Comment 3

[Paul Kerr [:pkerr]]

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

Comment 4

[Martin Thomson [:mt:]]

Can we use public suffix + 1?

Comment 5

[Paul Kerr [:pkerr]]

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

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

Version with single level wildcard implemented.

Comment 7

[Ethan Hugg [:ehugg]]

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.

Comment 8

[Ethan Hugg [:ehugg]]

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?

Comment 9

[Martin Thomson [:mt:]]

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;

Comment 10

[Paul Kerr [:pkerr]]

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

Comment 11

[Paul Kerr [:pkerr]]

Attachment: Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

Simplified version based on feedback. ConvertUTF8ToACE does appear to correctly handle subdomain consisting of only '*'

Comment 12

[Paul Kerr [:pkerr]]

Attachment: Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

Removed now unecessary include of ctype.h

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 8469634 [details] [diff] [review]
Allow sub-domain wildcard on ScreenSharing domain whitelist patterns

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

lgtm

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 17

[Martin Thomson [:mt:]]

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.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/104c671afe11

Comment 20

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

comment-only fix rs=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/d44797d191e5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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