1197791 - Password logged to Error Console

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tristan Miller]

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:[e-mail]/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.

Comment 1

[Tristan Miller]

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.

Comment 2

[Jason Duell]

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

Comment 3

[Dragana Damjanovic [:dragana]]

The error is at:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1826

I will make a patch

Comment 4

[Daniel Veditz [:dveditz]]

Given the similar bug do we want to strip this out at an earlier point?

Comment 5

[Dragana Damjanovic [:dragana]]

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 ...)

Comment 6

[Dragana Damjanovic [:dragana]]

Can you please cc me on bug 1198194 if it is similar to this one. I do not see it.
Thanks

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1197791.patch

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.

Comment 8

[Dragana Damjanovic [:dragana]]

Comment on attachment 8652815 [details] [diff] [review]
bug_1197791.patch

I am not sure if you  are the right person to review this.
Thanks!

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Dragana Damjanovic [:dragana]]

Comment on attachment 8652815 [details] [diff] [review]
bug_1197791.patch

Michal, can you please review the nsIURI part, thanks.

Comment 11

[Michal Novotny [:michal]]

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.

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: bug_1197791.patch

Comment 13

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=378effc6f0e4

Comment 14

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df7d16cc801

Comment 15

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9840d22fee

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1197791.patch

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5d35e376551