Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Possible security issue through path disclosure? #4971

Closed
Anguel opened this Issue · 24 comments

7 participants

@Anguel

Hi,

I just found this:
http://packetstormsecurity.org/files/117658/contao-disclose.txt
and thought I would post this here.

Has this issue been addressed? I have no idea if it is severe or not.

Regards,
Anguel

@leofeyer
Owner

It has not yet been addressed, because it is not severe and therefore not on top of the priority list.

The exception, in which the full path is visible, is only thrown if "displayErrors" is turned on in the back end settings, which should not be the case for productional websites. Otherwise only the yellow box saying "an error occurred" is displayed, which does not contain any path information.

Also, it seems that triggering the exception is all a possible exploit can do. I could not manipulate the SQL data in my tests, however I am not an experienced hacker. Maybe the author of the article knows how to.

@aschempp
Collaborator

Just an idea while reading this: why not show a top bar (similar to the old IE6 message) if the error output is enabled?

@NinaG

+1

@leofeyer
Owner

That's a good idea, although it does not fix the actual problem :)

We have to check whether it is possible to inject DB code which is then executed (instead of just triggering the Exception). I don't consider the Exception alone as a security issue, because an Exception with a stack trace is what we want to see while developing the website. And productional systems show the yellow box instead anyway.

BTW, I wonder why the PHP functions do not strip the full path right away. At least optionally.

@Anguel

Regarding the top bar proposal: A clearly visible reminder in the backend will probably be enough to warn that error output is still enabled. Otherwise people are happy that their site finally works and just forget to disable error output :-)

@Anguel

Just an idea: Is it an option to contact the guy who has found the vulnerability, thank him for the hint and ask him for some more details? http://packetstormsecurity.org/contact/

@aschempp
Collaborator

I agree, the reminder should only be in the backend. I suppose we can use the same notice as for the safe mode.

@leofeyer
Owner

@contao/workgroup-core The initial problem is still unfixed.

@aschempp
Collaborator

What is the original problem? Path disclosure or the possible SQL injection you mentioned (I can't see that)?

@leofeyer
Owner

The possible SQL injection which the author seems to see.

@aschempp
Collaborator
@leofeyer leofeyer referenced this issue from a commit
@leofeyer leofeyer More robust input validation in the back end filter menu and no more …
…absolute paths in error messages printed to the screen (thanks to aulmn) (see #4971)
3d43c11
@leofeyer
Owner

Fixed in 3d43c11. Besides providing a more robust input validation in the filters menu, I have modified the __error() and __exception() functions to only print relative paths to the screen (the logs will still contain absolute paths).

@leofeyer leofeyer closed this
@aschempp
Collaborator

Me like.

Just FYI, I think it would be good to split something like that into two commits, because they are each not related to each other.

@leofeyer
Owner

Well, they are related IMHO, because they are both countermeasures to the same issue :)

@LeoUnglaub

I know i am a little bit late on this issue, but i had a lot of other things on my mind. I have a problem with this bugfix. Don't get me wrong, the validating is a good idea. But manipulating the path in a stacktrace is a verrrrrrrrrrrrrrrry bad idea. I have never seen something like that.

There are two reasons why this is a bad and not needed change.

  • those information's are for developers only and they should never be visible on a live website.
  • the path in the stacktrace is VERY important for debugging purpose. You need to have the real path in it because not every installation is so easy that you have everything in an htdocs folder. If you have for example merged filesystems for the core you definitive need the full path.

@leofeyer Please can you revert the path disclosure.
Thanks and greetings
Leo

@leofeyer
Owner

should never be visible on a live website

Yeah, however experience has shown that it is.

You need to have the real path in it because not every installation is so easy that you have everything in an htdocs folder

Can you provide a real-world example?

@LeoUnglaub

Can you provide a real-world example?

Yes. For example Tristan is using an "overlay filesystem" for the extension development. I think he uses UnionFS for that.

Another example for that are softlinks. Xtra, Me, ... use them to link extensions into system/modules but have them in seperate external directorys with there own git tree. In this case it's very important to see the path before TL_ROOT to, because he differs from the core files. With the current state of __error() i can't see what file is used.

Greetings
Leo

a-normal-system-modules

@leofeyer
Owner

You do realize that only the root path is stripped, don't you? Any subfolder above will be preserved. Did you come across an error message which paths were actually ambiguous?

@tristanlins

@LeoUnglaub are you sure?
There is a simple replace str_replace(TL_ROOT, '…', $e->getFile()) that should not strip the pathname from file that are outside of TL_ROOT.

@xchs

Regarding 3d43c11#L2R156 : there is an issue with the ellipsis unicode character which is sometimes displayed as …. I have seen this a couple of times lately in the PHP stack trace error messages, e.g.

Fatal error: Uncaught exception Exception with message Query error: Table 'contao_develop.tl_log' doesn't exist (INSERT INTO tl_log (tstamp, source, action, username, text, func, ip, browser) VALUES(1361210873, 'BE', 'CRON', '', 'Purged the internal cache', 'Automator purgeInternalCache()', 'xxx.xxx.xxx.xxx', 'Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.57 Safari/537.17')) thrown in …/system/modules/core/library/Contao/Database/Statement.php on line 346

#0 …/system/modules/core/library/Contao/Database/Statement.php(261): Contao\Database\Statement->query()
#1 …/system/modules/core/library/Contao/System.php(159): Contao\Database\Statement->execute(1361210873, 'BE', 'CRON', '', 'Purged the inte...', 'Automator purge...', 'xxx.xxx.xxx.xxx', 'Mozilla/5.0 (Wi...')
#2 …/system/modules/core/library/Contao/Automator.php(193): Contao\System::log('Purged the inte...', 'Automator purge...', 'CRON')
#3 …/contao/install.php(206): Contao\Automator->purgeInternalCache()
#4 …/contao/install.php(957): InstallTool->run()
#5 {main}
@leofeyer
Owner

This is most likely due to Apache falling back to the default charset (not being UTF-8). Maybe we should add AddDefaultCharset UTF-8 to the .htaccess.default file?

@xchs

Or we just replace the ellipsis character by three simple dots ... :smile:

But anyway, adding AddDefaultCharset UTF-8 to the server configuration file might be a good idea as well.

@leofeyer
Owner

Removed in 3054b1a. Three dots just does not feel correct somehow :)

@xchs

:+1:

Three dots just does not feel correct somehow

Yeah, I know :) However, since the three dots were already used as a replacement for the ellipsis character in the stack trace messages (see e.g. the shortened strings for the log entry in the example above), I thought it might fit here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.