Block premature access to session storage

[znerol]

Description

• session_start() unconditionally calls SessionHandler::read() - even in cases where the session id has been newly generated just a couple of lines earlier.
• session_write_close() unconditionally calls SessionHandler::write() even if the session id has been newly generated for the current request and the session is empty.
• session_regenerate_id(true) unconditionally calls SessionHandler::destroy() even if the session id has been newly generated for the current request.

I think there is some room for optimization if the session module could suppress calls into the session handler in certain situations. I guess its best expressed as phpt what I'm having in mind.

---

Incoming requests without a session (cookie) shouldn't be accessing storage at all:

--TEST--
Request without session
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return false;
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

session_start();
session_write_close();
?>
--EXPECT--
OPEN: PHPSESSID
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-without-session.phpt]
========DIFF========
     OPEN: PHPSESSID
002+ READ: 78d7fe4a7170fb5a60934db9784d7d63
003+ WRITE: 78d7fe4a7170fb5a60934db9784d7d63,
     CLOSE
========DONE========

---

When logging in the initial empty session record shouldn't attempted to be destroyed. Also there is no point in attempting to read a session record with the newly generated id:

--TEST--
Request with login
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "user_id|i:42;";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return false;
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

session_start();
session_regenerate_id(TRUE);
$_SESSION['user_id']=42;
session_write_close();
?>
--EXPECTF--
OPEN: PHPSESSID
CLOSE
OPEN: PHPSESSID
VALIDATE_ID: %s
WRITE: %s %s
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-with-login.phpt]
========DIFF========
     OPEN: PHPSESSID
002+ READ: 9f2306bd1721271b20061dfe45d044a1
003+ DESTROY: 9f2306bd1721271b20061dfe45d044a1
     CLOSE
     OPEN: PHPSESSID
     VALIDATE_ID: a7efc5fb4eca6fd6e44799a6e50e7960
007+ READ: a7efc5fb4eca6fd6e44799a6e50e7960
     WRITE: %s %s
     CLOSE
========DONE========

---

When logging out the resulting empty session record shouldn't attempted to be read and written:

--TEST--
Request with logout
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "user_id|i:42;";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return $GLOBALS['validId'];
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

$GLOBALS['validId'] = true;
session_id(session_create_id());
session_start();
$_SESSION = [];
$GLOBALS['validId'] = false;
session_regenerate_id(TRUE);
session_write_close();
?>
--EXPECTF--
OPEN: PHPSESSID
VALIDATE_ID: %s
READ: %s
DESTROY: %s
CLOSE
OPEN: PHPSESSID
VALIDATE_ID: %s
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-with-logout.phpt]
========DIFF========
--
     CLOSE
     OPEN: PHPSESSID
     VALIDATE_ID: %s
008+ READ: 483617bb2349fe2345fcb67b391e7b1d
009+ WRITE: 483617bb2349fe2345fcb67b391e7b1d,
     CLOSE
========DONE========

[andypost]

Thank you raising it. +1 a bit more context from Drupal https://www.drupal.org/project/drupal/issues/3570824#comment-16451703

[iluuu1994]

/cc @Girgias

[Girgias]

So I've been thinking about this and looking at the implementation of ext/session. And I don't think this is very achievable in its totality.
PHP doesn't have "context" of when or even if data is read/written to the $_SESSION superglobal.

It should be possible to remove some reads, but I don't think it's possible to remove the writes.

You seem to also misunderstand how the session handler works.

Incoming requests without a session (cookie) shouldn't be accessing storage at all

The session handler will create a new session ID if you start a session and no ID exists or the ID is not valid (when using strict mode). And thus at any point data can be written to the superglobal and thus a write must actually be performed.

I will need to investigate further if the reads can actually be removed, because the code comments seem to imply session modules might expect a read.

Edit: this behaviour is even indicated in the docs for Custom Session Handlers. So this might even need an RFC if we want to change behaviour... which I'm not sure I fully want to engage in. Happy to try and experiment but I would want to partner with someone to write the RFC to properly present the advantages of the new behaviour.

[znerol]

Thanks for the response. It is quite likely that I do not have the full picture here.

At this stage I'm less interested in whether this is achievable but more in whether this is generally desirable.

