Passwords can accidentially end up in logs from badly made plugins

Bug #1567186 reported by Robert Lyon on 2016-04-07
256
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mahara
High
Robert Lyon
1.10
High
Unassigned
15.04
High
Unassigned
15.10
High
Unassigned
16.04
High
Robert Lyon
16.10
High
Unassigned

Bug Description

We have some code that suppresses the passwords in logs for LiveUser and for AuthLdap

But we need to extend it out to be more encompassing

Robert Lyon (robertl-9) on 2016-04-07
information type: Public → Private Security
Aaron Wells (u-aaronw) wrote :

Here's a quick test:

1. Edit htdocs/auth/user.php so that the function "login($username, $password)" on line 1438, has an ldap_bind() as its first command, like this:

    /**
     * Take a username and password and try to authenticate the
     * user
     *
     * @param string $username
     * @param string $password
     * @return bool
     */
    public function login($username, $password) {

        ldap_bind(null, null, 'password123'); // debugging

        $sql = 'SELECT

2. Log in to Mahara

3. Look at the Apache logs. The passwords for both LiveUser::login() and ldap_bind() should be starred out, like this:

[WAR] 65 (auth/user.php:1438) ldap_bind() expects parameter 1 to be resource, null given
 Call stack (most recent first):
   * log_message("ldap_bind() expects parameter 1 to be resource, nu...", 8, true, true, "/home/aaronw/www/mahara/htdocs/auth/user.php", 1438) at /home/aaronw/www/mahara/htdocs/lib/errors.php:490
  * error(2, "ldap_bind() expects parameter 1 to be resource, nu...", "/home/aaronw/www/mahara/htdocs/auth/user.php", 1438, array(size 2)) at Unknown:0
   * ldap_bind(null, null, "********") at /home/aaronw/www/mahara/htdocs/auth/user.php:1438
   * LiveUser->login("aaronw", "********") at /home/aaronw/www/mahara/htdocs/auth/lib.php:1460
   * login_submit(object(Pieform), array(size 6)) at Unknown:0
   * call_user_func_array("login_submit", array(size 2)) at /home/aaronw/www/mahara/htdocs/lib/pieforms/pieform.php:540
   * Pieform->__construct(array(size 9)) at /home/aaronw/www/mahara/htdocs/lib/mahara.php:4483
   * pieform_instance(array(size 9)) at /home/aaronw/www/mahara/htdocs/auth/lib.php:504
   * auth_setup() at /home/aaronw/www/mahara/htdocs/init.php:367
   * require("/home/aaronw/www/mahara/htdocs/init.php") at /home/aaronw/www/mahara/htdocs/index.php:16

Aaron Wells (u-aaronw) wrote :

Hm... actually even this can't give us complete protection. Passwords and other secure data are sometimes passed through other types of functions as well, for instance the set_field() to store one to the database, or string-handling functions. In those cases, neither the parameter name or position will give us any clue that it's sensitive data.

Maybe we should just turn off the printing of parameter values to the logs? Or at least only when productionmode is off? (And even when productionmode is on, still scrub out the ones that are obviously passwords.)

Aaron Wells (u-aaronw) wrote :

Spun off the "no param values" idea into Bug 1570221.

Aaron Wells (u-aaronw) on 2016-04-21
information type: Private Security → Public Security

Reviewed: https://reviews.mahara.org/6335
Committed: https://git.mahara.org/mahara/mahara/commit/15d479f51c3562faa4b08118da6f592bea7a6774
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 15d479f51c3562faa4b08118da6f592bea7a6774
Author: Aaron Wells <email address hidden>
Date: Thu Apr 14 19:07:46 2016 +1200

Bug 1567186: More thorough checking for passwords in stacktraces

Rather than having an increasing list of specific parameters
that we know to have passwords, this patch censors the content
of any parameter with a name that contains the string "password"
or "pw".

behatnotneeded: Can't test with Behat

Change-Id: Ifaa2ec10cf749c173b1a8d0928c6cc052124a83f

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6379
Committed: https://git.mahara.org/mahara/mahara/commit/ae4523770e5047d9c9fd17c38f5cdddf86ece437
Submitter: Robert Lyon (<email address hidden>)
Branch: 16.04_STABLE

commit ae4523770e5047d9c9fd17c38f5cdddf86ece437
Author: Aaron Wells <email address hidden>
Date: Thu Apr 14 19:07:46 2016 +1200

Bug 1567186: More thorough checking for passwords in stacktraces

Rather than having an increasing list of specific parameters
that we know to have passwords, this patch censors the content
of any parameter with a name that contains the string "password"
or "pw".

behatnotneeded: Can't test with Behat

Change-Id: Ifaa2ec10cf749c173b1a8d0928c6cc052124a83f

Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6387

Mahara Bot (dev-mahara) wrote :

Patch for "1.10_STABLE" branch: https://reviews.mahara.org/6388

Reviewed: https://reviews.mahara.org/6386
Committed: https://git.mahara.org/mahara/mahara/commit/0d45baa3467c9da6debdcebdb17634525017b931
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit 0d45baa3467c9da6debdcebdb17634525017b931
Author: Aaron Wells <email address hidden>
Date: Thu Apr 14 19:07:46 2016 +1200

Bug 1567186: More thorough checking for passwords in stacktraces

Rather than having an increasing list of specific parameters
that we know to have passwords, this patch censors the content
of any parameter with a name that contains the string "password"
or "pw".

behatnotneeded: Can't test with Behat

Change-Id: Ifaa2ec10cf749c173b1a8d0928c6cc052124a83f
(cherry picked from commit ae4523770e5047d9c9fd17c38f5cdddf86ece437)

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6388
Committed: https://git.mahara.org/mahara/mahara/commit/b77bd3479d81b85fbb7e025ff01b6ace2dd5ab17
Submitter: Aaron Wells (<email address hidden>)
Branch: 1.10_STABLE

commit b77bd3479d81b85fbb7e025ff01b6ace2dd5ab17
Author: Aaron Wells <email address hidden>
Date: Thu Apr 14 19:07:46 2016 +1200

Bug 1567186: More thorough checking for passwords in stacktraces

Rather than having an increasing list of specific parameters
that we know to have passwords, this patch censors the content
of any parameter with a name that contains the string "password"
or "pw".

behatnotneeded: Can't test with Behat

Change-Id: Ifaa2ec10cf749c173b1a8d0928c6cc052124a83f
(cherry picked from commit ae4523770e5047d9c9fd17c38f5cdddf86ece437)

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6387
Committed: https://git.mahara.org/mahara/mahara/commit/9d63e7e77a2beb0f780e16f54d1c8d952eba4433
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.04_STABLE

commit 9d63e7e77a2beb0f780e16f54d1c8d952eba4433
Author: Aaron Wells <email address hidden>
Date: Thu Apr 14 19:07:46 2016 +1200

Bug 1567186: More thorough checking for passwords in stacktraces

Rather than having an increasing list of specific parameters
that we know to have passwords, this patch censors the content
of any parameter with a name that contains the string "password"
or "pw".

behatnotneeded: Can't test with Behat

Change-Id: Ifaa2ec10cf749c173b1a8d0928c6cc052124a83f
(cherry picked from commit ae4523770e5047d9c9fd17c38f5cdddf86ece437)

Robert Lyon (robertl-9) on 2016-10-21
Changed in mahara:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers