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

[fix] FUD Full Path Disclosure #298

Closed
wants to merge 2 commits into from
Closed

Conversation

Knah-Tsaeb
Copy link

@TeamAlexandriZ report Full Path Disclosure sebsauvage#222

@ArthurHoaro
Copy link
Member

This doesn't work because the setting needs to be set before session initialisation.

@ArthurHoaro ArthurHoaro added bug it's broken! security in progress and removed bug it's broken! labels Jul 24, 2015
@Knah-Tsaeb
Copy link
Author

D'oh ! Of course, my bad. I would like go too fast.
How to cancel a pull request ?

@TeamAlexandriZ
Copy link

Hi,
I've see the fix +ini_set('display_errors', false);
It's a way... but i'm not sure it's the best way.
This is a full path disclosure by null session cookie, this is not a configuration problem but coding problem, hide the problem does not correct him. See this blog post of this well know pentester and dev of famous wpscan.
http://blog.dewhurstsecurity.com/2011/10/05/full-path-disclosure-fpd.html
The user cookie input shaarli must be sanitized as all user input to not cause error.
But it's just my opinion.

@nodiscc
Copy link
Member

nodiscc commented Jul 25, 2015

Hi everyone, @TeamAlexandriZ thank you very much for reporting this, nice find!
@Knah-Tsaeb thanks for the patch.

About fixing this

  • errors should never be displayed in production or on an internet-facing server, unless the admin has a very good reason. In this case, the vulnerable servers are improperly configured.

@nodiscc nodiscc added the bug it's broken! label Jul 25, 2015
@virtualtam
Copy link
Member

Hello!

Quick question (no Apache to run tests right now): does it disable all logging errors (i.e., will it hide errors on a debug server)?

In Apache, most settings can be configured through both virtualhost definition and custom .htaccess files, e.g.:

<VirtualHost *:80>
    ServerName   shaarli.my-domain.org
    DocumentRoot /absolute/path/to/shaarli/

    LogLevel  warn
    ErrorLog  /var/log/apache2/shaarli-error.log
    CustomLog /var/log/apache2/shaarli-access.log combined

    # do not display PHP errors
    php_flag display_startup_errors off
    php_flag display_errors off
    php_flag html_errors off

    # log PHP errors to a file
    php_flag  log_errors on
    php_value error_log  /var/log/apache2/shaarli-php-error.log
</VirtualHost>

Could someone check if this works, and update the Server configuration page accordingly?

@ArthurHoaro
Copy link
Member

Hi I don't have much time tonight but:

  • I'll submit a PR soon which prevent the error (so no FPD issue).

@ArthurHoaro how do we set this before the session is initialized?

  • just declare it before the session initialization.
  • @virtualtam php.ini, vhost config, .htaccess, and PHP code.

For example, on my server, in php.ini, I have display_errors = Off. But for my workspace virtual apache host, I added:

php_value error_reporting 2147483647 # constant for E_ALL
php_value display_errors 1

Should display_errors be forced for Shaarli... I don't know.

@virtualtam
Copy link
Member

A quick word about configuration priority :)

  • php.ini is overridden by application settings
    • I don't know if PHP flags passed in a VirtualHost definition override app setting, though
  • .htaccess are overriden by VirtualHosts, which in turn are overriden by the main Apache config

The virtual host definition can be used for convenience, to group all settings (native Apache entries, flags passed to PHP) in the same place. The real advantage here is that it is independent from the applications' sources, thus safe from upstream changes made to PHP flags or htaccess restrictions.

php_value error_reporting 2147483647 # constant for E_ALL

Stumbled on this SO thread as well, it looks both hackish (the value for E_ALL changes depending on the PHP version) and deprecated (see PHP Error function constants).

php_value display_errors 1

Shouldn't it be php_value display_errors on ?

If needed, I'll be back to a proper development environment next week to run more extensive tests :)

@virtualtam
Copy link
Member

@Knah-Tsaeb @TeamAlexandriZ should we close this PR as a duplicate of #306?

@nicolasdanelon
Copy link

I use Apache do you want me to test this?

On Thu, 13 Aug 2015 14:33 VirtualTam notifications@github.com wrote:

@Knah-Tsaeb https://github.com/Knah-Tsaeb @TeamAlexandriZ
https://github.com/TeamAlexandriZ should we close this PR as a
duplicate of #306 #306?


Reply to this email directly or view it on GitHub
#298 (comment).

@virtualtam
Copy link
Member

@nicolasdanelon If you have some time to toy with Apache and PHP configuration, that'd be awesome!

In the near future, there'll be development-ready Docker templates / images under docker-shaarli to experiment with config with no fear of breaking everything ;-)

@nicolasdanelon
Copy link

Using you configuration posted on 29 / 06 / 2015 with user shaarli and passwod shaarli

Name shaarli
Value bno9e4tbqqsd8odrlaonhj1et0
Host .shaarli.nicolasmd.com.ar
Path /
Expires Sat, 13 Aug 2016 16:52:29 GMT
Secure No
HttpOnly No

@virtualtam virtualtam modified the milestones: 0.5.1, 0.5.2 Aug 17, 2015
@ArthurHoaro
Copy link
Member

Alright, I'm not sure what we're talking about. I was just giving an example.

The question here is: should we force Shaarli to not display PHP errors?

IMHO, no. It should rely on server configuration. Anyway, if we do, this PR needs to be rebased.

@nodiscc
Copy link
Member

nodiscc commented Aug 20, 2015

should we force Shaarli to not display PHP errors?

IMHO, Yes by default unless a particular option is set in config.php.

@virtualtam
Copy link
Member

Closing this issue, as #306 adds the proper session cookie checks

@virtualtam virtualtam closed this Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug it's broken! security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants