Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File Inclusion with Possible RCE #574

Closed
Shinkurt opened this issue Dec 18, 2016 · 5 comments
Closed

File Inclusion with Possible RCE #574

Shinkurt opened this issue Dec 18, 2016 · 5 comments

Comments

@Shinkurt
Copy link

Shinkurt commented Dec 18, 2016

In Piwigo/admin/plugin.php

32: $sections = explode('/', $_GET['section']); 
59: $filename = PHPWG_PLUGINS_PATH . implode('/', $sections); 
60: if(is_file($filename))
62: include_once ($filename); 

As you can see, a user-controlled variable is being sent stright to include_once(). a person who managed to upload a .jpg file or any other content with PHP paylaod will just need to refer to that location and include_once() will execute it, making a code execution possible.

@plegall
Copy link
Member

plegall commented Dec 19, 2016

There are quite some security checks before including anything. Uploading a JPEG in the "upload" directory won't be able to get his file included. You can't have ".." as part of the $sections, for example.

@Shinkurt
Copy link
Author

Hey @plegall,

If you are implying:

if (!preg_match('/^[\w-]+$/', $plugin_id))
{
  die('Invalid plugin identifier');
}

As a security fix, it isn't sufficent because it only sanitizes $plugin_id = $sections[0];

How about other $secution[i] other than [0]? they aren't sanitized, we just have to send in another array. all the security checks apply to the $plugin_id which is just the first occurance of $sections.

Thanks,

@plegall
Copy link
Member

plegall commented Dec 20, 2016

$sections = explode('/', $_GET['section'] );
for ($i=0; $i<count($sections); $i++)
{
  if (empty($sections[$i]) or $sections[$i]=='..')
  {
    unset($sections[$i]);
    $i--;
  }
}

No part of the $sections can be ".."

@Shinkurt
Copy link
Author

@plegall not good enough. because that checks if $sections[i] is exactly "..", which is sad cause attackers can send in /..// or /..%2F and the $sections[$i]=='..' will return false. I suggest you doing a preg_replace() on it rather that one. tis easy to jump.

@plegall plegall closed this as completed in 9004fdf Jan 1, 2017
plegall added a commit that referenced this issue Jan 1, 2017
@plegall plegall self-assigned this Jan 1, 2017
@plegall plegall added this to the 2.8.5 milestone Jan 1, 2017
@Shinkurt
Copy link
Author

Shinkurt commented Jan 3, 2017

Use CVE-2016-10105 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants