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

I have found what I think to be a possible unserialize insecurity #2345

Closed
devcoinfet opened this issue Jan 3, 2018 · 4 comments
Closed

Comments

@devcoinfet
Copy link

devcoinfet commented Jan 3, 2018

your making unserialize calls using that file manager

elFinderSession.php

	protected function decodeData($data)
	{
		if ($this->base64encode) {
			if (is_string($data)) {
				if (($data = base64_decode($data)) !== false) {
					$data = unserialize($data);
				} else {
					$data = null;
				}
			} else {
				$data = null;
			}
		}
		return $data;
	}

it's checking that data is a base64 encoded string per the RFC for serialized data this is spot on but the problem I see here is the value contained therein inside of that data is passed directly to unserialize. I'm not sure if you're aware but it is creating an object out of the data that is serialized and the tool also has autoload modules and constructors in the class so if this data is at any point able to be manipulated by an unscrupulous admin or user this could lead to arbitrary code execution

my question to you is what purpose does this serve in your code?

and does it allow for the data to be manipulated?

like could I connect to this file and directly pass it data to your installs?

@nao-pon
Copy link
Member

nao-pon commented Jan 3, 2018

@devcoinfet Regarding concerns you pointed out, I think that elFinder itself has no problem. The main points are as follows.

  • The base64encode option is disabled by default
  • Passed data is session variable only

However, satisfying the following requirements is vulnerable to object injection attacks.

  • Enable base64encode option
  • Not use the elFinderSession session handler and saves data passed from the user to the session variable directly

If you use elFinder, you must not meet all of these conditions.

@devcoinfet
Copy link
Author

no, I do not use it I found this by auditing another codebase that uses it. If its not an issue glad to hear it

@nao-pon nao-pon closed this as completed in 91c2b43 Jan 4, 2018
@nao-pon
Copy link
Member

nao-pon commented Jan 4, 2018

@devcoinfet OK, I added the warning about object injection attacks into source code comment.

Thanks! 👍

@devcoinfet
Copy link
Author

anytime, glad I could help

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

No branches or pull requests

2 participants