Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password logging #73

Closed
keachi opened this issue Sep 15, 2016 · 6 comments
Closed

Password logging #73

keachi opened this issue Sep 15, 2016 · 6 comments

Comments

@keachi
Copy link

keachi commented Sep 15, 2016

If matterircd is started in debug mode, all login passwords get plaintext logged. IMHO is this a really bad idea.
I think such private messages can be easily filtered, and displayed like the following:

Sep 15 08:23:59 hostname matterircd-linux64[9050]: time="2016-09-15T08:23:59+02:00" level=debug msg="<- PRIVMSG mattermost :login user1 <filtered>" module=matterircd

Without such a filter, you should never run the matterircd on a productive system with debug enabled.

@42wim
Copy link
Owner

42wim commented Sep 15, 2016

Good point, I'll add this.
But running matterircd in production with debug isn't advised because of the overhead that debugs gives on busy systems.

@42wim
Copy link
Owner

42wim commented Sep 25, 2016

If anyone has time to do this, PR's are welcome.

@slowbro
Copy link
Contributor

slowbro commented Nov 2, 2017

I'd be happy to do this, but i'm having trouble figuring out the 'best' way - here are my ideas so far.

  1. Parse for a message that looks like 'PRIVMSG :login' and truncate it after 'login' (hiding username & password)
    a. this seems kind of hacky and coudl match other things?
  2. Move logging out of user.Decode() and make it more granular?
    a. but then you have a lot of repeated code, and there's also the possibility that something gets missed in the future (i.e. forgot to add the debug line)
  3. Abandon this change - as @42wim said, you really should not be running this with -debug in production, and debug logs always are inherently 'insecure' as you are dumping as much information as possible.

Input would be appreciated.

@42wim
Copy link
Owner

42wim commented Nov 2, 2017

@slowbro thanks for taking an interest :)
If you want to spend time on this it's very much appreciated!

I would take option 1 for this and do the check user.Decode()

Check if privmsg to mattermost or slack is sent, use the function on https://github.com/42wim/mm-go-irckit/blob/master/mmservice.go#L291 to parse the line, so you can also match long passwords.
Also take a look at https://github.com/42wim/mm-go-irckit/blob/master/mmservice.go#L93-L111 because the login command can have different amount of parameters depending on the options passed.

Maybe refactor those L93-L111 in a function that can be reused for your check.

@slowbro
Copy link
Contributor

slowbro commented Nov 8, 2017

Understood! I will start work on this.

@slowbro
Copy link
Contributor

slowbro commented Nov 8, 2017

Created a PR. I ended up not using parseCommandString since the fixes for non-rfc (i.e. missing :) come later.. this seemed like the most straightforward way to accomplish it.

Let me know if you want anything changed!

@42wim 42wim closed this as completed in d997fa5 Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants