View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001070 | Main CAcert Website | account administration | public | 2012-06-04 11:02 | 2014-09-02 20:53 |
| Reporter | wytze | Assigned To | NEOatNHNG | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Platform | Default | OS | any | OS Version | any |
| Product Version | 2012 Q2 | ||||
| Target Version | 2014 Q1 | Fixed in Version | 2014 Q1 | ||
| Summary | 0001070: Certain account passwords are logged in web server error log. | ||||
| Description | The checkpw() function in includes/general.php for rating a new user password contains the following code: $do = `grep '$pwd' /usr/share/dict/american-english`; When the user-chosen password (found in $pwd) happens to contain a single quote ('), this will result in the following error messages: sh: -c: line 0: syntax error near unexpected token `)' sh: -c: line 0: `grep 'XXXX\'XXXXX' /usr/share/dict/american-english' to be logged in the webserver's errorlog. I have blocked out the actual password with XXX in the above example, but in the real server log the actual password entered is logged in plain text. Although the webservers's errorlog can only be read by critical system administrators (trusted to deal with sensitive data), this is a potential security/privacy problem. Even critical sysadmins should not see this type of data on a semi-regular basis. Having this type of data in the errorlog creates an increased risk of exposing a user's password in an undesirable way. | ||||
| Steps To Reproduce | Use the CAcert application web interface to change your account password and enter a single quote somewhere in the new password. | ||||
| Additional Information | This error was discovered by a routine logfile inspection. The Arbitration regarding this case: https://wiki.cacert.org/Arbitrations/a20120614.1 | ||||
| Tags | No tags attached. | ||||
| Reviewed by | dastrath, NEOatNHNG, BenBE | ||||
| Test Instructions | |||||
|
|
This could lead to an attacker executing arbitrary shell commands as the web servers user. Inside the chroot but this is enough for many things. |
|
|
I have written a fix for it and applied it on the test server. Please test whether there are regressions (e.g. certain passwords with special characters not accepted anymore, password that matches a dictionary entry allowed etc.) and review the changes. |
|
|
The fix mentioned in Note #003057 has been applied as an emergency fix to the production server on June 14, 2012. This issue is deemed to require emergency fixing since leaving it unpatched would allow an attacker to execute arbitrary shell commands as the web server user inside the chroot, as noted in Note #003056. |
|
|
2nd review done by dirk with state OK within meeting https://wiki.cacert.org/Software/Assessment/20120807-S-A-MiniTOP still needs testing to add as working session on upcoming meeting https://wiki.cacert.org/Software/Assessment/20120814-S-A-MiniTOP |
|
|
1070 testing, starting 2012-08-14 22:20 change password 123456 Failure: Pass Phrase not Changed The Pass Phrase you submitted failed to contain enough differing characters and/or contained words from your name and/or email address. Only scored 1 points out of 6. A1! Failure: Pass Phrase not Changed The Pass Phrase you submitted was too short. London Failure: Pass Phrase not Changed The Pass Phrase you submitted failed to contain enough differing characters and/or contained words from your name and/or email address. Only scored 1 points out of 6. London123! Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. L o n d o n Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. !"§$%&/()=? Failure: Pass Phrase not Changed The Pass Phrase you submitted failed to contain enough differing characters and/or contained words from your name and/or email address. Only scored 1 points out of 6. "'London'" Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. <?php "Hello World"; ?> Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. try1: Failure: Pass Phrase not Changed You failed to correctly enter your current Pass Phrase. try2: success <script language="javascript" type="text/javascript"><!-- function init() { alert('Hello'); } // --></script> Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. https://cacert1.it-sls.de/index.php?id=5 try 1: missing domain in email address results in error msg, account not found => ok try 2: passes procedure email, dob success 3 of 5 questions, mixed order, success 23:05 no log enttries yet reseting testserver !"§$%&/()=? Failure: Pass Phrase not Changed The Pass Phrase you submitted failed to contain enough differing characters and/or contained words from your name and/or email address. Only scored 1 points out of 6. '!"§$%&/()=? Failure: Pass Phrase not Changed The Pass Phrase you submitted failed to contain enough differing characters and/or contained words from your name and/or email address. Only scored 1 points out of 6. "'London'" Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. <script language="javascript" type="text/javascript"><!-- function init() { alert('Hello'); } // --></script> Pass Phrase Changed Successfully Your Pass Phrase has been updated and your primary email account has been notified of the change. <?php echo 'Hello World'; ?> failures in pwd reset procedure '$(321+5432) failure '$(321A+5432b) passes pwd change <?php echo 'Hello World'; ?> passed pwd reset <?php echo 'Hello World'; ?> doesn't match <?php echo 'Hello World'; ?> fail admin console - find user set pwd <?php echo 'Hello World'; ?> error in log using pwd <?php echo 'Hello World'; ?> for login, passes |
|
|
tested in SA telco meeting https://wiki.cacert.org/Software/Assessment/20120814-S-A-MiniTOP good to go |
|
|
related to arb case https://wiki.cacert.org/Arbitrations/a20120614.1 |
|
|
Note as arbitrator of a20120614.1: I'm not sure that I see a second test for this patch. I can only find one done by Uli60 - and I cannot identify if the patch passed the test or not. I can only assume that it did because of later notes. I also would appreciate if the second review would be confirmed by the reviewer who did it. |
|
|
TL;DR: NACK due to various issues not addressed in the patch on the shell environment and the way this patch works around existing security mechanisms of the PHP interpreter. The patch for bug 1070[0] has been labelled as an emergency patch yet it fails in the time constraints set in the rules: From the point the first patch (according to the bug tracker) was written is about one week before critical confirms it being installed. Even further there is no documented test between the first announcement and the confirmation of critical that the patch has been installed. Only about 2 months later there is a test documented in the Wiki leaving a window of about 8 weeks where the code has potentially not been checked according to the 4 eye principle. The patch itself is minimal but fails to ensure to set the proper environment[1][2]. This is, the choice of using the escapeshellarg function in itself is correct and also its usage seems fine as is, but given we are expecting people to use every possible sequence of characters as passwords we also should take into account that things might break for non-ASCII characters at this point. For escapeshellarg[3] this means that the environment has to be set to the proper charset that supports all the characters that might pass by. There is a similarly called function escapeshellcmd[4] which does a somewhat more complicated escaping scheme than the one performed by escapeshellarg[3] and might depending on your shell fail to work properly and miss some characters that usually would need escaping. Neither escapeshellarg nor escapeshellcmd are guaranteed to work in all situations and are documented to be guaranteed to fail on e.g. Windows[5]. Given the quirks in both functions the most wise decision - even if this would incur a dramatic performance penalty - would have been to avoid the external process completely and do the search for the user-supplied password using a combination of fread[6] and stripos[7] while overlapping the last N byte from the previous iteration. Checking passwords fast isn't one of the things we need to optimize our performance on as long as we are doing it properly. The basic security standpoint in this case should be to avoid breaks of media and therefore various ways we might introduce XSS or work around security mechanisms of our execution environment[8]. So given this patch WAS handled as an emergency patch in a time-constrained manner the current solution is suitable as first aid and as first aid only. A proper version avoiding the above-mentioned pitfalls by avoiding the backtick operator[9] and external process execution[10] altogether is to be preferred. [0] https://bugs.cacert.org/view.php?id=1070 [1] http://php.net/escapeshellarg#88325 [2] http://php.net/escapeshellarg#99213 [3] http://php.net/escapeshellarg [4] http://php.net/escapeshellcmd [5] http://php.net/escapeshellarg#84789 [6] http://php.net/fread [7] http://php.net/stripos [8] http://php.net/ini.core#ini.open-basedir [9] http://php.net/language.operators.execution.php [10] http://php.net/ref.exec.php |
|
|
1) Tests after deployment: That's why it's called emergency patch and there's arbitration that has to evaluate the possible effects 2) Non-ASCII characters: a) such characters won't be contained in an American dictionary so a grep for such characters would not return any results anyway. b) escapeshellarg will in those cases remove those characters from the string so the worst thing that will happen is that all characters are stripped => grep misses an argument and fails. c) our web server only accepts latin1 encoding which means that all other characters will be entity-encoded anyway, if we wanted to support proper UTF8 we would have to rewrite a lot of code (or even throw it away completely) d) the locale is implicitly set in the L10N class. 3) escapeshellcmd(): Yep, that's why I used escapeshellarg. Windows is not one of our target plattforms. Most likely will not only the escapeshellarg also remove the %s but also will the shell fail to find grep. The worst that will happen however is that %s are strippped to which is not that big of a deal 4) For fread you have to do also do manual encoding if you want to support non-ASCII characters. I agree this would be a safer alternative (in the sense that some potential sources error could be avoided not that there are any known vulnerabilities in the present solution) but the effects should be tested. As you already stated such an implementation would have taken more time to develop so as an emergency procedure the produced patch was more appropriate at the time. Feel free to write a patch. |
|
|
An additional patch was applied to work around some issues with passwords which confused grep. With this patch passwords are now always interpreted by grep as literal strings and thus special characters should no longer cause trouble here. |
|
|
Retestd today with three testers: 1st tester tested pws: 11111111 aaaaaaaa AAAAAAAA <ü> were not set because of missing lower or upper case or numbers or special cases <ü>123ABC a1.a1. a1``´´'' '$(321A+5432b) ̊💩 <?php echo 'Hello World'; ?> $(ech huhu) ``123;(;)abc grep: Unmatched [ or [^ were accepted as PWs and worked for login Tester 2: !"§$%&/()=?` not allowed `hallo9& äpfel & börnen [] arised error grep: Unmatched [ or [^ {}̊💩 %&/$§hallo9 ^()=hallo0 ``123;(;)abc Tester 3: `echo hello welt` accept works <?php phpinfo(); ?> accept works #!/bin/sh not allowed #!/bin/sh6 accept works ~`hällö wält"-y accept works $(echo huhu) accept works grep: �[� oder �[^� ohne schlie�ende Klammer -äpfel & börnen [] |
|
|
After the change by Benny: Tester 1: grep: �[� oder �[^� ohne schlie�ende Klammer -abcdefg123 were accepted as PWs and worked for login Tester 2: äpfel & börnen [] Tester 3: äpfel & börnen [] => ok no more error given |
|
|
Have reviewed BenBE's follow up patch. Makes sense. |
|
|
cacert-devel: testserver-stable 7dc9f2b4 Timestamp: 2014-03-18 22:33:33 reviewed this patch, agrees to neo: Makes sense. |
|
|
The additional fix has been installed on the production server on April 1, 2014. See also: https://lists.cacert.org/wws/arc/cacert-systemlog/2014-04/msg00000.html |
|
|
note as arbitrator of a20120614.1: Please set the bug to public, now. |
|
|
Setting to public because Arbitration gave the OK. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2012-06-04 11:02 | wytze | New Issue | |
| 2012-06-06 20:03 | NEOatNHNG | Note Added: 0003056 | |
| 2012-06-06 20:03 | NEOatNHNG | View Status | public => private |
| 2012-06-06 20:10 | NEOatNHNG | Source_changeset_attached | => cacert-devel testserver de22378c |
| 2012-06-06 20:10 | NEOatNHNG | Source_changeset_attached | => cacert-devel testserver d5432de9 |
| 2012-06-06 22:16 | NEOatNHNG | Reviewed by | => NEOatNHNG |
| 2012-06-06 22:16 | NEOatNHNG | Note Added: 0003057 | |
| 2012-06-06 22:16 | NEOatNHNG | Assigned To | => NEOatNHNG |
| 2012-06-06 22:16 | NEOatNHNG | Status | new => needs review & testing |
| 2012-06-14 13:37 | wytze | Note Added: 0003067 | |
| 2012-08-07 23:14 | Uli60 | Note Added: 0003128 | |
| 2012-08-07 23:14 | Uli60 | Status | needs review & testing => needs testing |
| 2012-08-14 21:34 | Uli60 | Note Added: 0003136 | |
| 2012-08-14 23:33 | Uli60 | Note Added: 0003139 | |
| 2012-08-14 23:33 | Uli60 | Status | needs testing => ready to deploy |
| 2012-09-11 20:59 | Uli60 | Note Added: 0003184 | |
| 2012-09-11 20:59 | NEOatNHNG | Additional Information Updated | |
| 2012-09-11 20:59 | NEOatNHNG | Reviewed by | NEOatNHNG => dastrath, NEOatNHNG |
| 2012-12-11 22:55 | NEOatNHNG | Source_changeset_attached | => cacert-devel release 66c4e45e |
| 2014-01-28 21:13 | Eva | Note Added: 0004545 | |
| 2014-02-11 17:48 | Eva | Source_changeset_removed | cacert-devel testserver de22378c => |
| 2014-03-01 09:53 | BenBE | Note Added: 0004612 | |
| 2014-03-01 10:07 | BenBE | Note Edited: 0004612 | |
| 2014-03-01 17:34 | NEOatNHNG | Note Added: 0004613 | |
| 2014-03-18 22:35 | BenBE | Source_changeset_attached | => cacert-devel testserver-stable 3c9212e3 |
| 2014-03-18 22:35 | BenBE | Source_changeset_attached | => cacert-devel testserver-stable 7dc9f2b4 |
| 2014-03-18 22:43 | BenBE | Reviewed by | dastrath, NEOatNHNG => dastrath, NEOatNHNG, BenBE |
| 2014-03-18 22:43 | BenBE | Note Added: 0004653 | |
| 2014-03-18 22:43 | INOPIAE | Note Added: 0004654 | |
| 2014-03-18 22:44 | BenBE | Reviewed by | dastrath, NEOatNHNG, BenBE => BenBE |
| 2014-03-18 22:45 | INOPIAE | Note Added: 0004655 | |
| 2014-03-18 22:45 | NEOatNHNG | Reviewed by | BenBE => NEOatNHNG, BenBE |
| 2014-03-18 22:46 | NEOatNHNG | Note Added: 0004656 | |
| 2014-03-18 22:47 | INOPIAE | Note Edited: 0004654 | |
| 2014-03-18 22:48 | INOPIAE | Note Edited: 0004654 | |
| 2014-03-25 23:15 | egal | Note Added: 0004684 | |
| 2014-03-25 23:19 | BenBE | Reviewed by | NEOatNHNG, BenBE => dastrath, NEOatNHNG, BenBE |
| 2014-03-25 23:19 | BenBE | Product Version | => 2012 Q2 |
| 2014-03-25 23:19 | BenBE | Fixed in Version | => 2012 Q2 |
| 2014-03-25 23:19 | BenBE | Target Version | => 2014 Q1 |
| 2014-04-01 14:36 | wytze | Note Added: 0004688 | |
| 2014-04-01 14:36 | wytze | Status | ready to deploy => solved? |
| 2014-04-01 14:36 | wytze | Fixed in Version | 2012 Q2 => 2014 Q1 |
| 2014-04-01 14:36 | wytze | Resolution | open => fixed |
| 2014-04-01 20:56 | Eva | Note Added: 0004689 | |
| 2014-04-01 21:07 | NEOatNHNG | Note Added: 0004690 | |
| 2014-04-01 21:07 | NEOatNHNG | View Status | private => public |
| 2014-04-09 06:50 | BenBE | Source_changeset_attached | => cacert-devel release 7edae081 |
| 2014-09-02 20:53 | INOPIAE | Status | solved? => closed |
