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

RESOLVED FIXED in Instantbird 43

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: florian, Assigned: aleth)

Tracking

trunk
Instantbird 43

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1.6-blocking])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Currently when the PLAIN authentication mechanism is used, the debug log contains the user's password.
(Assignee)

Updated

4 years ago
Whiteboard: [1.6-blocking]
(Assignee)

Comment 1

4 years ago
Posted patch xmppauthlog.diff (obsolete) — Splinter Review
Attachment #8645460 - Flags: review?(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
Attachment #8645460 - Flags: review?(clokep) → review+
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Posted patch xmppauthlog.diff v2 (obsolete) — Splinter Review
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?
Attachment #8645460 - Attachment is obsolete: true
Attachment #8645469 - Flags: review?(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.
Attachment #8645469 - Flags: review?(clokep) → review+
(Assignee)

Comment 6

4 years ago
Posted patch xmppauthlog.diff v3 (obsolete) — Splinter Review
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.
Attachment #8645469 - Attachment is obsolete: true
Attachment #8645482 - Flags: review?(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...
(Assignee)

Comment 8

4 years ago
(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 on attachment 8645482 [details] [diff] [review]
xmppauthlog.diff v3

r- as discussed on IRC to split out the confusing section with encrypted.
Attachment #8645482 - Flags: review?(clokep) → review-
(Assignee)

Comment 10

4 years ago
PLAIN change removed.

I think we should land this unless someone knows any legacy auth servers for testing.
Assignee: nobody → aleth
Attachment #8645482 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8646551 - Flags: review?(clokep)
Attachment #8646551 - Flags: review?(clokep) → review+
(Assignee)

Comment 11

4 years ago
url:        https://hg.mozilla.org/comm-central/rev/ac1d626163bc50a95ab9f8f84504339a36e3a94c
changeset:  ac1d626163bc50a95ab9f8f84504339a36e3a94c
user:       aleth <aleth@instantbird.org>
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
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 43
You need to log in before you can comment on or make changes to this bug.