-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Implements wrapper method for a more secure unserialize with PHP 7 #13285
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,6 +256,33 @@ public static function mb_strtoupper($string) | |
return $string; | ||
} | ||
|
||
/** | ||
* Secure wrapper for unserialize, which by default disallows unserializing classes | ||
* | ||
* @param string $string String to unserialize | ||
* @param array $allowedClasses Class names that should be allowed to unserialize | ||
* | ||
* @return mixed | ||
*/ | ||
public static function safe_unserialize($string, $allowedClasses = []) | ||
{ | ||
if (PHP_MAJOR_VERSION >= 7) { | ||
try { | ||
return unserialize($string, ['allowed_classes' => empty($allowedClasses) ? false : $allowedClasses]); | ||
} catch (\Throwable $e) { | ||
$logger = StaticContainer::get('Psr\Log\LoggerInterface'); | ||
$logger->debug('Unable to unserialize a string: {message} (string = {string})', [ | ||
'message' => $e->getMessage(), | ||
'backtrace' => $e->getTraceAsString(), | ||
'string' => $string, | ||
]); | ||
return false; | ||
} | ||
} | ||
|
||
return @unserialize($string); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should unserialize errors be reported in any way? Is it a problem if an exception is thrown here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a lot of uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @diosmosis you mean something like a third parameter that defines whether to ignore error or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought
I thought about exceptions but noticed some of the uses you changed assume it will only return false on failure, so I guess we can't throw everywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 for logging, though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a debug log for that case and also added some tests... |
||
|
||
/* | ||
* Escaping input | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we log the string as well? For a debug log I presume it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this myself, let me know if that's a problem (can remove in new PR if needed).