Closed Bug 298823 Opened 16 years ago Closed 16 years ago
JAR URIs (and other types missing the host part) are not properly handled by ns
Script Security Manager::Lookup Policy()
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
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: 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.
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 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+
16 years ago
Attachment #190582 - Flags: review?(caillon) → review+
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: 16 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
Target Milestone: mozilla1.9alpha → mozilla1.8beta4
Comment on attachment 190582 [details] [diff] [review] jar: unwrapping and 3 slashes stop in nsScriptSecurityManager::LookupPolicy() a=dveditz for drivers
1.7 branch: mozilla/caps/src/nsScriptSecurityManager.cpp; new revision: 22.214.171.124; aviary 101 branch: mozilla/caps/src/nsScriptSecurityManager.cpp; new revision: 126.96.36.199.2.15;
You need to log in before you can comment on or make changes to this bug.