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

Disable the phar stream wrapper #105

Merged
merged 2 commits into from Oct 8, 2018
Merged

Conversation

ausi
Copy link
Member

@ausi ausi commented Sep 27, 2018

As of the newly discovered unserialization attack via the phar:// stream wrapper (Black Hat talk), we should disable the phar stream wrapper for the whole application.

It should be noted that this is only a preventive measure and shouldn’t be considered a “fix” for this kind of attacks. We must never pass user controlled values directly to any file operation functions without validating them.

If anybody needs the stream wrapper for a specific use case in their project or extension, I think the following code snippet should be safe to use:

if ($pharDisabled = !\in_array('phar', stream_get_wrappers(), true)) {
	stream_wrapper_restore('phar');
}
try {
	// Your code using phar://...
}
finally {
	if ($pharDisabled) {
		stream_wrapper_unregister('phar');
	}
}

@leofeyer leofeyer added the bug label Sep 28, 2018
@leofeyer leofeyer added this to the 4.4.27 milestone Sep 28, 2018
@leofeyer
Copy link
Member

What about adding this in src/Resouces/contao/functions as we do to initialize the Patchwork UTF8 library? The file is then loaded via Composer.

@ausi
Copy link
Member Author

ausi commented Sep 28, 2018

Then it would be disabled in any application that requires contao/core-bundle. Do we want that?

I think this is probably not something a bundle should do.

@aschempp
Copy link
Member

Didn't we agree to have an environment flag to disable this in the entry point?

@ausi
Copy link
Member Author

ausi commented Sep 28, 2018

As anyone can reenable the stream wrapper with stream_wrapper_restore('phar') we concluded that the environment flag is not necessary AFAIR.

@leofeyer
Copy link
Member

Then it would be disabled in any application that requires contao/core-bundle. Do we want that?

Probably yes, but we could also add it in the manager bundle instead.

@ausi
Copy link
Member Author

ausi commented Sep 28, 2018

This code is not related to autoloading, so it should not go into the composer.json autoload config IMO.

Any opinions from the other @contao/developers ?
Should this code go into the app.php or into src/Resouces/contao/functions and be loaded via Composer?

@Toflar
Copy link
Member

Toflar commented Sep 28, 2018

app.php.

@leofeyer leofeyer merged commit 55f46c6 into contao:4.4 Oct 8, 2018
@leofeyer
Copy link
Member

leofeyer commented Oct 8, 2018

Thank you @ausi.

leofeyer added a commit that referenced this pull request Oct 31, 2018
leofeyer added a commit that referenced this pull request Oct 31, 2018
@leofeyer leofeyer modified the milestones: 4.4.27, 4.4 May 14, 2019
@ausi ausi deleted the fix/disable-phar-wrapper branch December 3, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants