All PDFs served over HTTP have a crossed-out lock icon with the warning "Connection is Not Secure: Your login could be compromised". Examples: * http://www.grandmotherfish.com/grandmotherfish.pdf * http://www.longwood.edu/assets/chemphys/FusionRoute.pdf

Paolo, do you know why this could be happening? These files don't have passwords, so onDOMFormHasPassword shouldn't even be called.

There is indeed a password field in the PDF.js viewer. The point is that we should consider the PDF.js viewer itself as secure, since it's chrome code just like an "about" page.

Ah, then this is probably because we are using document.documentURI instead of the pages loadingPrincipal. I bet the principal is system, but the uri in the address bar is that of the webpage hosting the pdf. The documentURI change is hard. Is there an easier way to fix this bug in the shorter term? Paolo, any ideas?

Bug 1217766 - All PDFs trigger the insecure password warning. r=bgrins

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Indeed for the PDF.js case the principal of the document we use for the application/pdf content viewer is a "resource:" codebase principal, so using the principal URI just for the insecure passwords checks fixes the problem. Are you aware of any regressions this approach might cause? I've tested with normal HTTP and HTTPS documents as well as image content types, and it seems things continue to work well.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz https://reviewboard.mozilla.org/r/24085/#review21529 I don't know the implications of changing documentURIObject to nodePrincipal.URI but to start with, there should be a regression test for this added to browser_insecureLoginForms

(In reply to Brian Grinstead [:bgrins] from comment #6) > I don't know the implications of changing documentURIObject to > nodePrincipal.URI but to start with, there should be a regression test for > this added to browser_insecureLoginForms If I don't hear about potential regressions from Tanvi, I'll go ahead and write a test.

I'm trying to determine the consequences of changing from documentURI to nodePrincipal.URI. nodePrincipalURI is superior to documentURI and is more reliable for these types of security decisions (see https://bugzilla.mozilla.org/show_bug.cgi?id=1215344). This will may cause inconsistencies with the password manager which looks at documentURI. Particularly when we implement https://bugzilla.mozilla.org/show_bug.cgi?id=1217152 - we will check the nodePrincipalURI for security but then be filling passwords based off of documentURI. We should also update the two callers in https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#88 so that the webconsole and security UI are consistent. (Why don't I get insecure password messages in the webconsole on pdf pages?). The checkIfURIisSecure() method may also be changing in https://bugzilla.mozilla.org/show_bug.cgi?id=1217133 to call IsURIPotentiallyTrustworthy() implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1221365. This may also change the implications of this change. Hence, I need some more time to think about this and what the right path forward here is.

Some other things to consider: What happens on pages in an iframe sandbox? They will have a null nodePrincipalURI. But the URI that is actually loaded may or may not be https. How will we handle that case? What if we only check the nodePrincipalURI for documents that have no parents (top level documents) and who's nodePrincipalURI.scheme matches resource:.

Temporarily reducing priority to accommodate the addition of Bug 1223049 to the Release 45 plan. Will be reconsidered in the near future if production capacity exists.

I haven't had time to dig into this. I wonder if that inconsistency between the security UI and password manager will matter that much. We should make the webconsole and the security UI match though. And we should also figure out how to handle sandboxed loads. Is just checking for resource: a better option here? Paolo, what do you think? I know you are out for a bit, so I'll wait until December for a response.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz r? to MattN - Matt, are you okay with this change?

Tanvi and myself discussed this briefly yesterday, hence the review request. The idea is that the logic used for the passwords warning can be different from the one used for passwords autocomplete. Even if we hit some of the edge cases noted in previous comments for the passwords warning, which would be rare, this may only result in an incorrect indication, never in incorrect autocomplete. Another possibility is to add special code to whitelist the PDF.js origin only, since this is the main case reported in Nightly until now. A test case for pdf.js can already be written regardless of the final solution.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24085/diff/1-2/

This test case replicates what pdf.js does, see TestStreamConverter.onStartRequest here: https://reviewboard.mozilla.org/r/24085/diff/2#3 It's apparently a really custom way of setting up the document loading process. The actual pdf.js code is also slightly more complex, in that it passes the original channel as the first argument of the stream listener callbacks, while the data is from the new channel: http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#977 This didn't make a difference for the test case, so I used the simpler approach since it still recreated the situation we wanted to test, where the channel's originalURI is HTTP but the document node principal is a codebase "resource:". This principal is the one that is manually set up as "owner" in TestStreamConverter.onStartRequest. However, I don't understand why pdf.js should pass the original channel to the listener, and I guess we really need to ask someone from platform. Boris, who is familiar with the loading process, is currently away. Matt, do you have anyone in mind to whom we could ask?

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz https://reviewboard.mozilla.org/r/24085/#review25497 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:419 (Diff revision 2) > let isInsecure = > parentIsInsecure || > - !this.checkIfURIisSecure(doc.documentURIObject); > + !this.checkIfURIisSecure(doc.nodePrincipal.URI || > + doc.documentURIObject); I don't fully understand the security of resource: URIs so I'll defer to someone else who can take a look at https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1132 and https://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#508 Depending on how much we can trust resource URIs in general, another solution would be to add "resource" to the scheme whitelist in `isURIPotentiallyTrustworthy` as permitted by the spec.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Also, if it's decided that we can't trust all pdf.js resource URIs then we could also mitigate the issue by having pdf.js dynamically add the <input type=password> only when encountering a protected PDF (which is probably quite rare). Dan, can you review or delegate to someone else who knows about resource URIs?

Do system principals have a valid nodePrincipal.URI? I didn't think so, but this patch seems to be working for something that does have the system principal. The URI flags checked in checkIfURIIsSecure() make sense when we pass in documentURI, but not when we pass in nodePrincipal.URI. For example, no principal URI would start with javascript: but instead inherit a principal. So we need to rethink the flags we check here: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1139 I'll look into this more and also talk to Dan when he is back.

We should add a check for systemprincipal and treat those as secure. Looking at the 4 protocol flags more closely: * URI_INHERITS_SECURITY_CONTEXT - e.g. * "javascript" The nodePrincipal will be inherited and checked properly by isURIPotentiallyTrustworthy, so we don't need to check this flag. * URI_DOES_NOT_RETURN_DATA - e.g. * "mailto" * "tel" * "sms" If a top level page or an iframe has a mailto scheme, what principal does it have? There should be no content, so no risk of a password field. This check shouldn't hurt or help, and hence should be safe to remove. * URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT * "https", * "moz-safe-about" Both these cases should be covered if we check the principal. the https check is done in isURIPotentiallyTrustworthy and moz-safe-about should be covered by system principal. * URI_IS_LOCAL_RESOURCE - e.g. * "data", * "resource", * "moz-icon" * "file" There are a number of protocols we set this URI flag for [1]. If they all result in a systemPrincipal load, we don't need to check this flag. But this needs to be tested. [1] chrome - http://mxr.mozilla.org/mozilla-central/source/chrome/nsChromeProtocolHandler.cpp#65 host object? - http://mxr.mozilla.org/mozilla-central/source/dom/base/nsHostObjectProtocolHandler.cpp#492 data handler? - http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/data/nsDataHandler.cpp#56 extension protocol handler - http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/ExtensionProtocolHandler.cpp#43 android protocol handler? - http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAndroidProtocolHandler.cpp#133 file - http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/file/nsFileProtocolHandler.cpp#150 resource - http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.h#28 anno protocol handler (I think this has to do with stored favicons) - http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsAnnoProtocolHandler.cpp#238 moz-protocol-local-resource - http://mxr.mozilla.org/mozilla-central/source/chrome/test/unit/test_no_remote_registration.js#96 page thumbs? - http://mxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbsProtocol.js#63

I spoke to Paolo this morning. Sounds like the resource protocol has a codebaseprincipal (not system), so we do need to check for the protocol flag URI_IS_LOCAL_RESOURCE. (paolo, can you point me to the code that creates a codebaseprincipal?) This is what needs to be done here: * Changes to checkIfURIIsSecure ** Change the parameter passed in to a principal instead of a uri. ** Check for system principal and return secure at the top of checkIfURIIsSecure ** Then check the principalURI against isURITrustowrhty and URI_IS_LOCAL_RESOURCE. * Change callers of checkIfURIIsSecure to pass a principal instead of a uri ** http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#70 and line 88 ** http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#421

(In reply to Tanvi Vyas [:tanvi] from comment #20) > (paolo, can you point me to the code that creates a codebaseprincipal?) It's basically the same code block I pointed out in comment 15: http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#1019 It's also the block I'd like to be reviewed by someone on the platform team, to validate the test case.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24085/diff/2-3/

This version I'm quite comfortable with. It also solves bug 1221771.

https://reviewboard.mozilla.org/r/24085/#review26265 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1126 (Diff revision 3) > + // These may be expanded principals or null principals that we don't What if an https top level page includes an iframe sandbox to another https page. The second https page my have a password field in it. This doesn't mean that the page is insecure, so returning false wouldn't be right in that case. On the other hand, framed login pages are confusing because users don't know which credentials to enter (those of the framed page or those of the top level page). Users don't understand frames. This is a wider problem on the web though, and hence sandboxed frames shouldn't be the only ones punished for it. So we need to find a better way to handle nullprincipal loads. Is there another way to get the URI associated with them? We could do a nullprincipal check and if true use document.documentURI. In order to do this though, we'd have to pass the document in instead of the document.nodePrincipal. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1131 (Diff revision 3) > - return isSafe; > + // These checks include "file", "resource", HTTPS, and HTTP to "localhost". Can you call out in the comments that "resource" is checked by the protocol flag, while the rest of the schemes mentioned are from isURIPotentiallyTrustworthy(). You may want to split out the tests into a separate patch and r? that to Matt instead of Dan.

(In reply to Tanvi Vyas [:tanvi] from comment #24) > What if an https top level page includes an iframe sandbox to another https > page. The second https page my have a password field in it. I'm not familiar with sandboxed iframes. Do you have a test case? > So we need to find a better way to handle nullprincipal loads. Do sandboxed documents actually get a null principal? Since a null principal fails all security checks, can an element with a null principal do anything with a password it obtains? Maybe we should just consider null principals as secure?

(In reply to :Paolo Amadini from comment #25) > (In reply to Tanvi Vyas [:tanvi] from comment #24) > > So we need to find a better way to handle nullprincipal loads. > > Do sandboxed documents actually get a null principal? They get a nsNullPrincipal, not a principal set to null. In hindsight might have been a confusing name, think of it as nsUniqueOriginPrincipal instead.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz https://reviewboard.mozilla.org/r/24085/#review26789 resource: is a "substituting protocol handler" and I don't see anything that technically limits resource:/foo/<path> to resolving to a file: url. In fact we have tests that make resource:/testing/ map to various chrome: URLs -- which are still local but nothing stops someone from mapping a resource to a remote URL. That would violate assumptions a lot of code makes so I sure hope no one does that. It would be safer to explicitly check for resource: as a "potentially trustworthy" URI than to include all URI_IS_LOCAL_RESOURCE because some of those other "local" resources have been downloaded insecurely and there's no way to tell from the scheme. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1120 (Diff revision 3) > - * > + if (docPrincipal.isSystemPrincipal) { What if the principal is literally null? Is that impossible now? If not won't this throw when you try to check .isSystemPrincipal (and later .URI)? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1131 (Diff revision 3) > - return isSafe; > + // These checks include "file", "resource", HTTPS, and HTTP to "localhost". comment is wrong to say "HTTP" is included. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1133 (Diff revision 3) > + gNetUtil.URIChainHasFlags(uri, As Tanvi pointed out URI_IS_LOCAL_RESOURCE includes a lot of things and may include more in the future. The ones I see are mostly OK, except nsHostObjectProtocolHandler. It was marked URI_IS_LOCAL_RESOURCE back when bug 543870 landed File.url. This later morphed into blob:, and even later morphed into media streams and rtsp. None of those are necessarily local resources. Even blob which points at a *currently* local resource came from a web page loaded either securely or insecurely. Why not just add "resource:" to the explicit list inside CSP::isURIPotentiallyTrustworthy() -- that one we can (currently) be sure about.

https://reviewboard.mozilla.org/r/24085/#review26789 > comment is wrong to say "HTTP" is included. I missed the comma after "HTTPS" and misread it as "HTTPS and HTTP, and localhost". Comment is fine as-is. As Tanvi points out we should also add resource: to the comment.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24085/diff/3-4/

I've updated the logic of isURIPotentiallyTrustworthy as Dan suggested. The LoginManagerContent.isDocumentSecure function still doesn't handle all the cases, but as noted in bug 1162772 comment 21, that would be quite complicated and I think out of scope for resolving the specific issue that would allow us to move the insecure passwords check to the Developer Edition. Most null principals are now handled correctly by falling back to the document URI. Note that I haven't added a check for document.nodePrincipal being null as I'm not aware of cases where that could happen.

Still not sure whom to ask about comment 15, as Boris is still away. Bobby, maybe you can take a look or redirect the request?

Will look at comment 15 in detail tomorrow morning, but in the meantime, we should really have a concept of "secure context" that's more than just a URI and be using it here, no?

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz https://reviewboard.mozilla.org/r/24085/#review27649 I'm still deferring review of isDocumentSecure and /dom/ to dveditz/bz. ::: toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js:91 (Diff revision 4) > + gBrowser.removeTab(tab); Isn't BrowserTestUtils.removeTab preferred? ISTR intermittent failures using removeTab (from e10s?) because removeTab doesn't wait for all work related to removing to complete. Maybe this test won't be affected as-is so it's up to you but would like like us to prefer the safer method. ::: toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js:91 (Diff revision 4) > + gBrowser.removeTab(tab); > + > + yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () { > + this.cleanupFunction(); Aren't you trying to call cleanupFunction in the streamConverter_content.sjs which you just closed? Why not cleanup before removing? Could you make this more explicit by referring to `browser` instead of `gBrowser.selectedBrowser`, if that's the case, as I thought `gBrowser.selectedBrowser` would have changed synchonously by `removeTab` meaning the cleanup wouldn't work. Please ensure this cleanup code is actually working.

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz In addition to the information for comment 15, the patch will also need a review for the DOM part. We'd like to have this ready in this release to enable the Insecure Login Forms Warning in Developer Edition 46.

OK, so as far as comment 15 goes... A fundamental contract for necko is that you either pass in the original channel (which the caller might be using as a key in a hashtable or whatnot, and often does) or you dispatch a redirect notification from the original channel to the new channel if you want to use a different channel. In any case, passing in a different channel would not help much with the URI issue, since the whole point is pdfjs wants to keep showing the original PDF URI in the URL bar, so that new channel would also need to have that URI, right? So if our issue is that someone is using the URI to decide on secure vs not then we would have that problem still. Or is that not what we're doing?

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz https://reviewboard.mozilla.org/r/24085/#review27909 r=me on the DOM changes (and feedback+ at least on the rest; I did skim those parts over and they look pretty good!).

Comment on attachment 8682534 [details] MozReview Request: Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24085/diff/4-5/

(In reply to Boris Zbarsky [:bz] from comment #35) > you either pass in the original channel Which is curious for stream converters, because then you have to change the content type of the original channel like pdf.js does or the load process enters an infinite loop... > or you dispatch a redirect > notification from the original channel to the new channel if you want to use > a different channel. So, the stream converter implementation in the test isn't perfect as it doesn't dispatch this notification. That's fine since the test works anyways, but out of curiosity, what should we do at line 44 here to make the proper redirect notification? https://reviewboard.mozilla.org/r/24085/diff/5#6 > In any case, passing in a different channel would not help much with the URI > issue, since the whole point is pdfjs wants to keep showing the original PDF > URI in the URL bar, so that new channel would also need to have that URI, > right? So if our issue is that someone is using the URI to decide on secure > vs not then we would have that problem still. Or is that not what we're doing? Correct, we cannot check the URI alone. We now use the hint provided by the principal that's manually set by pdf.js (that as far as I can tell could even have been the system principal). Given the reviews I think that's good enough for now and will land the patch with the minor test changes Matt suggested, after a try build.

https://reviewboard.mozilla.org/r/24085/#review28203 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1131 (Diff revisions 3 - 4) > + let uri = docPrincipal.isCodebasePrincipal ? docPrincipal.URI Perhaps we should file a followup to handle expandedPrincipals. We could check each principal in the array to see if it is secure. Or we could just check if the principal is an expanded principal[1] and consider it secure, like mixed content blocker does. Would we have document.documentURIObject in the case of expanded principals? I'm not sure, but I guess we'd have to because otherwise we would fail this check[2]. [1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#603 https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Script_security#Expanded_principal [2] https://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#511

> Which is curious for stream converters, because then you have to change the content type Yep. > That's fine since the test works anyways You got lucky that this is a document load. For some other load types, this would leak. ;) > what should we do at line 44 here to make the proper redirect notification? You want to send an AsyncOnChannelRedirect notification to the nsIIOService and also look for an nsIChannelEventSink on the channel and then it's loadgroup. If found, then you send AsyncOnChannelRedirect to that as well. Note that these are allowed to cancel the redirect, and can do so async, so you have to wait for them to respond to your notification.... In necko, netwerk/base/nsAsyncRedirectVerifyHelper.cpp is used to do all the notification-sending, waiting, etc.

Verified fixed FF 46.0a2 (2016-01-25) Win 7, Ubuntu 14.04, OS X 10.10.5.