Don't log password during SASL auth

RESOLVED FIXED in 1.3

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1.3-blocking])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
*** Original post on bio 1786 at 2012-11-09 22:56:00 UTC ***

sendMessage needs a third parameter.
(Assignee)

Updated

5 years ago
Whiteboard: [1.3-blocking]
*** Original post on bio 1786 at 2012-11-09 23:04:12 UTC ***

(In reply to comment #0)
> sendMessage needs a third parameter.

This call: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircSASL.jsm#38
(Assignee)

Comment 2

5 years ago
Created attachment 8353839 [details] [diff] [review]
Patch

*** Original post on bio 1786 as attmnt 2079 at 2012-11-10 14:11:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353839 - Flags: review?(florian)
(Assignee)

Updated

5 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Blocks: 955004
Comment on attachment 8353839 [details] [diff] [review]
Patch

*** Original change on bio 1786 attmnt 2079 at 2012-11-10 22:53:39 UTC ***

r=me with the string changed to "<base64 encoded password, not logged>", as discussed on IRC.

Thanks for taking care of this! :-)
Attachment #8353839 - Flags: review?(florian) → review+
*** Original post on bio 1786 at 2012-11-10 23:47:50 UTC ***

http://hg.instantbird.org/instantbird/rev/ec702866871c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
*** Original post on bio 1786 at 2012-11-12 00:00:13 UTC ***

I dislike this string (and would have r-ed it), can we please change this to just
"AUTHENTICATE <base64 encoded nick, user and password not logged>"? Why is there a comma in "<base64 encoded password*,* not logged>"? That doesn't make sense.

(Also, for what it is worth I knew that we were logging this in plain text and did not find it to be an issue. I don't even think this needed patching.)
(Assignee)

Comment 6

5 years ago
Created attachment 8353845 [details] [diff] [review]
String change patch

*** Original post on bio 1786 as attmnt 2085 at 2012-11-12 13:04:00 UTC ***

String change to be even less vague ;)

(In reply to comment #5)
> (Also, for what it is worth I knew that we were logging this in plain text and
> did not find it to be an issue. I don't even think this needed patching.)
I should have caught this when reviewing the original SASL patch. I think it needs patching because either we log passwords or we don't. Since we take care not to log them for IDENTIFY, we shouldn't log them here, even if it is just to avoid possible user confusion and concern.
Attachment #8353845 - Flags: review?(florian)
(Assignee)

Updated

5 years ago
Whiteboard: [1.3-blocking] → [1.3-blocking][checkin-needed]
Comment on attachment 8353845 [details] [diff] [review]
String change patch

*** Original change on bio 1786 attmnt 2085 at 2012-11-15 02:55:51 UTC ***

Thanks for rewording this.
Attachment #8353845 - Flags: review?(florian) → review+
*** Original post on bio 1786 at 2012-11-18 18:55:55 UTC ***

(In reply to comment #7)
> Comment on attachment 8353845 [details] [diff] [review] (bio-attmnt 2085) [details]
> String change patch
> 
> Thanks for rewording this.

Committed as http://hg.instantbird.org/instantbird/rev/7ee99b705800 (for Instantbird 1.4)
Whiteboard: [1.3-blocking][checkin-needed] → [1.3-blocking]
You need to log in before you can comment on or make changes to this bug.