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

I just saw my mistake. I could now

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

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.

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 have tested this item ? unsuccessfully on 7b7f68d

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?

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

Yes, it makes sense!

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!!

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 ?

@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.

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

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!

Hi all (especially @brianteeman). Just checking in to make sure these fixes are not lost. Can you give me more info and I can adapt accordingly and update to latest branches etc.

Hello, there isn't much update on this... any progress?

Another ping to keep it fresh...

This pull request has been automatically rebased to 5.2-dev.

I have tested this item ✅ successfully on cbd39d4