Hi Joe,
On Thu, Mar 24, 2016 at 4:22 PM, Joe Watkins <pthreads@pthreads.org> wrote:
>
> First, thanks for all the effort ... You already know that, a refusal to
> merge a patch doesn't mean you wasted your time, it should serve to refocus
> your research.
>
> So, I thought I'd say some stuff, to aid with that ...
>
> The first thing I would do is break down the problem into smaller, much
> smaller, RFC's: We know there are many inconsistencies in the language, we
> could write a huge long list, and do a bumper RFC to solve them all at once.
> But would that be a good idea !?
Splitting patch is not a problem. Some changes are interrelated, but
many could be applied individually.
>
> As already mentioned it's much easier to consider each problem on it's
> own, and the solution for that problem on it's own, than it is to consider
> what we are going to do about this huge mess. Try to remember that while you
> have had your head buried in this stuff for a long time, the rest of us
> likely haven't.
>
> I don't really like to talk about security, it's not a thing I feel
> qualified to talk about ... but, there is inconsistency in saying "we should
> have a secure by default session module", and then introducing ini settings
> that can vary the effectiveness of the solution, based on guesses about the
> length, and speed at which elevators travel ...
>
> Having another setting which seems to be 18 times longer than the
> recommended value for "low security applications" is also extremely odd to
> me ...
>
> That you can't find sensible defaults for these values, says to me that
> these problems don't have solutions in our domain; Application security is a
> problem that can only be solved by the programmer using PHP.
>
> We should provide the tools, we should not lie about the validity of a
> session because of faults in our design, every layer we provide should be
> consistent, but the problem still belongs to the programmer using PHP.
I agree. It's like using TCP or UDP. I don't mind much either way, but
I prefer TCP like behavior for session manager because session manager
jobs is keeping reliable session.
Current PHP session is UDP, users are responsible use it correctly. If
we keep UDP way, I think we should remove session_regenerate_id(TURE)
at least. i.e. Remove deletion option from session_regenerate_id()
because it doesn't work and bad way to manage session ID.
I'll write PHP code for those who are care about session reliability
and security. Following code is minimum code. I wrote this code
directly, so take this as pseudo code. I might be missing something as
I do not verify this code, but it should be good enough to know what
users must do.
Using UDP like TCP is not too difficult, but not too simple also.
Session management is the same.
<?php
ini_set('session.use_strict_mode', 1);
session_start();
if (isset($_SESSION['deleted']) && $_SESSION['deleted'] < time() - 600) {
trigger_error('Possible attack detected');
session_unset();
session_destroy();
session_start();
session_regenerate_id();
}
// if you cannot loose session on occasion, execute following if block also.
if (isset($_SESSION['deleted']) && $_SESSION['deleted'] < time() - 60
&& isset($_SESSION['new_id'])) {
$new_id = $_SESSION['new_id'];
unset($_SESSION['new_id']);
session_commit();
session_id($new_id);
session_start();
}
if (!isset($_SESSION['update'] ||
(!isset($_SESSION['deleted']) && $_SESSION['updated'] < time() - 180)) {
// Should not update updated timestamp always
$_SESSION['updated'] = time();
}
if (empty($_SESSION['regenerated']) {
// New session
$_SESSION['regenerated'] = time();
}
if ($_SESSION['regenerated'] < time() - 1800) {
// This code does not keep older session IDs. If you need them, save
them here also.
$_SESSION['deleted'] = time();
session_commit(); // Older PHP must close session to write session
data. PHP 7.0 and up does not need this.
session_start(); // Since session is closed, session has to be
started. PHP 7.0 and up does not need this.
$old_id = session_id();
session_regenerate_id();
$new_id = session_id();
$_SESSION['regenerated'] = time();
unset($_SESSION['deleted']);
// if you cannot loose session on occasion, execute following code also.
session_commit();
session_id($old_session);
session_start();
$_SESSION['new_id'] = $new_id;
session_commit();
session_id($new_id);
session_start();
}
?>
This is not equal to what the proposed patch does. This is the least
requirement for reliable session management. It's much less efficient
in user script. It's cost of user space implementation.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
