Closed Bug 955218 Opened 10 years ago Closed 10 years ago

Don't log password during SASL auth

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.3-blocking])

Attachments

(2 files)

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

sendMessage needs a third parameter.
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
Attached patch PatchSplinter Review
*** 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: 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
Closed: 10 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.)
*** 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)
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.