It looks like that mechanism could be implemented on the application level with the following session proxy. In this case the create_sid() method is intercepted solely for the purpose of recording the newly created session id. So that this can be used to decide whether to allow or bock read, write and destroy operations.

<?php

/**
 * Implements a session handler proxy which blocks premature access to storage.
 *
 * Short circuits any read, write and destroy operation for newly created
 * session identifiers.
 */
class OptimizedStorageProxy implements \SessionHandlerInterface, \SessionIdInterface, \SessionUpdateTimestampHandlerInterface {

  private ?string $newSessionId = NULL;

  public function __construct(
    private \SessionHandlerInterface&\SessionUpdateTimestampHandlerInterface $handler
  ) {
  }

  public function open(string $savePath, string $sessionName): bool {
    $this->newSessionId = NULL;
    return $this->handler->open($savePath, $sessionName);
  }

  public function create_sid(): string {
    $this->newSessionId = session_create_id();
    return $this->newSessionId;
  }

  public function validateId(string $id): bool {
    return $this->handler->validateId($id);
  }

  public function read(string $sessionId): string|false {
    return $sessionId === $this->newSessionId ? '' : $this->handler->read($sessionId);
  }

  public function updateTimestamp(string $id, string $data): bool {
    return $this->handler->updateTimestamp($id, $data);
  }

  public function write(string $sessionId, string $data): bool {
    if ($sessionId === $this->newSessionId && $data === '') {
      return TRUE;
    }
    $this->newSessionId = NULL;
    return $this->handler->write($sessionId, $data);
  }

  public function destroy(string $sessionId): bool {
    return $sessionId === $this->newSessionId ? TRUE : $this->handler->destroy($sessionId);
  }

  public function close(): bool {
    $this->newSessionId = NULL;
    return $this->handler->close();
  }

  public function gc(int $max_lifetime): int|false {
    return $this->handler->gc($max_lifetime);
  }

}

Maybe let's turn the question around: What is the edge case where avoiding session storage for anonymous requests (no initial session id and no session data involved) would break existing applications?

[Girgias]

I'm not sure if there is an edge case that I can think of. But enough experience on working on PHP tells me that Hyrum's Law is going to apply here...

But my other question digging a bit more is: are the above PHPT tests assuming that session.lazy_write is enabled or not? Because that might be a way to ignore writes if the initial state is empty.

[znerol]

It looks like static zend_string *php_session_encode(void) returns NULL if the session is empty. At least the PHP equivalent session_decode() returns false for empty sessions, and it only does that when php_session_encode returns NULL.

https://3v4l.org/dnWt6#v8.3.30

If that is true, then the else branch is taken directly and the check for lazy_write is skipped in static void php_session_save_current_state(bool write):

php-src/ext/session/session.c

Lines 525 to 540 in 52e9436

	val = php_session_encode();
	if (val) {
	if (PS(lazy_write) && PS(session_vars)
	&& PS(mod)->s_update_timestamp
	&& PS(mod)->s_update_timestamp != php_session_update_timestamp
	&& zend_string_equals(val, PS(session_vars))
	) {
	ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
	handler_function_name = handler_class_name != NULL ? "updateTimestamp" : "update_timestamp";
	} else {
	ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
	}
	zend_string_release_ex(val, false);
	} else {
	ret = PS(mod)->s_write(&PS(mod_data), PS(id), ZSTR_EMPTY_ALLOC(), PS(gc_maxlifetime));
	}

[thg2k]

This is very interesting. I recently did a walkthru on the session extension code and I have to say there is a bit of confusion about who is responsible for what, like collision avoidance, creating empty sessions, etc.

If you want to move forward with an RFC i'd be happy to contribute with some analysis of the current state and the goal state, but keep in mind that the quoted Hyrum's Law is spot on here.

For example, there is no way you can avoid the read after create, because the read is supposed to actually create and lock the session and avoid parallel processing (not completely true if you also have validateId implemented, but this is beyond my current point). At least this is what the files module does. Most custom handlers likely don't, but they should, and for those who do, this would be a gigantic breaking change.

At the moment you are supposed to lock in the read callback and unlock in update/write. The update is opt-in (if you don't define it, write is called instead). The proper way would be to lock in create_sid/read and unlock in update/write, but this would require a new method to opt-in the new behavior, i.e. real_create_sid() that when present will be invoked with the assumption the session was actually created and locked. This would make the orchestrator even more complicated and hard to test.