Implements wrapper method for a more secure unserialize with PHP 7 #13285
Conversation
sgiehl
added
c: Security
as long as we support PHP 5.6 we should still have our Unit+Integration+System tests running 5.6. So maybe you could make the two MySQLI jobs |
sgiehl
force-pushed
the
secureunserialize
branch
from
0f6bd07
to
62bca2c
Compare
|
FYI @mattab we support PHP 5.5.9+ unless I missed something? |
|
Yes 👍meant 5.5 not 5.6 |
|
The UI tests are currently running on PHP 5.5. All other test suites are using PHP 5.6. |
sgiehl
force-pushed
the
secureunserialize
branch
2 times, most recently
from
9f043a0
to
f566c47
Compare
| } | ||
|
|
||
| return @unserialize($string); | ||
| } |
There was a problem hiding this comment.
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.
I see a lot of uses of @unserialize(), what about allowing callers to specify whether Common::safe_unserialize() should have errors silenced? Think that might be more flexible.
There was a problem hiding this comment.
@diosmosis you mean something like a third parameter that defines whether to ignore error or not?
Btw. unserialize doesn't throw an exception if the values can't be deserialized. It triggers a E_NOTICE. For PHP 7 it might be fine as it's "catchable" with the Error class. For PHP 5.x that simply results in errors depending on the php configuration.
Do you think it might be suitable to always ignore errors ion PHP 5 and always catch errors on PHP 7, but trigger a Log::debug in that case?
There was a problem hiding this comment.
I thought ErrorHandler might turn the notice into an exception, but I can't really remember and haven't tested it... But for this I figured we could let the callers silence Common::safe_unserialize() if they wanted, eg:
$value = @Common::safe_unserialize($value);
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.
added a debug log for that case and also added some tests...
|
I guess the .travis.yml change needs to be tweaked to whatever is decided upon, otherwise left one comment. |
sgiehl
force-pushed
the
secureunserialize
branch
from
f566c47
to
ee36092
Compare
sgiehl
force-pushed
the
secureunserialize
branch
from
ee36092
to
19b6141
Compare
| 'message' => $e->getMessage(), | ||
| 'backtrace' => $e->getTraceAsString() | ||
| ]); | ||
| return false; |
There was a problem hiding this comment.
Can we log the string as well? For a debug log I presume it's ok.
There was a problem hiding this comment.
Added this myself, let me know if that's a problem (can remove in new PR if needed).
|
Left one comment, otherwise looks good to merge. |
…atomo-org#13285) * Implements wrapper method for a more secure unserialize * run AllTests on PHP 7 * trigger a debug message if unserialize fails on PHP 7 + tests * Add string to deserialize to debug log.
As this changes will only affect PHP 7 we shouldn't merge this unless we have successful tests running with PHP 7.
I've switched the tests to PHP 7 on this branch, but it would be good to have them continuous to avoid problems in the future