View Issue Details

IDProjectCategoryView StatusLast Update
0001070Main CAcert Websiteaccount administrationpublic2014-09-02 20:53
Reporterwytze Assigned ToNEOatNHNG  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
PlatformDefaultOSany 
Product Version2012 Q2 
Target Version2014 Q1Fixed in Version2014 Q1 
Summary0001070: Certain account passwords are logged in web server error log.
DescriptionThe 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 ReproduceUse the CAcert application web interface to change your account password and enter a single quote somewhere in the new password.
Additional InformationThis error was discovered by a routine logfile inspection.

The Arbitration regarding this case: https://wiki.cacert.org/Arbitrations/a20120614.1
TagsNo tags attached.
Reviewed bydastrath, NEOatNHNG, BenBE
Test Instructions

Activities

NEOatNHNG

2012-06-06 20:03

administrator   ~0003056

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.

NEOatNHNG

2012-06-06 22:16

administrator   ~0003057

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.

wytze

2012-06-14 13:37

developer   ~0003067

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.

Uli60

2012-08-07 23:14

updater   ~0003128

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

Uli60

2012-08-14 21:34

updater   ~0003136

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

Uli60

2012-08-14 23:33

updater   ~0003139

tested in SA telco meeting
https://wiki.cacert.org/Software/Assessment/20120814-S-A-MiniTOP
good to go

Uli60

2012-09-11 20:59

updater   ~0003184

related to arb case
https://wiki.cacert.org/Arbitrations/a20120614.1

Eva

2014-01-28 21:13

updater   ~0004545

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.

BenBE

2014-03-01 09:53

updater   ~0004612

Last edited: 2014-03-01 10:07

View 2 revisions

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

NEOatNHNG

2014-03-01 17:34

administrator   ~0004613

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.

BenBE

2014-03-18 22:43

updater   ~0004653

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.

INOPIAE

2014-03-18 22:43

updater   ~0004654

Last edited: 2014-03-18 22:48

View 3 revisions

Retestd today with three testers:
1st tester
tested pws:
11111111
aaaaaaaa
AAAAAAAA
<&uuml;> were not set because of missing lower or upper case or numbers or special cases

<&uuml;>123ABC
a1.a1.
a1``´´''
'$(321A+5432b)
&#x30a;&#x1f4a9;
<?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 [^
{}&#x30a;&#x1f4a9;
%&/$§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 []

INOPIAE

2014-03-18 22:45

updater   ~0004655

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

NEOatNHNG

2014-03-18 22:46

administrator   ~0004656

Have reviewed BenBE's follow up patch. Makes sense.

egal

2014-03-25 23:15

administrator   ~0004684

cacert-devel: testserver-stable 7dc9f2b4
Timestamp: 2014-03-18 22:33:33

reviewed this patch, agrees to neo: Makes sense.

wytze

2014-04-01 14:36

developer   ~0004688

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

Eva

2014-04-01 20:56

updater   ~0004689

note as arbitrator of a20120614.1:

Please set the bug to public, now.

NEOatNHNG

2014-04-01 21:07

administrator   ~0004690

Setting to public because Arbitration gave the OK.

Issue History

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 View Revisions
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 View Revisions
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 View Revisions
2014-03-18 22:48 INOPIAE Note Edited: 0004654 View Revisions
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