After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771888 - mask nickserv identify passwords
mask nickserv identify passwords
Status: RESOLVED OBSOLETE
Product: polari
Classification: Applications
Component: general
3.21.x
Other Linux
: Normal enhancement
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-23 17:13 UTC by Mohammed Sadiq
Modified: 2021-06-10 11:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
util: Split out match_identify_message() method (4.46 KB, patch)
2016-09-23 23:51 UTC, Florian Müllner
committed Details | Review
chatView: Mask identify passwords in private chats (1.38 KB, patch)
2016-09-23 23:51 UTC, Florian Müllner
committed Details | Review
chatView: Be a bit silly on April 1st (1.18 KB, patch)
2016-09-23 23:52 UTC, Florian Müllner
accepted-commit_now Details | Review
Fix patch proposed for bug 771888 (913 bytes, patch)
2017-03-04 15:31 UTC, Alexánder Alzate
needs-work Details | Review
util: Match any nickname in identify command (1.09 KB, patch)
2017-03-05 00:52 UTC, Alexánder Alzate
committed Details | Review

Description Mohammed Sadiq 2016-09-23 17:13:07 UTC
Its nice that polari remembers the conversation with each user. This is also a problem when it saves the chats with nickserv.

I see identify mypass in the history of Nickserv chat. It shall be nice to mask those passwords.

Say if I do "/msg nickserv identify mypass" What I see in Nickserv private chat window should be "identify ******" (of course replacing * with the usual char for passwords, which is a smaller filled circle or a bigger \cdot (what ever it is called)).

Thanks
Comment 1 Florian Müllner 2016-09-23 17:19:09 UTC
Makes sense to me
Comment 2 Florian Müllner 2016-09-23 23:51:45 UTC
Created attachment 336176 [details] [review]
util: Split out match_identify_message() method

Matching an identify command is currently done by the room to emit
the ::identify-sent signal when the user entered the command, so
we can offer to save the password. We now want to mask the sent
password in the chat log, so split out the matching code into a
utility method.
Comment 3 Florian Müllner 2016-09-23 23:51:52 UTC
Created attachment 336177 [details] [review]
chatView: Mask identify passwords in private chats

We now send automatic identify messages when connecting, so the
sensitive password regularly appears in the NickServ conversation
where it is potentially subject to shoulder surfing. Litigate this
risk by masking the password part of identify messages.
Comment 4 Florian Müllner 2016-09-23 23:52:06 UTC
Created attachment 336178 [details] [review]
chatView: Be a bit silly on April 1st

Happy Easter everyone! Who is saying that passwords need to be
masked with boring dots all the time?
Comment 5 Bastian Ilsø 2016-10-09 11:34:11 UTC
Review of attachment 336176 [details] [review]:

looks good to me, one question:

::: src/lib/polari-util.c
@@ +60,3 @@
+  static GRegex *identify_message_regex = NULL;
+  GMatchInfo *match;
+  char *text, *stripped_text;

i see match and text is freed at the end of this function, where is stripped_text freed?
Comment 6 Bastian Ilsø 2016-10-09 11:38:21 UTC
Review of attachment 336177 [details] [review]:

looks good to me.
Comment 7 Bastian Ilsø 2016-10-09 11:42:59 UTC
Review of attachment 336178 [details] [review]:

i did this in a game once and got my account stolen :x
Comment 8 Florian Müllner 2016-10-09 12:32:10 UTC
(In reply to Bastian Ilsø from comment #5)
> i see match and text is freed at the end of this function, where is
> stripped_text freed?

It's not. Or rather: The function (macro actually) doesn't (re)allocate any memory, so text and stripped_text refer to the same memory that only needs to be freed once.
Comment 9 Bastian Ilsø 2016-10-09 13:48:28 UTC
Review of attachment 336176 [details] [review]:

(In reply to Florian Müllner from comment #8)
> (In reply to Bastian Ilsø from comment #5)
> > i see match and text is freed at the end of this function, where is
> > stripped_text freed?
> 
> It's not. Or rather: The function (macro actually) doesn't (re)allocate any
> memory, so text and stripped_text refer to the same memory that only needs
> to be freed once.

looks good to me then
Comment 10 Florian Müllner 2016-10-09 14:25:54 UTC
Attachment 336176 [details] pushed as 06dafa1 - util: Split out match_identify_message() method
Attachment 336177 [details] pushed as 3c7cb62 - chatView: Mask identify passwords in private chats
Comment 11 Mohammed Sadiq 2016-10-25 12:22:36 UTC
The messages from nickserv in notification still reveals the real password. Hope that this is already known and that's the reason why this bug isn't yet marked as closed.

Thanks.
Comment 12 Florian Müllner 2016-10-25 13:14:15 UTC
(In reply to Mohammed Sadiq from comment #11)
> Hope that this is already known and that's the reason why this bug isn't yet
> marked as closed.

It's known, but it's not the reason the bug hasn't been closed - the notification is not implemented by Polari at all. Those are provided by gnome-shell's built-in chat integration, which is generic for all telepathy one-to-one conversations. I'm afraid there's no intention of special-casing IRC/NickServ in gnome-shell.
Comment 13 Paweł 2016-12-06 22:33:50 UTC
(In reply to Florian Müllner from comment #12)
> (In reply to Mohammed Sadiq from comment #11)
> > Hope that this is already known and that's the reason why this bug isn't yet
> > marked as closed.
> 
> It's known, but it's not the reason the bug hasn't been closed - the
> notification is not implemented by Polari at all. Those are provided by
> gnome-shell's built-in chat integration, which is generic for all telepathy
> one-to-one conversations. I'm afraid there's no intention of special-casing
> IRC/NickServ in gnome-shell.

Well, that's bad then. Passwords should never be visible as plain text, inside a system notification, that just pops up. Is there nothing that can be made to hide or blacklist it?
Comment 14 Alexánder Alzate 2017-03-04 15:30:10 UTC
This patch is not working for me, my passwords are not masked and doing some research it is due to your regex pattern:

`"^identify (?:(\\w+) )?(\\S+)$"`

You are guessing the user name must match `\w+` but my user name is `alexander-alzate` which doesn't match. so I suggest using this pattern instead:

`"^identify (?:(\\S+) )?(\\S+)$"`

We know for sure that a user name should not contain spaces.
Comment 15 Alexánder Alzate 2017-03-04 15:31:55 UTC
Created attachment 347210 [details] [review]
Fix patch proposed for bug 771888
Comment 16 Florian Müllner 2017-03-04 22:10:33 UTC
Review of attachment 347210 [details] [review]:

Thanks for the patch!

Please provide a proper commit message, following https://chris.beams.io/posts/git-commit/. Also note that the patch will require rebasing after landing bug 778841.

::: src/lib/polari-util.c
@@ +68,2 @@
   if (G_UNLIKELY (identify_message_regex == NULL))
+    identify_message_regex = g_regex_new ("^identify (?:(\\S+) )?(\\S+)$",

Not sure it really matters, but the new regex matches strings like $%&"?, which isn't a valid nickname. Still, unlikely to cause any false positives in practice ...
Comment 17 Alexánder Alzate 2017-03-05 00:52:04 UTC
Created attachment 347247 [details] [review]
util: Match any nickname in identify command
Comment 18 Alexánder Alzate 2017-03-05 00:54:44 UTC
Comment on attachment 347247 [details] [review]
util: Match any nickname in identify command

Rebasing bug 778841 and commit message improved.
Comment 19 Alexánder Alzate 2017-03-05 00:55:15 UTC
Comment on attachment 347247 [details] [review]
util: Match any nickname in identify command

Rebasing bug 778841 commits and commit message improved.
Comment 20 Florian Müllner 2017-03-05 01:03:21 UTC
Review of attachment 347247 [details] [review]:

"A wide variety" is a bit of a stretch - the legal characters we are currently missing are []\`-^{|} - but OK. Keeping the regex readable certainly beats pointlessly following the spec to the letter.
Comment 21 Florian Müllner 2017-03-05 01:19:17 UTC
Comment on attachment 347247 [details] [review]
util: Match any nickname in identify command

Attachment 347247 [details] pushed as e1d28fa - util: Match any nickname in identify command
Comment 22 André Klapper 2021-06-10 11:05:56 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version of Polari, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/polari/-/issues/

Thank you for your understanding and your help.