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

Manage passwords with special characters #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pgaufillet
Copy link

Hi!

I have encountered issues with fetchmail_rc when pop/imap passwords include special characters. The analysis has revealed 2 causes:

  • The shell command is not correctly protected by quotes. The single quotes are eatten by the shell and the strings are not protected anymore when parsed by fetchmail. Adding surrounding double quotes ('"' . escapeshellarg($this->get_mail_password()) . '"') solves it. Maybe it should be applied to all string parameters of fetchmail (and probably at least _mail_user_name).
  • By default, get_input_value filters the input string and removes HTML tags. Unfortunately, it may remove parts of the password, for example when you have <? characters. A solution is to disable the HTML filter by adding true as third parameter: get_input_value('_mail_password', RCUBE_INPUT_POST, true). I don't think it raises any security issue as the password is never displayed or interpreted.

This pull request also replaces $rcmail_config by $config but I am not sure it is required or not, depending on the version of roundcube and the deployment platform. This commit is not linked to the password issue.

Comments and questions welcome.

Pierre

@gilles-hemmerle
Copy link
Contributor

Hi,
Thank you for your comments.
I will have a look at this by today when I got some time and merge / change the parameters in order to improve security of shell arguments.

Gilles.

Le 19 nov. 2015 à 11:11, pgaufillet notifications@github.com a écrit :

Hi!

I have encountered issues with fetchmail_rc when pop/imap passwords include special characters. The analysis has revealed 2 causes:

The shell command is not correctly protected by quotes. The single quotes are eatten by the shell and the strings are not protected anymore when parsed by fetchmail. Adding surrounding double quotes ('"' . escapeshellarg($this->get_mail_password()) . '"') solves it. Maybe it should be applied to all string parameters of fetchmail (and probably at least _mail_user_name).
By default, get_input_value filters the input string and removes HTML tags. Unfortunately, it may remove parts of the password, for example when you have <? characters. A solution is to disable the HTML filter by adding true as third parameter: get_input_value('_mail_password', RCUBE_INPUT_POST, true). I don't think it raises any security issue as the password is never displayed or interpreted.
This pull request also replaces $rcmail_config by $config but I am not sure it is required or not, depending on the version of roundcube and the deployment platform. This commit is not linked to the password issue.

Comments and questions welcome.

Pierre

You can view, comment on, or merge this pull request online at:

#7 #7
Commit Summary

Change $rcmail_config into $config.
Correctly escape passwords taking into account shell interpretation.
Password shall not be filtered of potential HTML tags.
File Changes

M bash_cron.php https://github.com/iabsis/roundcube-fetchmail-rc/pull/7/files#diff-0 (2)
M config.inc.php https://github.com/iabsis/roundcube-fetchmail-rc/pull/7/files#diff-1 (4)
M fetchmail_rc.php https://github.com/iabsis/roundcube-fetchmail-rc/pull/7/files#diff-2 (6)
M includes/fetchMailRc.php https://github.com/iabsis/roundcube-fetchmail-rc/pull/7/files#diff-3 (2)
Patch Links:

https://github.com/iabsis/roundcube-fetchmail-rc/pull/7.patch https://github.com/iabsis/roundcube-fetchmail-rc/pull/7.patch
https://github.com/iabsis/roundcube-fetchmail-rc/pull/7.diff https://github.com/iabsis/roundcube-fetchmail-rc/pull/7.diff

Reply to this email directly or view it on GitHub #7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants