298823 - JAR URIs (and other types missing the host part) are not properly handled by nsScriptSecurityManager::LookupPolicy()

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Giorgio Maone [:ma1]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

While nsScriptSecurityManager::SecurityCompareURIs() addresses specifically JAR
URIs by comparing their base URIs (
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#247
), weird things happen when LookupPolicy() tries to check a JAR URI against
domain policies.

Since nsPrincipal::GetOrigin() -
http://lxr.mozilla.org/seamonkey/source/caps/src/nsPrincipal.cpp#185 - returns
the whole URI instead of the domain only, mOriginToPolicyMap can't be looked up
by domain, because the naive domain parsing, based to "lastDot" and
"nextToLastDot" is fooled by dots that *always* follow the domain in a JAR URI:
at the very least the file extension separator at the end of the jarFile part
(i.e. ".jar").
OTOH, InitPolicy() populates mOriginToPolicyMap by the same
lastDot/nextToLastDot hack, so it cannot use a partial JAR URI specification as
a key that can be matched later by LookupPolicies().
Result is that the only capabilities.policies.myCustomPolicy.sites URI that
matches a JAR URI is "jar:", which matches *every* JAR URI. 
This is obviously too permissive for whitelists and too restrictive for
blacklists, considerably reducing CAPS usefulness.

The same problem affects other URI types missing host part, but the JAR case is
IMHO the most severe: deciding a permission that applies to all file:// URIs is
a matter of trusting your whole filesystem, which is wide but more acceptable
than putting together jar:chrome:// and jar://http://www.evilhackerz.ru ;-)

I believe there are two possible, mutually exclusive fixes, both involving
treating JAR  URIs as a special case (as SecurityCompareURIs already does):

1. In LookupPolicy(), check if principal origin is a jar: URI and if it is use
the part after the first colon for matching
2. In InitPolicy(), check if domain spec that is going to be added contains "/"
after its scheme: if this is the case, use its scheme rather than domainStart as
mOriginToPolicyMap key, and use whole URL spec to construct the DomainEntry (
see
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#3052 ).

Solution #1 would be probably more consistent with user expectation that
www.somedomain.com should match
jar:http://www.somedomain.com/somejar.jar!/somefile.html as well.
Solution #2 would allow finer grained control on jars and would fix the file://
problem, allowing matching of file:///home/john/myscripts/ URIs.

Reproducible: Always

Steps to Reproduce:
1. Go to http://software.informaction.com/test/capstest.html and follow
instructions to disable and reenable JavaScript
2. Go to jar:http://software.informaction.com/capstest.jar!/capstest.html and
follow instructions to disable and reenable JavaScript


Actual Results:  
I could successfully disable JavaScript on 
http://software.informaction.com/test/capstest.html

I could not disable JavaScript on
jar:http://software.informaction.com/capstest.jar!/capstest.html

Expected Results:  
CAPS should have allowed me to disable JavaScript on both URLs

Comment 1

[Daniel Veditz [:dveditz]]

solution 2 sounds great for file: urls. For jar: the only sane thing is to
unwrap it and use the internal domain. "jar" is a pseudo-protocol that tells us
how to treat the thing inside (like view-source:), any permission decisions
ought to be made on the thing itself.

Don't forget that jar: can be nested-- keep unwrapping until you hit a non-jar
scheme

Comment 2

[Giorgio Maone [:ma1]]

Attachment: jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy()

It fixes the main issue with jar: URIs matching, delegating it to the nested
URI.
Now a domain policy for "www.acme.com" matches
jar://http://www.acme.com/some.jar!somefile.htm as expected.
I could have fixed it in nsPrincipal::GetOrigin(), but I didn't want to take
the risk of breaking some caller.
Since GetOrigin() as it is returns the full URI spec for jar://* and other URIs
which are not guaranteed to have an host part, I had also to limit the origin
matching to the 3rd slash. 
The fine grained (path aware) file:// matching, instead, will have to wait
until we're sure we can touch GetOrigin(), because it currently returns
file://[host]/ (no path), since file:// URIs are actually supposed to have an
host part (possibly empty).
While I was studying the code I've found also one other (not usually reachable)
place where DomainPolicy::Drop() should be used instead of delete, and patched
it as well.

Comment 3

[Benjamin Smedberg]

Comment on attachment 190582 [details] [diff] [review]
jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy()

Please do not request approvals until reviews are complete.

Comment 4

[Giorgio Maone [:ma1]]

Requesting blocker status because this bug causes CAPS to be practically
worthless for black-list usage, allowing only "all or nothing" policies for jars. 
1) If you forget to put "jar:" (ALL jars) in your black-list, no domain can be
effectively be banned.
2) If you remember to put "jar:" (ALL jars) in your black-list, you will loose
too much functionality (e.g. Lotus Web Mail client runs from jar: URIs).

The first case is very likely to happen, since this flaw is not documented in
the CAPS page: until this bug is fixed, every black-list policy *must* include a
"jar:" site entry, otherwise is worthless.

Coming to the nsIURI versus custom string processing argument, I'm all for
(re)using nsIURI instances (probably nsIPrincipal needs to be refactored), but
my patch touches code which is *already* performing low-level parsing hacks.
Actually, it is placed inside a loop that *currently* receives a string from
nsIPrincipal and uses a lastdot+nextToLastdot (broken) trick to extract domain:
did you notice 2nd level domains can't be matched unless they are prefixed by a
scheme in the policy sites pref ("domain.com" doesn't match
"http://domain.com"), while 3rd level and up work as they are ("www.domain.com"
*does* match "http://www.domain.com")? Fortunately this one is easy to work
around (put three or more entries instead of one, e.g. "domain.com,
http://domain.com, https://domain.com"), but rather annoying if you want to
build a domain-wide policy. 

I think I can open another bug for the nsIURI refactoring (which would certainly
fix the forementioned 2nd level domain issue), but IMHO this patch fits well in
its  context, fixes this bug *as needed*, performs good and has no side effects.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 190582 [details] [diff] [review]
jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy()

sr=dveditz (but very tired, caillon please look this over)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Checking in nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <-- 
nsScriptSecurityManager.cpp
new revision: 1.270; previous revision: 1.269
done

Comment 7

[Jesse Ruderman]

Is this still needed on the Gecko 1.8 branch?

Comment 8

[Giorgio Maone [:ma1]]

(In reply to comment #7)
> Is this still needed on the Gecko 1.8 branch?
Yes, it has been landed on trunk only

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Checking in nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <-- 
nsScriptSecurityManager.cpp
new revision: 1.266.2.2; previous revision: 1.266.2.1
done

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 190582 [details] [diff] [review]
jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy()

a=dveditz for drivers

Comment 11

[:Gavin Sharp [email: [e-mail]]]

1.7 branch:
mozilla/caps/src/nsScriptSecurityManager.cpp; new revision: 1.229.2.19;
aviary 101 branch:
mozilla/caps/src/nsScriptSecurityManager.cpp; new revision: 1.229.6.6.2.15;