Closed Bug 298823 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: Security: CAPS, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: ma1, Assigned: ma1)

References

()

Details

(Keywords: fixed-aviary1.0.8, fixed1.7.13, fixed1.8, Whiteboard: [sg:fix])

Attachments

(1 file)

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
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
Assignee: dveditz → g.maone
Status: NEW → ASSIGNED
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.
Attachment #190582 - Flags: superreview?(dveditz)
Attachment #190582 - Flags: review?(caillon)
Attachment #190582 - Flags: approval1.8b4?
Attachment #190582 - Flags: approval1.7.11?
Attachment #190582 - Flags: approval-aviary1.0.7?
Attachment #190582 - Flags: approval1.7.11? → approval1.7.11-
Attachment #190582 - Flags: approval1.7.11- → approval1.7.12?
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.
Attachment #190582 - Flags: approval1.8b4?
Attachment #190582 - Flags: approval1.7.12?
Attachment #190582 - Flags: approval-aviary1.0.7?
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.
Flags: blocking1.8b4?
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.5?
Flags: blocking1.8b4?
Flags: blocking1.8b4+
Flags: blocking-aviary1.5?
Flags: blocking-aviary1.5+
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)
Attachment #190582 - Flags: superreview?(dveditz) → superreview+
Attachment #190582 - Flags: review?(caillon) → review+
Attachment #190582 - Flags: approval1.8b4?
Attachment #190582 - Flags: approval-aviary1.0.7?
Checking in nsScriptSecurityManager.cpp; /cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v <-- nsScriptSecurityManager.cpp new revision: 1.270; previous revision: 1.269 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Is this still needed on the Gecko 1.8 branch?
(In reply to comment #7) > Is this still needed on the Gecko 1.8 branch? Yes, it has been landed on trunk only
Attachment #190582 - Flags: approval1.8b4? → approval1.8b4+
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
Keywords: fixed1.8
Target Milestone: mozilla1.9alpha → mozilla1.8beta4
Whiteboard: [sg:fix]
Attachment #190582 - Flags: approval1.7.12?
Attachment #190582 - Flags: approval-aviary1.0.7?
Flags: blocking1.7.13?
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Comment on attachment 190582 [details] [diff] [review] jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy() a=dveditz for drivers
Attachment #190582 - Flags: approval1.7.13+
Attachment #190582 - Flags: approval1.7.12?
Attachment #190582 - Flags: approval-aviary1.0.8?
Attachment #190582 - Flags: approval-aviary1.0.8+
Attachment #190582 - Flags: approval-aviary1.0.7?
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;
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: