Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Fail with pathconfig.php #6855

Closed
luger95 opened this issue Apr 4, 2014 · 33 comments
Closed

Fail with pathconfig.php #6855

luger95 opened this issue Apr 4, 2014 · 33 comments
Labels
Milestone

Comments

@luger95
Copy link

luger95 commented Apr 4, 2014

Hello,

I just noticed that running a url like this, it changes the value of pathconfig.php and even without being connected to the backoffice.

removed for security

@andreasisaak
Copy link

Failed in 2.11, 3.1 and 3.2

@frontendschlampe
Copy link

same here ... :-/

@kikmedia
Copy link

kikmedia commented Apr 4, 2014

Confirmed for all systems with a pathconfig.php

@backbone87
Copy link
Contributor

quickfix:
folgende zeile:
2.11: https://github.com/contao/core/blob/support/2.11/contao/install.php#L72
3.x: https://github.com/contao/core/blob/master/contao/install.php#L51
ersetzen mit:

$strScript = strval($_SERVER['SCRIPT_NAME']);
preg_match('#^(.*)/contao/install\.php$#', $strScript, $arrMatches);
$strPath = strval($arrMatches[1]);
if ($strPath != TL_PATH)

@discordier
Copy link
Contributor

Confirmed security issue.
Users must(!) protect install.php until final fix is released.

@tristanlins
Copy link
Contributor

We can confirm, that this is a security issue.
To protect your install.php you will found some solutions in our news https://c-c-a.org/aktuelles/news/details/eine-neue-kritische-sicherheitsluecke-in-contao-wurde-entdeckt

@backbone87
Copy link
Contributor

My quickfix is not sufficient!

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

Just replace Environment::get('requestUri') with Environment::get('scriptName') in contao/install.php and system/initialize.php. Any objections?

@backbone87
Copy link
Contributor

not sufficient, your current scriptName implementation (which is horribly inconsistent across different sapis) will leave this issue open for at least cgi-fcgi

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

horribly inconsistent across different sapis

Please post the output of the $_SERVER array on your server. We have added unit tests for the Environment class in the "Contao on Symfony" branch and could not detect any inconsistencies.

@backbone87
Copy link
Contributor

schaust du hier: #5881

@discordier
Copy link
Contributor

IMO the pathconfig.php MUST NOT be deleted without proper authentication.
The install tool may define the constant on it's own if it finds a difference (to have the install tool itself find the correct style sheet etc) but MUST NOT persist them to file for untrusted calls.

