BUG-10807: session_regenerate_id fails to check results of validateId… #10813

bgardner-noggin · 2023-03-08T22:39:18Z

session_generate_id should only invoke create_sid while the validateID result is failing

… correctly

bgardner-noggin · 2023-03-08T22:40:04Z

I'm not sure if I raised this correctly, but could this fix also be applied to PHP 8.0 as this is the version available for oraclelinux 9

nielsdos

Thank you! This looks almost perfect. Test case looks good and reliable too.
Just some nits about the naming.

I'm not sure if I raised this correctly, but could this fix also be applied to PHP 8.0 as this is the version available for oraclelinux 9

Almost ;) In general, bugfixes should go to the lowest bug-supported branch they apply to. In this case it's 8.1. So you should rebase your PR to that branch.
Normally this PR wouldn't be applied to 8.0 since that release is security-only, and this bug does not look like a critical bug as far as I can see.

nielsdos · 2023-03-08T22:45:28Z

ext/session/tests/bug10807.phpt

@@ -0,0 +1,69 @@
+--TEST--
+Bug #10807 (session_regenerate_id with custom handler and use_strict_mode generates three new session ids)


I agree this is a bit confusing at first... But "Bug #XXXXX" is for the old bugs.php.net bug IDs. For GitHub we use "GH-XXXXX". So in this case you should change Bug #10807 to GH-10807.
Similarly, you should name your file gh10807 instead of bug10807.

Changes made. Might I suggest that the docs at https://qa.php.net/write-test.php are updated to handle this.

Re rebasing to the 8.1 branch, my fork doesn't have this branch and this has exceeded my github knowledge. Are you able to take this over for now?

iluuu1994 · 2023-03-08T23:33:14Z

Requesting a review from George because this was changed recently. (499fbcd)

ranvis · 2023-03-08T23:43:23Z

FWIW, there's another loop of the same kind and the fix is effectively reverting a commit made by @cmb69:
b510250

bgardner-noggin · 2023-03-08T23:50:32Z

Note that until I added a validateId function to my custom session handler, I could not get session_regenerate_id to work at all

ie the hidden validateId function caused me a world of pain until I happened to discover the session.use_strict_mode + custom validateId function

bgardner-noggin · 2023-03-08T23:51:55Z

Additionally, the fix by @cmb69 seems incorrect to be in that that loop will always call create_sid, ignore the validateId response and just use the last value from create_sid anyway

bgardner-noggin · 2023-03-08T23:53:31Z

However, I can look at adding a test case for invoking create_session_id during an active session as well

…ailing and non failing validation

bgardner-noggin · 2023-03-09T00:53:30Z

I added a test case for session_create_id for the failing and non failing validation during an active session.

nielsdos · 2023-03-09T07:17:46Z

I wonder if I misinterpreted the documentation. From: https://www.php.net/manual/en/sessionupdatetimestamphandlerinterface.validateid.php

Validates a given session ID. A session ID is valid, if a session with that ID already exists.

So the old code might've actually been correct. We're supposed to return false to indicate that no session already exists.
But this seems to be the opposite of what the name implies, hence my confusion.
The first comment on that doc page is then also wrong I think.
I'm not sure anymore what the right semantics here are to be honest...

Girgias · 2023-03-09T07:24:08Z

Oh god, this part of ext/session again... let me try to recall what the semantics are because they are very weird and unintuitive.

Girgias

I'm going to blame our docs and the stupid naming of this method for the mistake.

However, the change and the bug don't seem to be valid due to the misconception of what this method is meant to do.

This method must not be used to check the "shape" of an ID or if it satisfies what ever arbitrary requirements, those need to be done in create_sid() and throw an Exception if it doesn't (or just regenerate one that does). The only purpose of the validateID() method is to check if the ID given exists within the data storage. A possible better name would have been validateDataStorageExistenceOfId() or isIdInUseInDataStorage() (or even some other more descriptive name) to prevent this misconception.

Girgias · 2023-03-09T07:50:08Z

ext/session/session.c

@@ -2295,7 +2295,7 @@ PHP_FUNCTION(session_regenerate_id)
 		if ((!PS(mod_user_implemented) && PS(mod)->s_validate_sid) || !Z_ISUNDEF(PS(mod_user_names).ps_validate_sid)) {
 			int limit = 3;
 			/* Try to generate non-existing ID */
-			while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {
+			while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE) {


This change is definitely incorrect.

I know the method name is terrible, but validateId() MUST only return true if a session with this ID already exists.

Symfony seems to have hit that mistake too and fixed it: symfony/symfony#47843

Girgias · 2023-03-09T07:51:20Z

ext/session/session.c

@@ -2362,7 +2362,7 @@ PHP_FUNCTION(session_create_id)
 				break;
 			} else {
 				/* Detect collision and retry */
-				if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == SUCCESS) {
+				if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == FAILURE) {


Similarly, this change is also incorrect. As this allows you to create two sessions with the same ID, which is a massive issue.

Girgias · 2023-03-09T08:08:29Z

ext/session/tests/gh10807.phpt

+	public function validateId(string $id) : bool {
+
+		if ($this->fail_validation > 0 && intval($id)< $this->fail_validation) {
+			return FALSE;
+		}
+
+		return TRUE;
+
+	}


The logic of this is nonsensical.

Validations about the "shape" of the ID must happen in the create_sid() method.

The purpose of the validateId() method is only to validate that the given ID exists in the data storage.

Please read the Limitation and Compatibility sections of the RFC that introduced those features, which describes the behaviour of this method: https://wiki.php.net/rfc/strict_sessions#limitation

The rationale as to why uninitialized sessions should be discarded is explained in the RFC here

Yes, my validateId contents above are not a real world scenario, it is there to confirm that my change did not break the scenario where validateId returns false

nielsdos · 2023-03-09T08:20:18Z

Sorry about the confusion @bgardner-noggin, we should probably clarify the docs indeed... And thank you @Girgias for digging into this.

Girgias · 2023-03-09T08:24:29Z

Sorry about the confusion @bgardner-noggin, we should probably clarify the docs indeed... And thank you @Girgias for digging into this.

No worries, I think the fact that this is confusing is something to be concerned about, especially as this is related to a security feature. Might dedicate some time to actually propose an RFC fixing the whole user session handler stuff...

bgardner-noggin · 2023-03-09T08:35:39Z

This still seems broken. It seems you have confused my test case above which was ensuring that if validateId returns FALSE, that create_sid is reinvoked. My actual bug is that validateId is returning TRUE, ie the session token is valid, but PHP internals don't care and calls create_sid + validateSid multiple times

In my real world code, after a user successfully authenticates, I am invoking session_regenerate_id to prevent session fixation attacks. This leads to my original scenario

create_sid() <-- returns a new securely random generated token and inserts into the database
validateId($sid) <-this invocation returns TRUE as the session id is valid and is in the database
create_sid() + validateId($sid) <-- repeat 3 times
...

surely create_sid should only be reinvoked if validateId returns false?

Girgias · 2023-03-09T09:41:29Z

This still seems broken. It seems you have confused my test case above which was ensuring that if validateId returns FALSE, that create_sid is reinvoked. My actual bug is that validateId is returning TRUE, ie the session token is valid, but PHP internals don't care and calls create_sid + validateSid multiple times

In my real world code, after a user successfully authenticates, I am invoking session_regenerate_id to prevent session fixation attacks. This leads to my original scenario

create_sid() <-- returns a new securely random generated token and inserts into the database validateId($sid) <-this invocation returns TRUE as the session id is valid and is in the database create_sid() + validateId($sid) <-- repeat 3 times ...

surely create_sid should only be reinvoked if validateId returns false?

So I've continued to stare at the code, trying to dig through this mess and understand it.

I still believe your changed are incorrect because the test for bug 79413 is broken (as seen by the total failure of CI).

The issue you are having is with the collision detections, because your implementation of create_sid() actually creates the database entry. Looking at the implementations of the built-in modules (namely the file one) they only generate the session ID, they do not initialize it. The initialization happens in the read() method.

That's why the validate method fails for them, as the ID is not yet in a created state.

So... according to the current implementation, create_sid() is not meant to actually create the session, just generate a session ID, and read() is meant to take care of initialising a non-existent session. Not at all confusing semantics here...

Now the question is should session_regenerate_id()/session_create_id() try to do collision detection. Because, looking at the other built-in implementations again, they do their collision detections during the create_sid().

bgardner-noggin · 2023-03-09T09:50:49Z

I didn't see the 79413 failure during my make test run. I'll take a look at this tomorrow.

What you say above make sense, except it is different to what you said previously where you stated

The purpose of the validateId() method is only to validate that the given ID exists in the data storage.

As well as what the docs of validateId state

A session ID is valid, if a session with that ID already exists

Now if validateId is actually meant to be

Return True if the id is generated by us, but has no associated storage yet

then it makes sense, but the naming of the function and docs make me think otherwise.

bgardner-noggin · 2023-03-09T10:15:29Z

Return True if the id is generated by us, but has no associated storage yet

This statement cannot be true though, because validateId is invoked during the course of a normal start_session, ie not creating a new session id or regenerating an id, but through the normal course of using an existing session id which is in storage already.

Girgias · 2023-03-09T12:47:41Z

I didn't see the 79413 failure during my make test run. I'll take a look at this tomorrow.

What you say above make sense, except it is different to what you said previously where you stated

The purpose of the validateId() method is only to validate that the given ID exists in the data storage.

I don't see how what I said contradicts my other comments. That's still it's only purpose.

As well as what the docs of validateId state

A session ID is valid, if a session with that ID already exists

Now if validateId is actually meant to be

Return True if the id is generated by us, but has no associated storage yet

then it makes sense, but the naming of the function and docs make me think otherwise.

Naming and docs are completely irrelevant in PHP.
The actual implementation is the source of truth, for better or for worse (mostly worse). And the docs need to reflect the actual implementation. Moreover, there has been little to no change in the implementation of that aspect since its introduction in 2016, so I'm just here trying to figure out what on earth is happening.

However, I never implied that validateId() is meant to mean:

Return True if the id is generated by us, but has no associated storage yet

I said the expectation is that create_sid() is to only generate the session ID without creating an entry in the storage mechanism. As you correctly pointed out that validateId() is called during session initialization.

However, I don't think the expectation that create_sid() should not create an entry in the storage mechanism is sensible and even less intuitive than the validateId() method. As if we follow the current design, it means that read() needs to take into account creation of entries. So I'm not sure that the current implementation is sensible trying to perform that ID collision check as this is the source of a bunch of these issues.

Just a side question, but it seems you've basically rolled out a custom session mechanism already. Why rely on ext/session and on $_SESSION?

bgardner-noggin · 2023-03-09T21:15:39Z

So if I understand this correctly if I have a custom save handler

session_regenerate_id will invoke create_sid
create_sid should generate an id, but not insert it
validateId will be invoked, which will return false as the id is not in storage
read(id) should then insert the record

As to why I was using the session extension, this is because it provides other useful things, like managing the session cookie

However, the docs are not clear and I think i will end up using a completely custom model for this and other reasons, which is disappointing.

Eg another problem I can see from looking at the source code is that if I have a custom save handler that implements the SessionIDInterface, then I would expect session_create_id() would always call my custom handler, yet I can see that this is not the case when the session is not active and it will fallback to the internal PHP function

https://github.com/php/php-src/blob/master/ext/session/session.c#L2357

if (!PS(in_save_handler) && PS(session_status) == php_session_active) {
...
} else {
new_id = php_session_create_id(NULL);
}

Girgias · 2023-03-10T14:04:36Z

So if I understand this correctly if I have a custom save handler

1. session_regenerate_id will invoke create_sid

2. create_sid should generate an id, but not insert it

3. validateId will be invoked, which will return false as the id is not in storage

4. read(id) should then insert the record

This is my understanding, yes. But I do think the semantics of 2 are bad and this might need some discussion on the Internals Mailing list as this behaviour is unintuitive.

As to why I was using the session extension, this is because it provides other useful things, like managing the session cookie

However, the docs are not clear and I think i will end up using a completely custom model for this and other reasons, which is disappointing.

Right, thanks for the explanation. Yeah, the docs are in a sad state of affairs for that section.

Eg another problem I can see from looking at the source code is that if I have a custom save handler that implements the SessionIDInterface, then I would expect session_create_id() would always call my custom handler, yet I can see that this is not the case when the session is not active and it will fallback to the internal PHP function

https://github.com/php/php-src/blob/master/ext/session/session.c#L2357

if (!PS(in_save_handler) && PS(session_status) == php_session_active) {
...
} else {
new_id = php_session_create_id(NULL);
}

Yeah, this does look problematic indeed...

Thanks for your time anyway.

BUG-10807: session_regenerate_id fails to check results of validateId…

790e3fe

… correctly

github-actions bot added the Extension: session label Mar 8, 2023

bgardner-noggin mentioned this pull request Mar 8, 2023

session_regenerate_id with custom handler and use_strict_mode generates three new session ids #10807

Open

nielsdos requested changes Mar 8, 2023

View reviewed changes

iluuu1994 requested a review from Girgias March 8, 2023 23:33

Renamed files as per request

281ec2d

Added further test for session_create_id during active session with f…

d4a8a02

…ailing and non failing validation

Girgias self-assigned this Mar 9, 2023

Girgias linked an issue Mar 9, 2023 that may be closed by this pull request

session_regenerate_id with custom handler and use_strict_mode generates three new session ids #10807

Open

Girgias requested changes Mar 9, 2023

View reviewed changes

		@@ -0,0 +1,69 @@
		--TEST--
		Bug #10807 (session_regenerate_id with custom handler and use_strict_mode generates three new session ids)

BUG-10807: session_regenerate_id fails to check results of validateId… #10813

Are you sure you want to change the base?

BUG-10807: session_regenerate_id fails to check results of validateId… #10813

Uh oh!

Conversation

bgardner-noggin commented Mar 8, 2023

Uh oh!

bgardner-noggin commented Mar 8, 2023

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

bgardner-noggin Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranvis commented Mar 8, 2023

Uh oh!

bgardner-noggin commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgardner-noggin commented Mar 8, 2023

Uh oh!

bgardner-noggin commented Mar 8, 2023

Uh oh!

bgardner-noggin commented Mar 9, 2023

Uh oh!

nielsdos commented Mar 9, 2023

Uh oh!

Girgias commented Mar 9, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

bgardner-noggin Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Mar 9, 2023

Uh oh!

Girgias commented Mar 9, 2023

Uh oh!

bgardner-noggin commented Mar 9, 2023

Uh oh!

Girgias commented Mar 9, 2023

Uh oh!

bgardner-noggin commented Mar 9, 2023

Uh oh!

bgardner-noggin commented Mar 9, 2023

Uh oh!

Girgias commented Mar 9, 2023

Uh oh!

bgardner-noggin commented Mar 9, 2023

Uh oh!

Girgias commented Mar 10, 2023

Uh oh!

Uh oh!

iluuu1994 commented Mar 8, 2023 •

edited

Loading

bgardner-noggin commented Mar 8, 2023 •

edited

Loading