search

christianwach / civicrm-admin-utilities

CiviCRM Admin Utilities is a WordPress plugin that modifies and enhances CiviCRM's appearance and behaviour in single site and multisite WordPress installs.
https://wordpress.org/plugins/civicrm-admin-utilities/
GNU General Public License v2.0
20 stars 10 forks source link

Security bug fixed #17

Closed elgodolfredo closed 4 years ago

elgodolfredo commented 4 years ago

Hello. I was looking for repositories that have security bugs for an assignment that I have to do on my college. This one in particular uses "extract()" and it's not recommended. In this page its explained in detail https://www.php.net/manual/en/function.extract.php

Can u please give a look to my commit and if its possible merge? I'll appreciate it a lot! :)

Thanks!

christianwach commented 4 years ago

@elgodolfredo Thanks for the PR, I appreciate your enthusiasm. A few comments from me might help you give some feedback to your college about the task:

I'm going to write this in BOLD ALL CAPS because it's so important... NEVER EVER OPEN PUBLIC PRS FOR WHAT YOU THINK MAY BE SECURITY ISSUES. NOT EVER. ALWAYS CONTACT DEVELOPERS PRIVATELY. This is (obviously) because revealing a security issue lets everyone know that it's there.

Okay, now that's out of the way, let me explain why the use of extract() here isn't the security concern that you think it is...

  1. Although the PHP manual says "Warning Do not use extract() on untrusted data" (which is presumably why you wrote the PR) you'll also have noticed these lines which come before extract() that ensure that we trust the source of the data.
  2. With one exception (see next point) the values in the extracted variables are not used - except to make decisions about what to save in the plugin options. The content of these options is not derived from $_POST.
  3. You'll also notice that where there is an array in the $_POST data, the contents of the array is sanitised - because it is saved in the database.

Now, you could argue that extract() makes it difficult to find the origin of the variable - but that's a separate stylistic issue. Indeed, this is why WordPress ditched its usage of extract() - not because it's a security concern but because variables appear "magically". For reference, here's the relevant ticket:

https://core.trac.wordpress.org/ticket/22400

Right, that said, I agree that extract() is not elegant but if I were to replace the existing code, I'd write something along the lines of:

$results = [];
$vars = [ 'one', 'two', 'three' ];
foreach( $vars AS $var ) {
    $results[$var] = empty( $_POST[$var] ) ? $_POST[$var] : '';
}

Or something like that.

I'm going to close this PR because I don't want the commit or merge history to contain the phrase "Security bug fixed" which might unnecessarily worry the users of this code.