1163541 - JS XMPP debug logs shouldn't contain the user's password

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Florian Quèze [:florian]]

Currently when the PLAIN authentication mechanism is used, the debug log contains the user's password.

Comment 1

[aleth [:aleth]]

Attachment: xmppauthlog.diff

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8645460 [details] [diff] [review]
xmppauthlog.diff

Review of attachment 8645460 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK, do we need to do anything for GTalk's PlainFullBindAuth? https://mxr.mozilla.org/comm-central/source/chat/protocols/gtalk/gtalk.js#22

Comment 3

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #2)
> Looks OK, do we need to do anything for GTalk's PlainFullBindAuth?
> https://mxr.mozilla.org/comm-central/source/chat/protocols/gtalk/gtalk.js#22

Good catch.

Comment 4

[aleth [:aleth]]

Attachment: xmppauthlog.diff v2

Now includes gtalk, legacyAuth, and MUC passwords too.

I haven't tested these three - anyone know any password-protected MUCs and/or servers that don't support SASL?

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8645469 [details] [diff] [review]
xmppauthlog.diff v2

Review of attachment 8645469 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable. We should test this before marking checkin-needed.

Comment 6

[aleth [:aleth]]

Attachment: xmppauthlog.diff v3

Tested joining a MUC.
 
Fixed an edge case where we would select SASL PLAIN even though alternatives were available.

I don't know of a server to test legacy auth with.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8645482 [details] [diff] [review]
xmppauthlog.diff v3

Review of attachment 8645482 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +314,2 @@
>          let mech = m.innerText;
> +        if (mech == "PLAIN") {

Why the change to this._encrypted? That seems unrelated...

Comment 8

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #7)
> > +        if (mech == "PLAIN") {
> 
> Why the change to this._encrypted? That seems unrelated...

It's moved further down. The point is that if you are on an encrypted connection, then with the code as it was, we'd pick PLAIN immediately in the else clause.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8645482 [details] [diff] [review]
xmppauthlog.diff v3

r- as discussed on IRC to split out the confusing section with encrypted.

Comment 10

[aleth [:aleth]]

Attachment: xmppauthlog.diff v4

PLAIN change removed.

I think we should land this unless someone knows any legacy auth servers for testing.

Comment 11

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/ac1d626163bc50a95ab9f8f84504339a36e3a94c
changeset:  ac1d626163bc50a95ab9f8f84504339a36e3a94c
user:       aleth <[e-mail]>
date:       Wed Aug 12 01:09:49 2015 +0200
description:
Bug 1163541 - JS XMPP debug logs shouldn't contain the user's password. r=clokep