Avoid setting an explicit session ID via GET args.

[coling]

This is considered a failing metric in automated PCI scans under the "session hijacking" category and thus should be avoided.

PHP 4.3 introduced the "session.use_only_cookies" PHP configuration option which meant that passing in a session ID via GET/POST variables can be disabled. The code in Joomla should at very least honour this setting.

Alternatively, if no good reason for this code exists, it should be removed entirely.

Summary of Changes

Add an additional condition around code which allows user-passed arguments to be used as the session ID when no current session is set (i.e. fresh landing). This additional condition honours php.ini configuration.

Testing Instructions

1. Visit a joomla site
2. Note the session cookie name and delete the current session cookies
3. In a separate browser/private window, visit the same joomla site and log in.
4. In the authenticated session, extract the session id from the cookies
5. Visit the site again in the non-private window (no active session), but pass in arguments ?SESSIONNAME=mygeneratedsessionid in the URL.
6. You have now hijacked the session and should be authenticated as the user from the private window

Actual result BEFORE applying this Pull Request

If the php.ini setting use_only_cookies is turned on, the session hijack will still work by passing in GET args.

Expected result AFTER applying this Pull Request

If the php.ini setting use_only_cookies is turned on, the session hijack will NOT work by passing in GET args.

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org:

• No documentation changes for manual.joomla.org needed

[carlitorweb]

I cant reproduce the problem, but maybe I did something wrong.

I just saw my mistake. I could now

[brianteeman]

https://github.com/joomla/joomla-cms/security/policy

[SniperSister]

Commenting from the JSST side of things: I would not consider it as a security issue on it's own, as it requires that an authenticated session ID has been extracted in the first place. If that succeeds, it doesn't make a difference if the session ID can be supplied as GET/POST arg or COOKIE arg, as cookies are user supplied content too and an attacker could very easily supply the attacked ID as a cookie.

Nevertheless it's worth fixing as we should respect the mentioned php.ini setting the application.

[brianteeman]

Nevertheless it's worth fixing as we should respect the mentioned php.ini setting the application.

This code doesnt do that

The ini_get() function returns a string value, so it won't return empty() even if the value is false.

[brianteeman]

I have tested this item 🔴 unsuccessfully on 7b7f68d

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43451.}

[coling]

Nevertheless it's worth fixing as we should respect the mentioned php.ini setting the application.

This code doesnt do that

The ini_get() function returns a string value, so it won't return empty() even if the value is false.

I based my code on setting the value in the .ini files and the resulting return values in code. I didn't test using e.g. ini_set() to set it to a (string) value.

[x]$ echo "session.use_only_cookies = On" > on.ini 
[x]$ echo "session.use_only_cookies = Off" > off.ini
[x]$ php -c on.ini -r 'var_dump(ini_get("session.use_only_cookies")); var_dump(empty(ini_get("session.use_only_cookies")));'
string(1) "1"
bool(false)
[x]$ php -c off.ini -r 'var_dump(ini_get("session.use_only_cookies")); var_dump(empty(ini_get("session.use_only_cookies")));'
string(0) ""
bool(true)

(FWIW the same results are observed when setting the on/off values to 1 and 0 and true and false too)

In this context the fix appears to work? But appreciate that more robust parsing would be better.

Before I write more robust parsing of ini_get() results (feel free to point me to an example), does anyone have any comment as to whether the code is needed at all?

[SniperSister]

does anyone have any comment as to whether the code is needed at all?

Yes, it makes sense!

[coling]

Commenting from the JSST side of things: I would not consider it as a security issue on it's own, as it requires that an authenticated session ID has been extracted in the first place. If that succeeds, it doesn't make a difference if the session ID can be supplied as GET/POST arg or COOKIE arg, as cookies are user supplied content too and an attacker could very easily supply the attacked ID as a cookie.

Nevertheless it's worth fixing as we should respect the mentioned php.ini setting the application.

Indeed. I did consider first reporting it as a security issue but came to the same conclusion. 👍🏼

But the PCI scan result marked it as failing, so regardless of whether or not I think the real issue is elsewhere I've still got to fix it!!

[coling]

does anyone have any comment as to whether the code is needed at all?

Yes, it makes sense!

For the avoidance of doubt, I'm referring to the functionality of the exiting code rather than my change ! Just for my own piece of mind, can you give an example of when this would be useful? i.e. a real world situation when you want to initialise a specific session?

The best fix is just to kill off that whole conditional block if possible 😄

[SniperSister]

@coling ah ok, then we had a misunderstanding: my "it makes sense" was referring to your pull request, not the existing code.

Regarding the existing snippet: I don't have any useful example for a potential usage, however we have to stick to the semver policy of the project and mark the code block as deprecated and leave it for removal in future versions.

[coling]

I have tested this item 🔴 unsuccessfully on 7b7f68d

Can you clarify what you mean by unsuccessfully here? Were you able to reproduce the initial issue as per @carlitorweb or could you not reproduce? Or did my change not prevent the issue after applying?

As per #43451 (comment), setting the values in the .ini files should be fine to parse via an empty() check as per the PHP documentation notes.

But that said, I do have an updated fix which uses filter_var() to make the parsing more robust. I'm happy to push that if you can clarify what other parts failed for you.

Cheers

[coling]

Hi @brianteeman Do you have any further info about the failure case as requested above? I'm keen to get this updated for merging but need a little more info (see my comments above). Cheers!