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

Check _temp_tarname before deleting temp file in destructor. #34

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

mcdruid
Copy link
Contributor

@mcdruid mcdruid commented Feb 1, 2021

This is a security hardening which I believe is safe to discuss in public.

The __destruct() method can be abused via deserialization / object injection to delete arbitrary files.

As it looks like _temp_tarname is only ever set in one place:

    public function _openRead()
    {
        if (strtolower(substr($this->_tarname, 0, 7)) == 'http://') {

            // ----- Look if a local copy need to be done
            if ($this->_temp_tarname == '') {
                $this->_temp_tarname = uniqid('tar') . '.tmp';

...it seems like we should be able to check the filename before actually deleting a "temp file".

Additional checking could be done to ensure that _tarname matches the above etc..., but just checking _temp_tarname should be sufficient to avoid abuse.

@mrook
Copy link
Member

mrook commented Feb 1, 2021

LGTM, thanks!

@mrook mrook merged commit 10db38c into pear:master Feb 1, 2021
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

Successfully merging this pull request may close these issues.

2 participants