Next, the content of the resulting path MUST be cleaned or even better validated.
For the validation, we can not assume that the subdir is the same as on the file system (installation within sub directory) as the installation might be within some "virtual directory" (alias etc.).
For the cleaning, at least critical characters like ["';,:.$] etc. must be rejected (do NOT try to sanitize/filter them, it does not make sense here).

This is not a quick fix like: "If value A is not right, let's use value B".

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

@backbone87: As I already commented in #5881, we are using the exact same code as TYPO3 to determine the script name:

https://github.com/TYPO3/TYPO3.CMS/blob/TYPO3_3-8/t3lib/class.t3lib_div.php#L2668

https://github.com/TYPO3/TYPO3.CMS/blob/639d5e588b4804ddd1194cd479b6b25c61616d46/typo3/sysext/core/Classes/Utility/GeneralUtility.php#L3156

And the bug in the other ticket has not yet been confirmed.

@discordier
Copy link
Contributor

@leofeyer @backbone87 No matter if the other bug is confirmed or not but this issue is about using untrusted input without any further check and persisting it into a config file without authentification and authorization.

No matter from which environment it originates (granted, browser is plain wrong but who guarantees that the server setup does not allow to manipulate the $_SERVER it).

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

Just tested my fix on uberspace.de (cgi-fcgi environment) and it did actually work.

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

persisting it into a config file without authentification and authorization

The pathconfig.php needs to be written when you first set up Contao. Before you even set the install tool password. Therefore there is no other way than to do it at this point.

@discordier
Copy link
Contributor

Of course, if there IS an install tool password, do NOT update the path.
If there is NO install tool password, assume a fresh installation and go on.

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

It is not that easy. After moving an installation, the install tool shall be able to auto-detect the path change and update the configuration accordingly. At this point, there is an install tool password.

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

And if the install tool does not auto-detect the path change, there is no way to enter the install tool password. (Assuming your next question will be "why don't we auto-detect after the install tool password has been entered".)

@discordier
Copy link
Contributor

I know that, that's why I said, the install tool shall detect it:

The install tool may define the constant on it's own if it finds a difference (to have the install tool itself find the correct style sheet etc) but MUST NOT persist them to file for untrusted calls.

This is:

  1. detect change, use the new value but leave pathconfig.php unchanged.
  2. Install tool will prompt "Path appears changed, please authenticate."
  3. After authenticating the user in the install tool, the new pathconfig will get saved.

Of course after moving the installation the frontend will be broken and all of the backend (aside of the install tool) but we should not care about that as it is to be fixed by logging into the install tool.

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

Ok, so here's my proposal (see 427661a).

  1. Only the install tool is allowed to persist the website path and this will only happen after the user has authenticated.
  2. The initialize.php script will define (but not persist) the TL_PATH constant, so it is possible to work with the back end even if there is no pathconfig.php yet.
  3. Any convenience routines, which automatically removed the pathconfig.php file or deleted the websitePath parameter, have been removed.

The drawback, however, is that if you move an installation, the system is not usable anymore unless you have manually adjusted or removed the pathconfig.php file. You cannot even log into the install tool with a wrong website path.

What are your thoughts?

@backbone87
Copy link
Contributor

at a first glance, i am fine with it

@fiedsch
Copy link

fiedsch commented Apr 4, 2014

The drawback, however, is that if you move an installation, the system is not usable anymore unless you have manually adjusted or removed the pathconfig.php file. You cannot even log into the install tool with a wrong website path.

What are your thoughts?

Personally I don't have any problems with the mentioned drawback, as I always manually adjust localconfig.php and/or pathconfig.php as this seems faster than using the installtool (after searching for the installtool password).
If this is only my +1 against many -1 -- I can't tell.

@discordier
Copy link
Contributor

See PR #6858.
Something like this should solve the problem for the install.php, shouldn't it?

I have not tested it thoroughly but think it should work.
This solution will ignore the pathconfig.php when in install.php but use it for every other execution.

What are the exact downsides here?

@leofeyer
Copy link
Member

leofeyer commented Apr 4, 2014

Looks good to me. While reading your code, I notice that due to my changes, the install tool now writes the pathconfig.php file upon every request. Not good either :)

@leofeyer leofeyer added this to the 3.2.9 milestone Apr 4, 2014
@discordier
Copy link
Contributor

I was only addressing the problem about the install tool with my commit. :)

About the saving upon every request, see additional commit in my PR.

@discordier
Copy link
Contributor

@leofeyer
Copy link
Member

leofeyer commented Apr 5, 2014

Here's the latest version of the patch: hotfix/3.2.9...feature/pathconfig

Please revise and provide feedback, so we can release updates on Monday.

@tristanlins
Copy link
Contributor

Looks good for me, could not break my installation anymore :-)

Please revise these little optimizations: #6862 #6863

@aschempp
Copy link
Member

aschempp commented Apr 6, 2014

Is there a reason to set __SCRIPT__ in every file if we only need it for install.php?

@ausi
Copy link
Member

ausi commented Apr 6, 2014

Wouldn't it be better to use a different name, without double underscores, for the constant __SCRIPT__?

Maybe something like TL_SCRIPT_NAME?

From the PHP Manual:

// This is valid, but should be avoided:
// PHP may one day provide a magical constant
// that will break your script
define("__FOO__", "something");

@aschempp
Copy link
Member

aschempp commented Apr 6, 2014

I'd use something like define('TL_INSTALL', true); ...

@leofeyer
Copy link
Member

leofeyer commented Apr 7, 2014

Thank you everyone for your help!

@leofeyer leofeyer closed this as completed Apr 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests