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

Avoid Full Path Disclosure error on session error. #306

Merged
merged 1 commit into from
Aug 24, 2015

Conversation

ArthurHoaro
Copy link
Member

  • Add a function to validate session ID.
  • Generate a new session ID if an invalid token is passed.

related to #298

ping @TeamAlexandriZ

@@ -82,6 +69,23 @@
exit;
}

// Force cookie path (but do not change lifetime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this code section be cleaned up (indentation + comments)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@virtualtam
Copy link
Member

@Knah-Tsaeb @TeamAlexandriZ could you test this PR and give us some feedback? :)

@TeamAlexandriZ
Copy link

Give me a patched online target and I'll looking around. (with server error enabled for test this patch)

@virtualtam virtualtam modified the milestones: 0.5.1, 0.5.2 Aug 17, 2015
@ArthurHoaro ArthurHoaro force-pushed the fpd branch 2 times, most recently from 167a0fa to 6d0e5a3 Compare August 22, 2015 08:05
  * Add a function to validate session ID.
  * Generate a new session ID if an invalid token is passed.
@ArthurHoaro
Copy link
Member Author

@TeamAlexandriZ
Copy link

Hi, it's OK for cookie but on this server all php file of the folder /tests give a full path disclosure to :
https://workspace.hoa.ro/shaarli-fpd/tests/CachedPageTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/CacheTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/ConfigTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/LinkDBTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/TimeZoneTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/UrlTest.php
https://workspace.hoa.ro/shaarli-fpd/tests/UtilsTest.php

Error found :
Warning: require_once(application/Utils.php): failed to open stream: No such file or directory in /var/www/html/codiad/workspace/shaarli-fpd/tests/UtilsTest.php on line 6

Fatal error: require_once(): Failed opening required 'application/Utils.php' (include_path='.:/usr/share/pear:/usr/share/php') in /var/www/html/codiad/workspace/shaarli-fpd/tests/UtilsTest.php on line 6

@virtualtam
Copy link
Member

@TeamAlexandriZ thanks for testing!

@ArthurHoaro that's weird, there should be a .htaccess for the tests/ directory preventing access

@ArthurHoaro
Copy link
Member Author

I need to check but it might only be because I disallowed htaccess rules.

@ArthurHoaro
Copy link
Member Author

Confirmed, with htaccess rules, those files return an error 403.

@virtualtam
Copy link
Member

Merge-ready then ;-)

virtualtam added a commit that referenced this pull request Aug 24, 2015
Avoid Full Path Disclosure error on session error.
@virtualtam virtualtam merged commit ce8e248 into shaarli:master Aug 24, 2015
@TeamAlexandriZ
Copy link

On Nginx, the .htaccess doesn't work.

@virtualtam
Copy link
Member

@TeamAlexandriZ Nginx's configuration and philosophy are quite different from Apache's (and related to server configuration rather than to this particular PR)

There is a wiki page dedicated to Server configuration which has sections for Apache2 and Nginx, and aims at providing Shaarli users with a sane, secure server setup -all improvements are welcome!

@TeamAlexandriZ
Copy link

Yes, but add

location ^~ /tests/ {
deny all;
}

to your /etc/nginx/deny.conf wiki example because actually there are nothing that deny the access for Nginx users and the FPD is visible.

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 3, 2015
Improves shaarli#306
Relates to shaarli#335 & shaarli#336

Issue:
 - the format of the value returned by `uniqid()` depends on PHP settings
 - the regex checking the session ID does not cover all cases

Fix:
 - apply a hash function to the session ID (SHA1)

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

TODO:
 - remove `uniqid()` usage

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 6, 2015
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <[email protected]>
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.

3 participants