Closed Bug 1197791 Opened 4 years ago Closed 4 years ago
Password logged to Error Console
When connecting to a remote server that uses SHA-1 certificates, the following log messages are visible in the Error Console: This site makes use of a SHA-1 Certificate; it's recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1. https://USERNAME:PASSWORD@example.com/some/path These warning messages were implemented in Firefox as a result of Bug 1068949 but I suspect they were applied to some core component, because I am seeing these console messages in SeaMonkey 2.33.1 and Thunderbird 38.2.0. (In the case of Thunderbird, this is because I'm using a calendar extension which connects to a Microsoft Exchange server via HTTPS.) The security problem here is that "USERNAME" and "PASSWORD" are my real username and password. I don't think that the password should be output to the Error Console, since this makes it easy for an attacker to observe it by shoulder-surfing or by momentary access to an unattended machine. Though the SHA-1 warning is valid, it's not necessary to display the username and password for the site being accessed. Regardless of *how* this component came to be passed a URL with credentials (whether clicking on a link, direct user input into the location bar, or an API call by some browser or mail client add-on), it should strip out these credentials before logging the SHA-1 warning.
On further checking, displaying the password is condemned by RFC 3986: > Applications should not render as clear text any data > after the first colon (":") character found within a userinfo > subcomponent unless the data after the colon is the empty string > (indicating no password). Applications may choose to ignore or > reject such data when it is received as part of a reference and > should reject the storage of such data in unencrypted form. The > passing of authentication information in clear text has proven to be > a security risk in almost every case where it has been used.
Component: Security → Networking: HTTP
ccing some usual suspects. Honza probably knows how to fix this but is on PTO for a few days--if anyone can grab it sooner that would be great
The error is at: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1826 I will make a patch
Given the similar bug do we want to strip this out at an earlier point?
doing it in HttpBaseChannel is not enough, because nsDocument is loggin as well. I have decided to maybe do it later. for web console: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#1514 and for error console: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsScriptError.cpp#198 or http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsScriptError.cpp#180 (but here I need to parse url again ...)
Can you please cc me on bug 1198194 if it is similar to this one. I do not see it. Thanks
Th attached patch will fix leaking password in case of errors messages made using nsIScriptError. This does not hide password for calls of console.log(...) and console.error(...) I am not sure where is the right place to fix this. It is possible to fix it in webconsole.js but I do not thing that is the right place. Mabe someone more familier with this code could take a look.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Comment on attachment 8652815 [details] [diff] [review] bug_1197791.patch I am not sure if you are the right person to review this. Thanks!
Attachment #8652815 - Flags: review?(continuation)
Comment on attachment 8652815 [details] [diff] [review] bug_1197791.patch I'm not really the right reviewer for this. Maybe Blake could review it. (bz or bholley might also be able to, but bz has a bunch of stuff in his queue and bholley in on PTO.) I think you need to rev the UUID on the IDL interface you changed.
Attachment #8652815 - Flags: review?(continuation) → review?(mrbkap)
Comment on attachment 8652815 [details] [diff] [review] bug_1197791.patch Michal, can you please review the nsIURI part, thanks.
Attachment #8652815 - Flags: review?(michal.novotny)
Comment on attachment 8652815 [details] [diff] [review] bug_1197791.patch Review of attachment 8652815 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsIURI.idl @@ +251,5 @@ > + /** > + * Returns the anonymized spec attribute. This will only affect uri with > + * password. The password part of uri will be transformed into "****". > + */ > + AUTF8String getAnonymousSpec(); You have to change UUID when changing idl. Anyway I don't like this change. Instead of adding a method to nsIURI that is needed only for nsStandardURL, it would be better to create a new interface (e.g. nsIAnonymousURI) which will be implemented only by nsStandardURL. Btw, using word "anonymous" here is IMO not correct since only the password is hidden. Something like "passwordless" is maybe ugly, but more correct.
Attachment #8652815 - Flags: review?(michal.novotny) → review-
Attachment #8657779 - Flags: review?(michal.novotny) → review+
Attachment #8657779 - Flags: review?(mrbkap) → review+
Whiteboard: local machine access requried. → [post-critsmash-triage] local machine access requried.
Whiteboard: [post-critsmash-triage] local machine access requried. → [post-critsmash-triage][adv-main43+] local machine access requried.
Whiteboard: [post-critsmash-triage][adv-main43+] local machine access requried. → [post-critsmash-triage][adv-main43-] local machine access requried.
You need to log in before you can comment on or make changes to this bug.