View Issue Details

IDProjectCategoryView StatusLast Update
0001273Main CAcert Websitesource codepublic2014-12-05 00:45
ReporterNEOatNHNG Assigned ToNEOatNHNG  
PriorityhighSeverityminorReproducibilityN/A
Status closedResolutionfixed 
PlatformDefaultOSany 
Product Version2014 Q2 
Target Version2014 Q2Fixed in Version2014 Q4 
Summary0001273: Replace all backtick operators with calls to runCommand() or shell_exec()
DescriptionBacktick operators are very hard to search for and should therefore be avoided (especially because the backtick character also serves as identifier escaping in MySQL).

Replace all backtick operators with calls to the appropriate function and fix any missing shell argument escaping along the way.
TagsNo tags attached.
Reviewed byNEOatNHNG, BenBE
Test InstructionsVerify that all processes mentioned in #c4854 are still working. For test instruction see #c5022

Activities

felixd

2014-06-15 09:04

updater   ~0004832

A fix is available here: https://github.com/yellowant/cacert-devel/commits/bug-1273

I locacted all places in a semi-automated manner (see commit message)

felixd

2014-06-15 09:52

updater   ~0004834

Last edited: 2014-06-15 09:53

View 2 revisions

I updated the patch so that former 'echo "$user_input"|command' will now be executed with "runCommand"

felixd

2014-06-17 22:04

updater   ~0004854

The following processes are affected by this change:

build a client key with browser gen (includes/account.php:405)
build a client key with csr (includes/account.php:469)
check if the whois-email is possible to choose for domain check (includes/account.php:573)
generate a server cert (includes/account.php:744,)
renew it ( includes/account.php:915, includes/account.php:945)

build a client org key with browser gen (includes/account.php:1562)
build a client org key with csr (includes/account.php:1616)
create org server certificate (includes/account.php:1870)
and renew it

change password to "american-english" word (includes/general.php:222)
add an additional email address(includes/general.php:543)

sign gpg key (www/gpg.php)

NEOatNHNG

2014-06-22 00:47

administrator   ~0004867

Second review OK.

felixd

2014-07-01 23:59

updater   ~0004878

build a client key with browser gen => OK
build a client key with csr => OK

check if the whois-email is possible to choose for domain check
( fail... but error is not directly related to this bug
  pushed a fix: https://github.com/yellowant/cacert-devel/commit/d6e99bdb72f1d616a1a7e08c333f7c2079edaaf9)
generate a server cert => OK
renew it => OK

build a client org key with browser gen =>OK
build a client org key with csr/IE generation => (couldnt test)
create org server certificate => OK
renew it => OK

change password to "american-english" (tested "ashtray" showed "0", without dict would be "1") => OK

add an additional email address => OK
sign gpg key => OK

==> nearly OK (couldn't test "whois")

Eva

2014-07-08 20:55

updater   ~0004880

I do not get how to test this. I can do all this stuff, (and mostly have done this a lot, recently) but I will not be able to tell the difference.

Please tell me how to say if a test is ok or failed and what to look out for. It is hard to tell from the outside, what calls were done on the inside.

BenBE

2014-08-07 20:54

updater   ~0004921

The non-existence of any difference is exactly what makes the difference to make this test successful.

Eva

2014-08-08 06:07

updater   ~0004922

At least the escaping could make a difference, if some was missing, before.

It is hard to test for this anywhere.

Also this test probably should require to monitor the logfiles.

Eva

2014-09-23 19:53

updater   ~0005021

Last edited: 2014-09-23 19:55

View 2 revisions

Can the Test-Instructions please be verified by a Software Assessor?

It is quite special that ONLY american stuff should be affected, for exaple.

INOPIAE

2014-09-23 20:01

updater   ~0005022

Last edited: 2014-09-23 20:36

View 3 revisions

Test description:
Account stuff:
- try to add a password which is listed in an American-English dictionary
- add a new email address
- add a new domain
(check for mail from whois is not implemented on test server)

Cert Stuff:
- create new client cert with browser
- create new client cert with csr

- create new server cert
- renew server cert

- create org client cert
- create org server cert
- renew org server cert

- sign gpg key

Result should be that all tests are successful

INOPIAE

2014-09-23 20:36

updater   ~0005023

Account stuff:
- try to add a password which is listed in an American-English dictionary. I was able to add the following password "Elevator 1" => fail
- add a new email address => ok
- add a new domain =>ok

Cert Stuff:
- create new client cert with browser => ok
- create new client cert with csr => ok

- create new server cert => ok
- renew server cert => ok

- create org client cert => ok
- create org server cert => ok
- renew org server cert => ok

- sign gpg key => ok

Password failed, rest ok

reinhardm

2014-10-21 18:52

updater   ~0005063

Last edited: 2014-10-21 19:45

View 3 revisions

I fetched this evening at 20:20 h from git
git clone git://git-cacert.it-sls.de/cacert.git production
git clone git://git-cacert.it-sls.de/cacert-devel.git test-server

and run these commands
find production -type f -name \*php -print -exec egrep -n -e '\$\w+\s*=\s*`' {} \; >> /tmp/production.txt
find test-server -type f -name \*php -print -exec egrep -n -e '\$\w+\s*=\s*`' {} \; >> /tmp/test-server.txt

result for test-server is
test-server/scripts/gpgfillmissingkeyid.php
42: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
test-server/scripts/gpgcheck3.php
113: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
test-server/scripts/gpgfillmissingemail.php
57: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
test-server/scripts/cron/warning.php
42: $subject = `openssl x509 -in $crt_name -text -noout|grep Subject:`;
test-server/www/gpg.php
528: $do=`gpg --homedir $cwd --batch --export-options export-minimal --export $cmd_keyid >$csrname`;
test-server/www/api/ccsr.php
78: $do = `/usr/bin/openssl req -in $incsr_esc -out $checkedcsr_esc`;
100: $do = `../../scripts/runclient`;
test-server/includes/account.php
405: $res=`openssl spkac -verify -in $CSRname_esc`;
469: $do = `/usr/bin/openssl req -in $tmpfname_esc -out $tmpname_esc`; // -subj "$csr"`;
945: $cert = `/usr/bin/openssl x509 -in $crt_name`;
1562: $res=`openssl spkac -verify -in $CSRname_esc`;
1616: $do = `/usr/bin/openssl req -in $tmpfname_esc -out $tmpname_esc`;
2094: $cert = `/usr/bin/openssl x509 -in $crtname`;
test-server/includes/general.php
222: $do = `grep -F -- $shellpwd /usr/share/dict/american-english`;
530: $do = `/usr/bin/gpg --homedir /home/gpg --clearsign "$tmpfname"|/usr/sbin/sendmail "$to"`;
test-server/pages/account/15.php
33: $cert = `/usr/bin/openssl x509 -in $crtname`;
test-server/pages/account/19.php
34: $cert = `/usr/bin/openssl x509 -in $crtname`;
test-server/pages/account/6.php
63: $cert = `/usr/bin/openssl x509 -in $crtname $outform`;
85: $cert = `/usr/bin/openssl x509 -in $crtname -outform DER`;
114: $cert = `/usr/bin/openssl x509 -in $crtname -outform PEM`;
test-server/pages/account/23.php
33: $cert = `/usr/bin/openssl x509 -in $crtname`;

result for production is
production/scripts/gpgfillmissingkeyid.php
42: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
production/scripts/gpgcheck3.php
113: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
production/scripts/gpgfillmissingemail.php
57: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
production/scripts/cron/warning.php
42: $subject = `openssl x509 -in $crt_name -text -noout|grep Subject:`;
production/www/gpg.php
522: $do=`gpg --homedir $cwd --batch --export-options export-minimal --export $cmd_keyid >$csrname`;
production/www/api/ccsr.php
78: $do = `/usr/bin/openssl req -in $incsr_esc -out $checkedcsr_esc`;
100: $do = `../../scripts/runclient`;
production/includes/account.php
399: $res=`openssl spkac -verify -in $CSRname_esc`;
463: $do = `/usr/bin/openssl req -in $tmpfname_esc -out $tmpname_esc`; // -subj "$csr"`;
938: $cert = `/usr/bin/openssl x509 -in $crt_name`;
1561: $res=`openssl spkac -verify -in $CSRname_esc`;
1615: $do = `/usr/bin/openssl req -in $tmpfname_esc -out $tmpname_esc`;
2092: $cert = `/usr/bin/openssl x509 -in $crtname`;
production/includes/general.php
222: $do = `grep -F -- $shellpwd /usr/share/dict/american-english`;
530: $do = `/usr/bin/gpg --homedir /home/gpg --clearsign "$tmpfname"|/usr/sbin/sendmail "$to"`;
production/pages/account/15.php
33: $cert = `/usr/bin/openssl x509 -in $crtname`;
production/pages/account/19.php
34: $cert = `/usr/bin/openssl x509 -in $crtname`;
production/pages/account/6.php
63: $cert = `/usr/bin/openssl x509 -in $crtname $outform`;
85: $cert = `/usr/bin/openssl x509 -in $crtname -outform DER`;
114: $cert = `/usr/bin/openssl x509 -in $crtname -outform PEM`;
production/pages/account/23.php
33: $cert = `/usr/bin/openssl x509 -in $crtname`;

please check and correct



after correcting and fetching the patched correct sources
git checkout origin/testserver-stable
the result is
nochmal - das bleibt jetzt ├╝brig
bug-1273/scripts/gpgfillmissingkeyid.php
42: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
bug-1273/scripts/gpgcheck3.php
113: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
bug-1273/scripts/gpgfillmissingemail.php
57: $gpg = `gpg --with-colons --homedir /tmp $crt 2>/dev/null`;
bug-1273/scripts/cron/warning.php
44: $subject = `openssl x509 -in $crt_name -text -noout|grep Subject:`;

felixd

2014-10-21 20:40

updater   ~0005067

I updated the patches on github to include the patch for "scripts/cron/warning.php"

felixd

2014-10-28 22:16

updater   ~0005078

1. for (0005023) "Elevator 1" => fail. Was clarified: "Elevator" was actually missing in the dictionary, while "elevator" was contained and the code always did not ignore the case.
"elevator" was rejected as password.

2. magu and INOPIAE successfully tested the additional patch for the warning. We triggered such a warning and both received the correct email. (Additionally it was checked, that the special case, when information was missing in the database, was also correctly handled)

MartinGummi

2014-10-28 22:30

updater   ~0005080

Last edited: 2014-10-28 22:31

View 2 revisions

https://bugs.cacert.org/view.php?id=1273#c5078

this is correct

janmaco

2014-10-28 23:40

updater   ~0005081

Account stuff:
- try to add a password which is listed in an American-English dictionary: I tried to change the pw to 'membership' and it wasn't accepted, but the error displayed does not indicate clearly, that the rejection is actually caused by a matching word in the dictionary. => OK
- add a new email address: Can add a new email address => OK
- add a new domain: Worked as expected. => OK
(check for mail from whois is not implemented on test server)

Cert Stuff:
- create new client cert with browser: Correctly generated and listed. => OK
- create new client cert with csr: Correctly created and listed. => OK

- create new server cert: Correctly created and listed. => OK
- renew server cert: Correctly renewed. => OK

- create org client cert:
*tested with browser => OK
*tested with csr => OK
- create org server cert: Correctly created and listed. => OK
- renew org server cert: Correctly renewed. => OK

- sign gpg key: Correctly created and listed. => OK

BenBE

2014-11-23 15:06

updater   ~0005121

Sent to crit for deployment.

wytze

2014-11-24 10:18

developer   ~0005124

The fix has been installed on the production server on November 24, 2014. See also:
https://lists.cacert.org/wws/arc/cacert-systemlog/2014-11/msg00016.html

Issue History

Date Modified Username Field Change
2014-04-19 01:39 NEOatNHNG New Issue
2014-04-19 01:39 NEOatNHNG Assigned To => NEOatNHNG
2014-04-26 11:52 BenBE Status new => needs work
2014-04-26 11:52 BenBE Category => source code
2014-06-15 09:04 felixd Note Added: 0004832
2014-06-15 09:52 felixd Note Added: 0004834
2014-06-15 09:53 felixd Note Edited: 0004834 View Revisions
2014-06-15 16:25 BenBE Source_changeset_attached => cacert-devel testserver-stable 884770fd
2014-06-15 16:25 felixd Source_changeset_attached => cacert-devel testserver-stable e288e77f
2014-06-15 16:25 felixd Source_changeset_attached => cacert-devel testserver-stable fdd9deca
2014-06-15 16:25 felixd Source_changeset_attached => cacert-devel testserver-stable b6ee5404
2014-06-15 16:32 BenBE Reviewed by => BenBE
2014-06-15 16:32 BenBE Status needs work => needs review & testing
2014-06-17 22:04 felixd Note Added: 0004854
2014-06-22 00:47 NEOatNHNG Reviewed by BenBE => NEOatNHNG, BenBE
2014-06-22 00:47 NEOatNHNG Note Added: 0004867
2014-06-22 00:47 NEOatNHNG Status needs review & testing => needs testing
2014-07-01 23:59 felixd Note Added: 0004878
2014-07-08 20:55 Eva Note Added: 0004880
2014-08-07 20:54 BenBE Note Added: 0004921
2014-08-08 06:07 Eva Note Added: 0004922
2014-09-23 19:43 felixd Test Instructions => Verify that all processes mentioned in #c4854 are still working.
2014-09-23 19:53 Eva Note Added: 0005021
2014-09-23 19:55 Eva Note Edited: 0005021 View Revisions
2014-09-23 20:01 INOPIAE Note Added: 0005022
2014-09-23 20:02 INOPIAE Test Instructions Verify that all processes mentioned in #c4854 are still working. => Verify that all processes mentioned in #c4854 are still working. for instruction see #c5022
2014-09-23 20:03 INOPIAE Test Instructions Verify that all processes mentioned in #c4854 are still working. for instruction see #c5022 => Verify that all processes mentioned in #c4854 are still working. For test instruction see #c5022
2014-09-23 20:16 INOPIAE Note Edited: 0005022 View Revisions
2014-09-23 20:36 INOPIAE Note Added: 0005023
2014-09-23 20:36 INOPIAE Note Edited: 0005022 View Revisions
2014-10-21 18:52 reinhardm Note Added: 0005063
2014-10-21 18:56 reinhardm Note Edited: 0005063 View Revisions
2014-10-21 19:45 reinhardm Note Edited: 0005063 View Revisions
2014-10-21 20:40 felixd Note Added: 0005067
2014-10-21 20:50 BenBE Source_changeset_attached => cacert-devel testserver-stable 350a2f0d
2014-10-21 20:50 felixd Source_changeset_attached => cacert-devel testserver-stable c2cc1c5e
2014-10-28 22:16 felixd Note Added: 0005078
2014-10-28 22:30 MartinGummi Note Added: 0005080
2014-10-28 22:31 MartinGummi Note Edited: 0005080 View Revisions
2014-10-28 23:40 janmaco Note Added: 0005081
2014-10-28 23:42 felixd Status needs testing => ready to deploy
2014-11-23 15:06 BenBE Note Added: 0005121
2014-11-23 16:00 BenBE Source_changeset_attached => cacert-devel release 0bae20ba
2014-11-24 10:18 wytze Note Added: 0005124
2014-11-24 10:18 wytze Status ready to deploy => solved?
2014-11-24 10:18 wytze Fixed in Version => 2014 Q4
2014-11-24 10:18 wytze Resolution open => fixed
2014-12-05 00:45 NEOatNHNG Status solved? => closed