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

Merged
merged 4 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@sgiehl
Member

sgiehl commented Aug 13, 2018

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

@mattab mattab added this to the 3.7.0 milestone Aug 17, 2018

@mattab

This comment has been minimized.

Member

mattab commented Aug 17, 2018

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

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 Alltests run php 5.6 ?

@mattab mattab modified the milestones: 3.7.0, 3.6.1 Sep 1, 2018

@sgiehl sgiehl force-pushed the secureunserialize branch from 0f6bd07 to 62bca2c Sep 9, 2018

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 9, 2018

FYI @mattab we support PHP 5.5.9+ unless I missed something?

@mattab

This comment has been minimized.

Member

mattab commented Sep 9, 2018

Yes 👍meant 5.5 not 5.6

@sgiehl

This comment has been minimized.

Member

sgiehl commented Sep 10, 2018

The UI tests are currently running on PHP 5.5. All other test suites are using PHP 5.6.
I'd suggest to switch the AllTests to PHP 7, so they are running a bit faster. But we need to adjust some more stuff so all tests have the same results across PHP versions.

@sgiehl sgiehl force-pushed the secureunserialize branch 2 times, most recently from 9f043a0 to f566c47 Sep 24, 2018

@mattab mattab requested a review from diosmosis Sep 27, 2018

}
return @unserialize($string);
}

This comment has been minimized.

@diosmosis

diosmosis Sep 28, 2018

Member

Should unserialize errors be reported in any way? Is it a problem if an exception is thrown here?

This comment has been minimized.

@diosmosis

diosmosis Sep 28, 2018

Member

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.

This comment has been minimized.

@sgiehl

sgiehl Sep 28, 2018

Member

@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?

This comment has been minimized.

@diosmosis

diosmosis Sep 28, 2018

Member

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.

This comment has been minimized.

@diosmosis

diosmosis Sep 28, 2018

Member

👍 for logging, though

This comment has been minimized.

@sgiehl

sgiehl Oct 9, 2018

Member

added a debug log for that case and also added some tests...

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 28, 2018

I guess the .travis.yml change needs to be tweaked to whatever is decided upon, otherwise left one comment.

@sgiehl sgiehl force-pushed the secureunserialize branch from f566c47 to ee36092 Oct 9, 2018

@sgiehl sgiehl force-pushed the secureunserialize branch from ee36092 to 19b6141 Oct 9, 2018

'message' => $e->getMessage(),
'backtrace' => $e->getTraceAsString()
]);
return false;

This comment has been minimized.

@diosmosis

diosmosis Oct 9, 2018

Member

Can we log the string as well? For a debug log I presume it's ok.

This comment has been minimized.

@diosmosis

diosmosis Oct 10, 2018

Member

Added this myself, let me know if that's a problem (can remove in new PR if needed).

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 9, 2018

Left one comment, otherwise looks good to merge.

@diosmosis diosmosis merged commit eb965d4 into 3.x-dev Oct 10, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@diosmosis diosmosis deleted the secureunserialize branch Oct 10, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

Implements wrapper method for a more secure unserialize with PHP 7 (m…
…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.

@J0WI J0WI referenced this pull request Nov 23, 2018

Open

Run tests against PHP 7 #88

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