Closed Bug 1197791 Opened 4 years ago Closed 4 years ago

Password logged to Error Console

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
b2g-master --- fixed

People

(Reporter: psychonaut, Assigned: dragana)

References

Details

(Keywords: privacy, sec-low, Whiteboard: [post-critsmash-triage][adv-main43-] local machine access requried.)

Attachments

(1 file, 2 obsolete files)

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
Flags: needinfo?(jduell.mcbugs)
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
Flags: needinfo?(jduell.mcbugs)
See Also: → 1198194
Given the similar bug do we want to strip this out at an earlier point?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: privacy, sec-low
Whiteboard: local machine access requried.
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
Attached patch bug_1197791.patch (obsolete) — Splinter Review
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.
Group: core-security → network-core-security
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-
Attached patch bug_1197791.patch (obsolete) — Splinter Review
Attachment #8652815 - Attachment is obsolete: true
Attachment #8652815 - Flags: review?(mrbkap)
Attachment #8657779 - Flags: review?(mrbkap)
Attachment #8657779 - Flags: review?(michal.novotny)
Attachment #8657779 - Flags: review?(michal.novotny) → review+
Blocks: 1202186
Attachment #8657779 - Flags: review?(mrbkap) → review+
Attachment #8657779 - Attachment is obsolete: true
Attachment #8659253 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5d35e376551
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Group: network-core-security → core-security-release
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.
Alias: CVE-2015-7209
Alias: CVE-2015-7209
Whiteboard: [post-critsmash-triage][adv-main43+] local machine access requried. → [post-critsmash-triage][adv-main43-] local machine access requried.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.