14 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

Few years ago, I have proposed strict session.
It seems PHP 5.4 and php-src don't have protection against session
adoption yet.

Since there will be many TLDs, session adoption attack will be
very easy for some domains until browsers support them.
Even without new TLDs, attacker may place cookie file to attack
session adoption or can exploit paths or domains, since there is no
standard for precedence.

e.g. Domain has more precedence over path on Chrome while
path has greater precedence on IE. This is due to the order difference
of sent cookies. (If there are multiple cookies are set for domain/path, only
one became the outstanding cookie. I think PHP uses first IIRC while other
implementation may use the last. Therefore, browser may not able to
solve this issue, since it may destroy apps specific to browser)

Even if a programmer sets path and domain for PHP session id cookie,
attackers may exploit this to fix session id with session managers that are
vulnerable to session adoption.

If you don't get idea, play with cookie with/without domain/path is set/unset.
You'll see how attacker may use session adoption. Default session module's
configuration (domain not set, path set to /) is very easy to exploit anyway.

I usually set both exact application domain/path, and unset all domain/path
cookies for session id to prevent the attack. I guess this is not
widely adopted.
Even this is not enough. For example, if subdomain is added, Chrome has
greater precedence for subdomain and attacker may exploit it.

I pasted a patch for PHP 5.2 that rejects uninitialized session id. I
think original
patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to
current PHP. If one would like to old behavior, he/she can just turn off the
strict session.

There are too many ways to exploit session with session adoption vulnerable
session manger. Simple solution is making session manager strict.

Any comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Index: ext/session/php_session.h

--- ext/session/php_session.h (リビジョン 288932)
+++ ext/session/php_session.h (作業コピー)
@@ -23,7 +23,7 @@

#include "ext/standard/php_var.h"

-#define PHP_SESSION_API 20020330
+#define PHP_SESSION_API 20051121

#define PS_OPEN_ARGS void **mod_data, const char *save_path, const
char *session_name TSRMLS_DC
#define PS_CLOSE_ARGS void **mod_data TSRMLS_DC
@@ -32,6 +32,7 @@
#define PS_DESTROY_ARGS void **mod_data, const char *key TSRMLS_DC
#define PS_GC_ARGS void **mod_data, int maxlifetime, int *nrdels TSRMLS_DC
#define PS_CREATE_SID_ARGS void **mod_data, int *newlen TSRMLS_DC
+#define PS_VALIDATE_SID_ARGS void **mod_data, const char *key TSRMLS_DC

/* default create id function */
PHPAPI char *php_session_create_id(PS_CREATE_SID_ARGS);
@@ -45,6 +46,7 @@
int (*s_destroy)(PS_DESTROY_ARGS);
int (*s_gc)(PS_GC_ARGS);
char *(*s_create_sid)(PS_CREATE_SID_ARGS);

   int (*s_validate_sid)(PS_VALIDATE_SID_ARGS);

} ps_module;

#define PS_GET_MOD_DATA() *mod_data
@@ -57,6 +59,7 @@
#define PS_DESTROY_FUNC(x) int ps_delete_##x(PS_DESTROY_ARGS)
#define PS_GC_FUNC(x) int ps_gc_##x(PS_GC_ARGS)
#define PS_CREATE_SID_FUNC(x) char *ps_create_sid_##x(PS_CREATE_SID_ARGS)
+#define PS_VALIDATE_SID_FUNC(x) int
ps_validate_sid_##x(PS_VALIDATE_SID_ARGS)

#define PS_FUNCS(x)
PS_OPEN_FUNC(x);
@@ -65,11 +68,12 @@
PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

```
  PS_CREATE_SID_FUNC(x)
```

```
  PS_CREATE_SID_FUNC(x); \
```
PS_VALIDATE_SID_FUNC(x)

#define PS_MOD(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \

   ps_delete_##x, ps_gc_##x, php_session_create_id

   ps_delete_##x, ps_gc_##x, php_session_create_id, ps_validate_sid_##x

/* SID enabled module handler definitions */
#define PS_FUNCS_SID(x)
@@ -79,11 +83,12 @@
PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

```
  PS_CREATE_SID_FUNC(x)
```

```
  PS_CREATE_SID_FUNC(x); \
```
PS_VALIDATE_SID(x)

#define PS_MOD_SID(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \

   ps_delete_##x, ps_gc_##x, ps_create_sid_##x

   ps_delete_##x, ps_gc_##x, ps_create_sid_##x, ps_validate_sid_##x

typedef enum {
php_session_disabled,
@@ -121,6 +126,7 @@
zend_bool use_only_cookies;
zend_bool use_trans_sid; /* contains the INI value of
whether to use
trans-sid /
zend_bool apply_trans_sid; / whether or not to enable trans-sid for
the current request */

   zend_bool use_strict_mode;      /* whether or not PHP accepts

unknown session ids */

   long hash_func;
   long hash_bits_per_character;

Index: ext/session/tests/session_commit_variation4.phpt

--- ext/session/tests/session_commit_variation4.phpt (リビジョン 288932)
+++ ext/session/tests/session_commit_variation4.phpt (作業コピー)
@@ -1,10 +1,11 @@
--TEST--
Test `session_commit()` function : variation
--SKIPIF--
-<?php include('skipif.inc'); ?>
+<?php
+die('skip'); // don't work with strict session patch
+?>
--FILE--
<?php

ob_start();

/*
Index: ext/session/tests/session_id_basic.phpt

--- ext/session/tests/session_id_basic.phpt (リビジョン 288932)
+++ ext/session/tests/session_id_basic.phpt (作業コピー)
@@ -1,10 +1,11 @@
--TEST--
Test `session_id()` function : basic functionality
--SKIPIF--
-<?php include('skipif.inc'); ?>
+<?php
+die('skip'); // does not work with strict session
+?>
--FILE--
<?php

ob_start();

/*
Index: ext/session/tests/session_write_close_variation4.phpt

--- ext/session/tests/session_write_close_variation4.phpt (リビジョン 288932)
+++ ext/session/tests/session_write_close_variation4.phpt (作業コピー)
@@ -1,10 +1,11 @@
--TEST--
Test `session_write_close()` function : variation
--SKIPIF--
-<?php include('skipif.inc'); ?>
+<?php
+die('skip'); // does not work with strict session
+?>
--FILE--
<?php

ob_start();

/*
Index: ext/session/mod_files.c

--- ext/session/mod_files.c (リビジョン 288932)
+++ ext/session/mod_files.c (作業コピー)
@@ -464,6 +464,35 @@
return SUCCESS;
}

+PS_VALIDATE_SID_FUNC(files)
+{

```
  char buf[MAXPATHLEN];
```
```
  int fd;
```
```
  PS_FILES_DATA;
```
```
  if (!ps_files_valid_key(key)) {
```
```
           return FAILURE;
```
```
   }
```
```
   if (!PS(use_strict_mode)) {
```
```
           return SUCCESS;
```
```
   }
```

   if (!ps_files_path_create(buf, sizeof(buf), data, key)) {

```
          return FAILURE;
```
```
   }
```

   fd = VCWD_OPEN_MODE(buf, O_RDWR | O_BINARY,

                          data->filemode);

```
   if (fd != -1) {
```
```
           close(fd);
```
```
           return SUCCESS;
```
```
   }
```
```
  return FAILURE;
```

Local variables:
tab-width: 4
Index: ext/session/mod_user.h
===================================================================
--- ext/session/mod_user.h (リビジョン 288932)
+++ ext/session/mod_user.h (作業コピー)
@@ -22,7 +22,7 @@
#define MOD_USER_H

typedef union {

```
  zval *names[6];
```

  zval *names[8];
 struct {
         zval *ps_open;
         zval *ps_close;

@@ -30,6 +30,8 @@
zval *ps_write;
zval *ps_destroy;
zval *ps_gc;

```
           zval *ps_create;
```
```
           zval *ps_validate;
 } name;
```

} ps_user;

Index: ext/session/session.c

--- ext/session/session.c (リビジョン 288932)
+++ ext/session/session.c (作業コピー)
@@ -462,6 +462,15 @@
return;
}

/* If there is an ID, use session module to verify it */
if (PS(id)) {

   if (PS(mod)->s_validate_sid(&PS(mod_data), PS(id) TSRMLS_CC)

== FAILURE) {

```
       efree(PS(id));
```
```
       PS(id) = NULL;
```
```
       PS(send_cookie) = 1;
```
```
   }
```
}

 /* If there is no ID, use session module to create one */
 if (!PS(id)) {

new_session:
@@ -699,6 +708,7 @@
STD_PHP_INI_BOOLEAN("session.cookie_httponly", "",
PHP_INI_ALL, OnUpdateBool, cookie_httponly, php_ps_globals,
ps_globals)
STD_PHP_INI_BOOLEAN("session.use_cookies", "1",
PHP_INI_ALL, OnUpdateBool, use_cookies, php_ps_globals,
ps_globals)
STD_PHP_INI_BOOLEAN("session.use_only_cookies", "0",
PHP_INI_ALL, OnUpdateBool, use_only_cookies, php_ps_globals,
ps_globals)

  STD_PHP_INI_BOOLEAN("session.use_strict_mode",  "1",

PHP_INI_ALL, OnUpdateBool, use_strict_mode, php_ps_globals,
ps_globals)
STD_PHP_INI_ENTRY("session.referer_check", "",
PHP_INI_ALL, OnUpdateString, extern_referer_chk, php_ps_globals,
ps_globals)
STD_PHP_INI_ENTRY("session.entropy_file", "",
PHP_INI_ALL, OnUpdateString, entropy_file, php_ps_globals,
ps_globals)
STD_PHP_INI_ENTRY("session.entropy_length", "0",
PHP_INI_ALL, OnUpdateLong, entropy_length, php_ps_globals,
ps_globals)
@@ -1535,12 +1545,12 @@
}
/* }}} */

-/* {{{ proto void session_set_save_handler(string open, string close,
string read, string write, string destroy, string gc)
+/* /* {{{ proto void session_set_save_handler(string open, string
close, string read, string write, string destroy, string gc[, string
create, string validate])
Sets user-level functions */
static PHP_FUNCTION(session_set_save_handler)
{

```
  zval **args[6];
```
```
  int i;
```

```
  zval **args[8];
```

  int i, numargs;
 ps_user *mdata;
 char *name;

@@ -1548,10 +1558,20 @@
RETURN_FALSE;
}

  if (ZEND_NUM_ARGS() != 6 || zend_get_parameters_array_ex(6,

args) == FAILURE)

```
          WRONG_PARAM_COUNT;
```

numargs = ZEND_NUM_ARGS();
args[6] = NULL;
args[7] = NULL;
if (numargs < 6 || numargs > 8 ||
zend_get_parameters_array_ex(numargs, args) == FAILURE)
```
   WRONG_PARAM_COUNT;
```

```
  for (i = 0; i < 6; i++) {
```

if (PS(session_status) != php_session_none)
```
   RETURN_FALSE;
```
for (i = 0; i < 8; i++) {

   if (i >= 6 && (args[i] == `NULL` || ZVAL_IS_NULL(*args[i]))) {

```
       continue;
```

   }
         if (!zend_is_callable(*args[i], 0, &name)) {
                 php_error_docref(NULL TSRMLS_CC, E_WARNING,

"Argument %d is not a
valid callback", i+1);
efree(name);
@@ -1572,12 +1592,15 @@
}

   mdata = emalloc(sizeof(*mdata));

```
  for (i = 0; i < 6; i++) {
```

```
  for (i = 0; i < 8; i++) {
```

           if (i >= 6 && (args[i] == `NULL` || ZVAL_IS_NULL(*args[i]))) {

                   mdata->names[i] = NULL;

```
                   continue;
```

           }
         ZVAL_ADDREF(*args[i]);
         mdata->names[i] = *args[i];
 }

 PS(mod_data) = (void *) mdata;

 RETURN_TRUE;

Index: ext/session/mod_mm.c

--- ext/session/mod_mm.c (リビジョン 288932)
+++ ext/session/mod_mm.c (作業コピー)
@@ -445,6 +445,42 @@
return SUCCESS;
}

+PS_VALIDATE_SID_FUNC(mm)
+{

```
  PS_MM_DATA;
```
```
  ps_sd *sd;
```
```
  const char *p;
```
```
  char c;
```
```
  int ret = SUCCESS;
```
```
  for (p = key; (c = *p); p++) {
```

          /* valid characters are a..z,A..Z,0..9 */

```
          if (!((c >= 'a' && c <= 'z')
```
```
         || (c >= 'A' && c <= 'Z')
```
```
         || (c >= '0' && c <= '9')
```
```
         || c == ','
```
```
         || c == '-')) {
```
```
                  return FAILURE;
```
```
          }
```
```
  }
```
if (!PS(use_strict_mode)) {
```
   return SUCCESS;
```
}
```
  mm_lock(data->mm, MM_LOCK_RD);
```
```
  sd = ps_sd_lookup(data, key, 0);
```
```
  if (sd) {
```
```
   mm_unlock(data->mm);
```
```
          return SUCCESS;
```
```
  }
```
```
  mm_unlock(data->mm);
```
```
  return FAILURE;
```

#endif

/*
Index: ext/session/mod_user.c

--- ext/session/mod_user.c (リビジョン 288932)
+++ ext/session/mod_user.c (作業コピー)
@@ -23,7 +23,7 @@
#include "mod_user.h"

ps_module ps_mod_user = {

```
  PS_MOD(user)
```

```
  PS_MOD_SID(user)
```

};

#define SESS_ZVAL_LONG(val, a)
@@ -167,6 +167,83 @@
FINISH;
}

+PS_CREATE_SID_FUNC(user)
+{

```
  int i;
```
```
   char *val = NULL;
```
```
  zval *retval;
```
```
  ps_user *mdata = PS_GET_MOD_DATA();
```
if (!mdata)
```
          return estrndup("", 0);
```
if (PSF(create) == NULL || ZVAL_IS_NULL(PSF(create))) {

   return php_session_create_id(mod_data, newlen TSRMLS_CC);

}

  retval = ps_call_handler(PSF(create), 0, `NULL` TSRMLS_CC);

```
  if (retval) {
```

          if (Z_TYPE_P(retval) == IS_STRING) {

                  val = estrndup(Z_STRVAL_P(retval), Z_STRLEN_P(retval));

```
          } else {
```
```
       val = estrndup("", 0);
```
```
   }
```
```
          zval_ptr_dtor(&retval);
```
```
  } else {
```
```
   val = estrndup("", 0);
```
}
return val;
+}

+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{

```
  size_t len;
```
```
  const char *p;
```
```
  char c;
```
```
  int ret = SUCCESS;
```
```
  for (p = key; (c = *p); p++) {
```

          /* valid characters are a..z,A..Z,0..9 */

```
          if (!((c >= 'a' && c <= 'z')
```
```
         || (c >= 'A' && c <= 'Z')
```
```
         || (c >= '0' && c <= '9')
```
```
         || c == ','
```
```
         || c == '-')) {
```
```
                  ret = FAILURE;
```
```
                  break;
```
```
          }
```
```
  }
```
```
  len = p - key;
```
```
  if (len == 0)
```
```
          ret = FAILURE;
```
```
  return ret;
```

+PS_VALIDATE_SID_FUNC(user)
+{

```
  zval *args[1];
```
```
  STDVARS;
```

   if (PSF(validate) == `NULL` || ZVAL_IS_NULL(PSF(validate))) {

           return ps_user_valid_key(key TSRMLS_CC);

```
   }
```
```
  SESS_ZVAL_STRING(key, args[0]);
```

  retval = ps_call_handler(PSF(validate), 1, args TSRMLS_CC);

```
  if (retval) {
```
```
          convert_to_long(retval);
```

          ret = Z_LVAL_P(retval) ? SUCCESS : FAILURE;

```
          zval_ptr_dtor(&retval);
```
```
  }
```
```
   return ret;
```

Local variables:
tab-width: 4
Index: ext/sqlite/sess_sqlite.c
===================================================================
--- ext/sqlite/sess_sqlite.c (リビジョン 288932)
+++ ext/sqlite/sess_sqlite.c (作業コピー)
@@ -189,6 +189,70 @@
return SQLITE_RETVAL(rv);
}

+PS_VALIDATE_SID_FUNC(sqlite)
+{

```
  PS_SQLITE_DATA;
```
```
  char *query;
```
```
  const char *tail;
```
```
  sqlite_vm *vm;
```
```
  int colcount, result;
```
```
  const char **rowdata, **colnames;
```
```
  char *error;
```
char *p, c;
```
  int ret = FAILURE;
```
```
  for (p = key; (c = *p); p++) {
```

          /* valid characters are a..z,A..Z,0..9 */

```
          if (!((c >= 'a' && c <= 'z')
```
```
         || (c >= 'A' && c <= 'Z')
```
```
         || (c >= '0' && c <= '9')
```
```
         || c == ','
```
```
         || c == '-')) {
```
```
                  return FAILURE;
```
```
          }
```
```
  }
```
if (!PS(use_strict_mode)) {
```
   return SUCCESS;
```
}

  query = sqlite_mprintf("SELECT value FROM session_data WHERE

sess_id='%q' LIMIT 1", key);

```
  if (query == NULL) {
```
```
          /* no memory */
```
```
          return FAILURE;
```
```
  }
```

  if (sqlite_compile(db, query, &tail, &vm, &error) != SQLITE_OK) {

          php_error_docref(NULL TSRMLS_CC, E_WARNING, "SQLite: Could not

compile session validate query: %s", error);

```
          sqlite_freemem(error);
```
```
          sqlite_freemem(query);
```
```
          return FAILURE;
```
```
  }
```

  switch ((result = sqlite_step(vm, &colcount, &rowdata, &colnames))) {

```
          case SQLITE_ROW:
```

                  if (rowdata[0] != NULL) {

```
           ret = SUCCESS;
```
```
                  }
```
```
                  break;
```
```
          default:
```

                  sqlite_freemem(error);

```
                  error = NULL;
```
```
  }
```

  if (SQLITE_OK != sqlite_finalize(vm, &error)) {

          php_error_docref(NULL TSRMLS_CC, E_WARNING, "SQLite: session

validate: error %s", error);

```
          sqlite_freemem(error);
```
```
          error = NULL;
```
```
  }
```
```
  sqlite_freemem(query);
```
```
  return ret;
```

#endif /* HAVE_PHP_SESSION && !defined(COMPILE_DL_SESSION) */

14 years ago by Rui Hirokawa — view source — reply

unread

Hi,

I strongly recommend to submit the Strict session patch for
php-src(HEAD) because the vulnerability of PHP against the session
adoption/fixation attack is annoying issue of the PHP programmers for
many years.

I also suggest to apply this patch for PHP_5_4 after PHP 5.4.0 is
released. For PHP 5.4.1, I suggest that the default value of
session.use_strict_mode should be 0 (Off) to maintain the backward
compatibility.

Rui

Yasuo Ohgaki wrote:

Hi all,

Few years ago, I have proposed strict session.
It seems PHP 5.4 and php-src don't have protection against session
adoption yet.

Since there will be many TLDs, session adoption attack will be
very easy for some domains until browsers support them.
Even without new TLDs, attacker may place cookie file to attack
session adoption or can exploit paths or domains, since there is no
standard for precedence.

e.g. Domain has more precedence over path on Chrome while
path has greater precedence on IE. This is due to the order difference
of sent cookies. (If there are multiple cookies are set for domain/path, only
one became the outstanding cookie. I think PHP uses first IIRC while other
implementation may use the last. Therefore, browser may not able to
solve this issue, since it may destroy apps specific to browser)

Even if a programmer sets path and domain for PHP session id cookie,
attackers may exploit this to fix session id with session managers that are
vulnerable to session adoption.

If you don't get idea, play with cookie with/without domain/path is set/unset.
You'll see how attacker may use session adoption. Default session module's
configuration (domain not set, path set to /) is very easy to exploit anyway.

I usually set both exact application domain/path, and unset all domain/path
cookies for session id to prevent the attack. I guess this is not
widely adopted.
Even this is not enough. For example, if subdomain is added, Chrome has
greater precedence for subdomain and attacker may exploit it.

I pasted a patch for PHP 5.2 that rejects uninitialized session id. I
think original
patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to
current PHP. If one would like to old behavior, he/she can just turn off the
strict session.

There are too many ways to exploit session with session adoption vulnerable
session manger. Simple solution is making session manager strict.

Any comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

I've made the patch for trunk. I checked by using session module
tests using valgrind.

https://gist.github.com/1379668

This patch adds

validate_sid() to ps_modules (Save handlers)
use_strict_session to php.ini (On by default, off for old behavior)
display that save handler supports strict session or not via
phpinfo() (So that user could know behavior)
update PHP_SESSION_API version (So that save handler authors could
write portable code)

Compatibility issues are

save handlers that are currently working should also work with this
patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro.
These ps modules should implement validate_sid(). Modules that are
using PS_MOD/PS_FUNCS are not affected, they just marked as "adaptive"
module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())
NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but
it was the same as PS_MOD()/PS_MOD_FUNCS(). If old ps_module does not
compile, just remove "_SID" from PS_MOD_SID()/PS_MOD_FUNCS_SID().
session ID string is checked so that chars are alphanumeric + ','
'-' when use_strict_session=On.
(mod_file.c checks session ID this way w/o this patch to prevent
problems. Using restrictive session ID string is better for other ps
modules. IMHO)
session read failure is not rescued to eliminate possible infinite
loops. Bundled ps modules were not using this at least.

You will see some tests are failing since they depend on adaptive
session. By looking into failing test results, you can see this patch
is generating new session ID if it's not a already initialized
session. I'll modify these tests (set use_strict_session=Off) and add
some tests for new feature (new validate_sid() function for user and
use_class save handler) after commit.

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?), but they should implement
validate_sid() to prevent adoption from now on. With new code, there
will never be infinite loops. Currently bundled save handlers are not
using this feature.

If there is no objection, I'll commit it to trunk.
It will save much time if I don't have to write wiki for rfc.

Comments are appreciated.

--
Yasuo Ohgaki

2011/11/12 9:50 "Rui Hirokawa" rui_hirokawa@yahoo.co.jp:

Hi,

I strongly recommend to submit the Strict session patch for
php-src(HEAD) because the vulnerability of PHP against the session
adoption/fixation attack is annoying issue of the PHP programmers for
many years.

I also suggest to apply this patch for PHP_5_4 after PHP 5.4.0 is
released. For PHP 5.4.1, I suggest that the default value of
session.use_strict_mode should be 0 (Off) to maintain the backward
compatibility.

Rui

Yasuo Ohgaki wrote:

Hi all,

Few years ago, I have proposed strict session.
It seems PHP 5.4 and php-src don't have protection against session
adoption yet.

Since there will be many TLDs, session adoption attack will be
very easy for some domains until browsers support them.
Even without new TLDs, attacker may place cookie file to attack
session adoption or can exploit paths or domains, since there is no
standard for precedence.

e.g. Domain has more precedence over path on Chrome while
path has greater precedence on IE. This is due to the order difference
of sent cookies. (If there are multiple cookies are set for domain/path, only
one became the outstanding cookie. I think PHP uses first IIRC while other
implementation may use the last. Therefore, browser may not able to
solve this issue, since it may destroy apps specific to browser)

Even if a programmer sets path and domain for PHP session id cookie,
attackers may exploit this to fix session id with session managers that are
vulnerable to session adoption.

If you don't get idea, play with cookie with/without domain/path is set/unset.
You'll see how attacker may use session adoption. Default session module's
configuration (domain not set, path set to /) is very easy to exploit anyway.

I usually set both exact application domain/path, and unset all domain/path
cookies for session id to prevent the attack. I guess this is not
widely adopted.
Even this is not enough. For example, if subdomain is added, Chrome has
greater precedence for subdomain and attacker may exploit it.

I pasted a patch for PHP 5.2 that rejects uninitialized session id. I
think original
patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to
current PHP. If one would like to old behavior, he/she can just turn off the
strict session.

There are too many ways to exploit session with session adoption vulnerable
session manger. Simple solution is making session manager strict.

Any comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Christopher Jones — view source — reply

unread

It will save much time if I don't have to write wiki for rfc.

Please add at least the contents of your email to an RFC so there is
some description that doesn't require trawling through mail archives
to find.

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/

13 years ago by Ferenc Kovacs — view source — reply

unread

On Wed, Nov 23, 2011 at 12:31 AM, Christopher Jones <
christopher.jones@oracle.com> wrote:

It will save much time if I don't have to write wiki for rfc.

Please add at least the contents of your email to an RFC so there is
some description that doesn't require trawling through mail archives
to find.

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/

I don't know that Yasuo has wiki account or not, so I went ahead and
created a draft(just dumping the info there):
https://wiki.php.net/rfc/strict_sessions

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source — reply

unread

2011/11/23 Ferenc Kovacs tyra3l@gmail.com:>>> It will save much time
if I don't have to write wiki for rfc.

Please add at least the contents of your email to an RFC so there is
some description that doesn't require trawling through mail archives
to find.

Thank you. I see you've made new page.
I'll add description later.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Ferenc,

I can login to wiki, but cannot write to the page.

https://wiki.php.net/rfc/strict_sessions

Could you update the page with attached file?

Thank you.

--
Yasuo Ohgaki

13 years ago by Ferenc Kovacs — view source — reply

unread

Ferenc,

I can login to wiki, but cannot write to the page.

https://wiki.php.net/rfc/strict_sessions

Could you update the page with attached file?

Thank you.

--
Yasuo Ohgaki

Sure,

Hannes, could you please give karma to Yasuo so that he can edit the rfc
himself?
Thanks!

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Hannes Magnusson — view source — reply

unread

He has svn account.
If you login with your svn credentials you'll have full write karma to
everything on the wiki.

If you don't remember your password; https://master.php.net/forgot.php

(your username is yohgaki)

-Hannes

Ferenc,

I can login to wiki, but cannot write to the page.

https://wiki.php.net/rfc/strict_sessions

Could you update the page with attached file?

Thank you.

--
Yasuo Ohgaki

Sure,
Hannes, could you please give karma to Yasuo so that he can edit the rfc
himself?
Thanks!

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source — reply

unread

2011/11/23 Hannes Magnusson bjori@php.net:

He has svn account.
If you login with your svn credentials you'll have full write karma to
everything on the wiki.

If you don't remember your password; https://master.php.net/forgot.php

(your username is yohgaki)

Thank you Hannes,

I don't know why I could not press "save" button, but I can save page
now. I've already fixed a few broken English.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Ferenc Kovacs — view source — reply

unread

2011/11/23 Hannes Magnusson bjori@php.net:

He has svn account.
If you login with your svn credentials you'll have full write karma to
everything on the wiki.

If you don't remember your password; https://master.php.net/forgot.php

(your username is yohgaki)

Thank you Hannes,

I don't know why I could not press "save" button, but I can save page
now. I've already fixed a few broken English.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

My guess is that you were trying to edit the page while I had it open for
editing also, so I think that it was my bad. :)

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

Thanks to Perrie, I realized I should try to this patch to be accepted
for 5.4 branch.

As you may already knew, session adoption is "real" threat for PHP
applications. Therefore, this patch should be in 5.4.0 for better
security.

In case of you've missed why session adoption is real threat for PHP
applications, please try to set the same cookies to various path and
domains. Then check and compare the result with IE and Chrome/Firefox.
This patch is really needed for PHP.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

Some users that have tested this patch asked me if it's possible
deleting offending cookies that enable targeted DoS attack.

https://wiki.php.net/rfc/strict_sessions

I would like to add patch that deletes offending cookies which may
controlled by php.ini setting. I can try to delete possible offending
cookies, but recent browsers only sent outstanding cookie. Therefore,
it's impossible to know if it deleted all offending cookies was
successfully deleted. This feature will be best effort based feature.

I think the default setting for deleting cookies should be off by
default, so that users could notice configuration problems. (i.e.
cookie path/domain, session name)

This patch eliminates session adoption/fixation, but introduces
targeted DoS as I mentioned already. Even if it may not be possible to
delete all malicious cookies, but it is worthwhile to have this
feature.

Any comments?

Hannes, I could edit the page once, but "save" button is disabled for
some reason. Could you check my karma? Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Ferenc Kovacs — view source — reply

unread

Hannes, I could edit the page once, but "save" button is disabled for
some reason. Could you check my karma? Thank you.

you have to either provide a comment for the wiki change or check the Minor
Changes checkbox to be allowed to save your work.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Hannes Magnusson — view source — reply

unread

Hannes, I could edit the page once, but "save" button is disabled for
some reason. Could you check my karma? Thank you.

Please fill out the large pink field called "Edit summary".. and write
something in it.
The save button will then be enabled.

-Hannes

13 years ago by Yasuo Ohgaki — view source — reply

unread

Thank you Ferenc and Hannes!

--
Yasuo Ohgaki

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi folks,

I updated wiki page to remove draft status.

https://wiki.php.net/rfc/strict_sessions

Any corrections and comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi folks,

2011/11/29 Yasuo Ohgaki yohgaki@ohgaki.net:

I updated wiki page to remove draft status.

https://wiki.php.net/rfc/strict_sessions

Any corrections and comments are appreciated.

I think I've wrote everything needed into the Wiki page to understand
the issue, so I changed status to "Under Discussion".

According to

https://wiki.php.net/rfc/voting

minimum discussion period is 2 weeks and voting is required.
How should I proceed?

BTW, please note that this patch is "must have" patch for security season.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Daniel K. — view source — reply

unread

Yasuo Ohgaki wrote:

I've made the patch for trunk. I checked by using session module
tests using valgrind.

https://gist.github.com/1379668

As a general comment there seems to be lots of extra white space
in the 'empty' lines of the patch.

Index: ext/session/php_session.h

--- ext/session/php_session.h (revision 319702)
+++ ext/session/php_session.h (working copy)
@@ -69,7 +72,7 @@
PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

PS_CREATE_SID_FUNC(x)

PS_CREATE_SID_FUNC(x);

This is bogus, either you meant to do as for PS_FUNCS_SID below;

PS_CREATE_SID_FUNC(x)

PS_CREATE_SID_FUNC(x); \
PS_VALIDATE_SID_FUNC(x)

or you should leave it alone. Your comments below suggests the latter.

@@ -83,11 +86,12 @@
PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

PS_CREATE_SID_FUNC(x)

PS_CREATE_SID_FUNC(x); \

PS_VALIDATE_SID_FUNC(x)

[...]

Index: ext/session/mod_user_class.c

--- ext/session/mod_user_class.c (revision 319702)
+++ ext/session/mod_user_class.c (working copy)
@@ -141,4 +141,74 @@

RETVAL_LONG(PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels TSRMLS_CC));
}

+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{
size_t len;

const char *p;

char c;

int ret = SUCCESS;

for (p = key; (c = *p); p++) {
/* valid characters are a..z,A..Z,0..9 */

and ',' and '-', bug copied from mod_files.c?

if (!((c >= 'a' && c <= 'z')
	   || (c >= 'A' && c <= 'Z')
	   || (c >= '0' && c <= '9')
	   || c == ','
	   || c == '-')) {
	ret = FAILURE;
	break;
}
}
Index: ext/session/mod_mm.c
===================================================================
--- ext/session/mod_mm.c (revision 319702)
+++ ext/session/mod_mm.c (working copy)
@@ -257,6 +257,38 @@
free(data);
}
+/* If you change the logic here, please also update the error message in

ps_files_open() appropriately */

Really?

+static int ps_mm_valid_key(const char *key)
+{
size_t len;

const char *p;

char c;

int ret = 1;

for (p = key; (c = *p); p++) {
/* valid characters are a..z,A..Z,0..9 */

Deja vu.

if (!((c >= 'a' && c <= 'z')
		|| (c >= 'A' && c <= 'Z')
		|| (c >= '0' && c <= '9')
		|| c == ','
		|| c == '-')) {
	ret = 0;
	break;
}
}
Index: ext/session/mod_user.c
===================================================================
--- ext/session/mod_user.c (revision 319702)
+++ ext/session/mod_user.c (working copy)
@@ -163,6 +163,81 @@
+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{

size_t len;

const char *p;

char c;

int ret = SUCCESS;

for (p = key; (c = *p); p++) {
/* valid characters are a..z,A..Z,0..9 */

Again.

if (!((c >= 'a' && c <= 'z')
	   || (c >= 'A' && c <= 'Z')
	   || (c >= '0' && c <= '9')
	   || c == ','
	   || c == '-')) {
	ret = FAILURE;
	break;
}
}
Index: ext/session/session.c
===================================================================
--- ext/session/session.c (revision 319702)
+++ ext/session/session.c (working copy)
@@ -1663,7 +1670,10 @@
/* remove shutdown function */
remove_user_shutdown_function("session_shutdown", sizeof("session_shutdown") TSRMLS_CC);
for (i = 0; i < 6; i++) {
for (i = 0; i < 8; i++) {
 if (i >= num_args) {
	 break;
 }
@@ -1677,7 +1687,10 @@
zend_alter_ini_entry("session.save_handler", sizeof("session.save_handler"), "user", sizeof("user")-1, PHP_INI_USER, PHP_INI_STAGE_RUNTIME);
}

for (i = 0; i < 6; i++) {
for (i = 0; i < 8; i++) {
 if (i >= num_args) {
	 break;
 }

What about:

for (i = 0, i < num_args, i++)

@@ -2218,9 +2231,13 @@
for (i = 0, mod = ps_modules; i < MAX_MODULES; i++, mod++) {
if (*mod && (*mod)->s_name) {
smart_str_appends(&save_handlers, (*mod)->s_name);
	smart_str_appendc(&save_handlers, ' ');
	 if ((*mod)->s_validate_sid) {
		 smart_str_appends(&save_handlers, "(strict) ");
	 } else {
		 smart_str_appends(&save_handlers, "(adaptive) ");
}
}
}

Funky indentation and brace placement.

This patch adds

validate_sid() to ps_modules (Save handlers)

use_strict_session to php.ini (On by default, off for old behavior)

display that save handler supports strict session or not via
phpinfo() (So that user could know behavior)

update PHP_SESSION_API version (So that save handler authors could
write portable code)

Reading the patch seems to confirm this.

Compatibility issues are

save handlers that are currently working should also work with this
patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro.

None of the bundled ones do currently.

These ps modules should implement validate_sid(). Modules that are
using PS_MOD/PS_FUNCS are not affected, they just marked as "adaptive"
module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())
NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but
it was the same as PS_MOD()/PS_MOD_FUNCS().

Well, not exactly the same.

#define PS_MOD(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x,
ps_delete_##x, ps_gc_##x, php_session_create_id

#define PS_MOD_SID(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x,
ps_delete_##x, ps_gc_##x, ps_create_sid_##x

If old ps_module does not
compile, just remove "_SID" from PS_MOD_SID()/PS_MOD_FUNCS_SID().

This is bad advice due to the difference between the SID/non-SID
versions.

session ID string is checked so that chars are alphanumeric + ','
'-' when use_strict_session=On.

php_session_initialize() checks for some additional invalid characters
as well. Why be strict and allow only [0-9a-zA-Z,-], different storage
backends have different requirements.

(mod_file.c checks session ID this way w/o this patch to prevent
problems. Using restrictive session ID string is better for other ps
modules. IMHO)

Why is it better? And if it is, could the function be made generic
instead of it being copied around, bugs and all?

session read failure is not rescued to eliminate possible infinite
loops. Bundled ps modules were not using this at least.

More on loops below.

You will see some tests are failing since they depend on adaptive
session. By looking into failing test results, you can see this patch
is generating new session ID if it's not a already initialized
session. I'll modify these tests (set use_strict_session=Off) and add
some tests for new feature (new validate_sid() function for user and
use_class save handler) after commit.

Why not at the same time? It seems appropriate to prove that you have
tested the new features when you commit them?

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails and additionally sets
PS(invalid_session_id)

This could never work with:

session_id("foo");
session_start();

could it?

Have you checked that this still works as advertised with the patch
applied?

I may have additional comments if I can find time to test it.

Daniel K.

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Daniel,

2011/12/1 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

I've made the patch for trunk. I checked by using session module
tests using valgrind.

https://gist.github.com/1379668

As a general comment there seems to be lots of extra white space in the
'empty' lines of the patch.

Spaces are left by my emacs.
I don't think there are many lines with extra spaces, but I'll check.

Index: ext/session/php_session.h

--- ext/session/php_session.h (revision 319702)
+++ ext/session/php_session.h (working copy)
@@ -69,7 +72,7 @@

PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

PS_CREATE_SID_FUNC(x)

PS_CREATE_SID_FUNC(x);

This is bogus, either you meant to do as for PS_FUNCS_SID below;

In this case ; may not be needed.
I'll just remove it.

@@ -83,11 +86,12 @@

PS_WRITE_FUNC(x);
PS_DESTROY_FUNC(x);
PS_GC_FUNC(x); \

PS_CREATE_SID_FUNC(x)

PS_CREATE_SID_FUNC(x); \

PS_VALIDATE_SID_FUNC(x)
[...]
Index: ext/session/mod_user_class.c
===================================================================
--- ext/session/mod_user_class.c (revision 319702)
+++ ext/session/mod_user_class.c (working copy)
@@ -141,4 +141,74 @@

RETVAL_LONG(PS(default_mod)->s_gc(&PS(mod_data), maxlifetime,
&nrdels TSRMLS_CC));

}

+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{

size_t len;

const char *p;

char c;

int ret = SUCCESS;

+ for (p = key; (c = *p); p++) {

/* valid characters are a..z,A..Z,0..9 */

and ',' and '-', bug copied from mod_files.c?

No, it's from the original patch that has written by Stefan.
Restricting chars is generally considered better for security.

I'm considering just omitting lower special chars, but also I think
there aren't any point that allowing chars are not needed for the
ps_module. Allowing 'x','y','z',etc for hash function is meaning
less.

if (!((c >= 'a' && c <= 'z')

|| (c >= 'A' && c <= 'Z')

|| (c >= '0' && c <= '9')

|| c == ','

|| c == '-')) {

ret = FAILURE;

break;

}

}
Index: ext/session/mod_mm.c
===================================================================
--- ext/session/mod_mm.c (revision 319702)
+++ ext/session/mod_mm.c (working copy)
@@ -257,6 +257,38 @@
free(data);
}
+/* If you change the logic here, please also update the error message in

ps_files_open() appropriately */

Really?

Copied from existing files ps_module.

Index: ext/session/session.c

--- ext/session/session.c (revision 319702)
+++ ext/session/session.c (working copy)
@@ -1663,7 +1670,10 @@
/* remove shutdown function */
remove_user_shutdown_function("session_shutdown",
sizeof("session_shutdown") TSRMLS_CC);

- for (i = 0; i < 6; i++) {

for (i = 0; i < 8; i++) {

if (i >= num_args) {

break;

}
@@ -1677,7 +1687,10 @@
zend_alter_ini_entry("session.save_handler",
sizeof("session.save_handler"), "user", sizeof("user")-1, PHP_INI_USER,
PHP_INI_STAGE_RUNTIME);

}
- for (i = 0; i < 6; i++) {

for (i = 0; i < 8; i++) {

if (i >= num_args) {

break;

}

What about:

for (i = 0, i < num_args, i++)

That's better. I just put "if" when I noticed num_args matters.
I'll change it.

@@ -2218,9 +2231,13 @@
for (i = 0, mod = ps_modules; i < MAX_MODULES; i++, mod++) {
if (*mod && (*mod)->s_name) {
smart_str_appends(&save_handlers, (*mod)->s_name);

smart_str_appendc(&save_handlers, ' ');

if ((*mod)->s_validate_sid) {

smart_str_appends(&save_handlers,
"(strict) ");

} else {

smart_str_appends(&save_handlers,
"(adaptive) ");
}
}

}

Funky indentation and brace placement.

I though tabfied, but it seem there are spaces.
What do you mean brace placement?

These ps modules should implement validate_sid(). Modules that are
using PS_MOD/PS_FUNCS are not affected, they just marked as "adaptive"
module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())
NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but
it was the same as PS_MOD()/PS_MOD_FUNCS().

Well, not exactly the same.

I forgot that.
Thanks.

#define PS_MOD(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x,
ps_delete_##x, ps_gc_##x, php_session_create_id

#define PS_MOD_SID(x)
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x,
ps_delete_##x, ps_gc_##x, ps_create_sid_##x

- session ID string is checked so that chars are alphanumeric + ','
'-' when use_strict_session=On.

php_session_initialize() checks for some additional invalid characters as
well. Why be strict and allow only [0-9a-zA-Z,-], different storage backends
have different requirements.

It already explained before. It's for precaution. Is there any good
for allowing useless chars for hashes?

It checks within ps_module, so ps_module writer can use any chars other than

/* check session name for invalid characters */
if (PS(id) && strpbrk(PS(id), "\r\n\t <>'\"\\")) {

(mod_file.c checks session ID this way w/o this patch to prevent
problems. Using restrictive session ID string is better for other ps
modules. IMHO)

Why is it better? And if it is, could the function be made generic instead
of it being copied around, bugs and all?

It's possible to restrict valid chars. But as you said, different
ps_module may have different needs.

- session read failure is not rescued to eliminate possible infinite
loops. Bundled ps modules were not using this at least.

More on loops below.

I could not understand what you mean.
New code removes possible infinite loop.

You will see some tests are failing since they depend on adaptive
session. By looking into failing test results, you can see this patch
is generating new session ID if it's not a already initialized
session. I'll modify these tests (set use_strict_session=Off) and add
some tests for new feature (new validate_sid() function for user and
use_class save handler) after commit.

Why not at the same time? It seems appropriate to prove that you have tested
the new features when you commit them?

It's easier to understand that is going on with diffs, isn't it?
Simply adding bunch of INI entries is enough to make failing tests work.

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry logic
kicks in when PS_READ_FUNC() fails and additionally sets
PS(invalid_session_id)

This could never work with:

session_id("foo");
session_start();

could it?

Of course it will, if not let me know.

Did you disabled strict session?
session.use_strict_session=0

Have you checked that this still works as advertised with the patch applied?

Few tests has modified INI section so that they use

session.use_strict_session=0

I may have additional comments if I can find time to test it.

Thanks.
Please let me know if you find any.

Regards,

--
Yasuo Ohgaki

13 years ago by Rasmus Lerdorf — view source — reply

unread

Hi Daniel,

2011/12/1 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

I've made the patch for trunk. I checked by using session module
tests using valgrind.

https://gist.github.com/1379668

As a general comment there seems to be lots of extra white space in the
'empty' lines of the patch.

Spaces are left by my emacs.
I don't think there are many lines with extra spaces, but I'll check.

Please just tell your diff to ignore whitespace when you create the
patch. It is annoying to have to sift through extra irrelevant lines.

-Rasmus

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rasmus,

2011/12/1 Rasmus Lerdorf rasmus@lerdorf.com:

Please just tell your diff to ignore whitespace when you create the
patch. It is annoying to have to sift through extra irrelevant lines.

I think Daniel mean there are extra spaces for indent.
I'll fix it.

Since Daniel mentioned that he cannot disable strict session, I
compiled against current head. According to run-tests.php, it seems
there is something wrong that prevents disabling strict session. I'll
fix it, then notify updated patch.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

I think Daniel mean there are extra spaces for indent.
I'll fix it.

Done.

Since Daniel mentioned that he cannot disable strict session, I
compiled against current head. According to run-tests.php, it seems
there is something wrong that prevents disabling strict session. I'll
fix it, then notify updated patch.

Sorry Daniel. You should do

session.use_strict_mode=0

NOT

session.use_strict_session=0

I also updated tests that are exploiting adoptive sessions.

https://gist.github.com/1379668

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Daniel K. — view source — reply

unread

Yasuo Ohgaki wrote:

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

I think Daniel mean there are extra spaces for indent.
I'll fix it.

That's exactly it, however the updated patch still has problems.

Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.

Since Daniel mentioned that he cannot disable strict session,

I did no such thing. from where did you get that idea?

I mentioned that I had not tested the patch at all, just read it.

I also updated tests that are exploiting adoptive sessions.

https://gist.github.com/1379668

I see you've addressed a few of my comments, but not all.

Specifically, excessive whitespace remains. Both on otherwise empty
lines, and on lines indented with a tab and a space, which just looks
sloppy. See e.g. PS_VALIDATE_SID_FUNC(mm) as an example showing both
defects. You've only fixed it in ext/session/session.c, do the same
to the rest of the patch.

These comments should either be fixed to match the code, or deleted.

  /* valid characters are a..z,A..Z,0..9 */

```
  if (!((c >= 'a' && c <= 'z')
```
```
  	   || (c >= 'A' && c <= 'Z')
```
```
  	   || (c >= '0' && c <= '9')
```
```
  	   || c == ','
```
```
  	   || c == '-')) {
```

ps_user_valid_key returns SUCCESS/FAILURE.
ps_mm_valid_key returns 1/0 as does the exsting ps_files_valid_key.

I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod
may be in for a nasty surpris because of this.

Remember, this is not just about the return value of hash functions, as
this is used to validate session_ids set with session_id() as well.

Daniel K.

13 years ago by Yasuo Ohgaki — view source — reply

unread

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

I think Daniel mean there are extra spaces for indent.
I'll fix it.

That's exactly it, however the updated patch still has problems.

Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.

Does CODING_STANDARDS mention this?

Since Daniel mentioned that he cannot disable strict session,

I did no such thing. from where did you get that idea?

Because you wrote this.

This could never work with:

session_id("foo");
session_start();

could it?

I think you understands it can be controlled by session.use_strict_mode now.

I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod may
be in for a nasty surpris because of this.

Remember, this is not just about the return value of hash functions, as this
is used to validate session_ids set with session_id() as well.

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

When user tried to use session_id() to set their on ID when
session.use_strict_mode=1, it would be nicer to emit warning error.
Failing silently is not good.

Any comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi all,

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

When user tried to use session_id() to set their on ID when
session.use_strict_mode=1, it would be nicer to emit warning error.
Failing silently is not good.

Any comments?

Updated patch so that session_id() raise warning when use_strict_mode is on.

I forgot to mention that I've fixed session_set_save_handler() to take
care new parameters.

If everybody is comfortable with this patch. I would like to write
docs for this.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Daniel K. — view source — reply

unread

Yasuo Ohgaki wrote:

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:
Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.

Does CODING_STANDARDS mention this?

I hope not, this should be obvious.

Since Daniel mentioned that he cannot disable strict session,

I did no such thing. from where did you get that idea?

Because you wrote this.

You cut out the essential lines before:

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails and additionally sets
PS(invalid_session_id)

Where I proposition that the existing logic (which you removed in your
patch) could not have done any looping. Because, then the code below
would not have worked.

This could never work with:

session_id("foo");
session_start();

could it?

No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this
must have been added for the benefit of external modules once upon a time.

However, the current code seems to never have worked for anyone.

If we suppose that it exists an external session module which sets
PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of
session_id("foo"); session_start(); would have left you with a NULL
session_id, then a new one would have been created after goto new_session.

I hope someone would have discovered this had they actually implemented
such a module.

I take this as a sign that no such module exists, but I may of course be
wrong in that.

I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod may
be in for a nasty surpris because of this.

Remember, this is not just about the return value of hash functions, as this
is used to validate session_ids set with session_id() as well.

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If by 'user' you mean any PHP script, I disagree.

The purported goal of the patch was to thwart attempts to choose ones own
session_id by sending fake/stale cookies. To also make session_id() unusable
from PHP scripts, or to thwart the script-writers ability to choose the
session_id seems like a serious bug.

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

Of course, but I think we need to find out the semantics, and the expected
use case / protection level this new feature is supposed to have.

With my current understanding of the patch, and my use of PHP, it seems
mostly useless. But, I stress again that I have just read the code, I have
not actually used it yet.

Daniel K.

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Daniel,

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.

Does CODING_STANDARDS mention this?

I hope not, this should be obvious.

Depends on editor or IDE setting/behavior, but I don't have problem with this.

Since Daniel mentioned that he cannot disable strict session,

I did no such thing. from where did you get that idea?

Because you wrote this.

You cut out the essential lines before:

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails and additionally sets
PS(invalid_session_id)

Where I proposition that the existing logic (which you removed in your
patch) could not have done any looping. Because, then the code below would
not have worked.

This could never work with:

session_id("foo");
session_start();

could it?

No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this must
have been added for the benefit of external modules once upon a time.

However, the current code seems to never have worked for anyone.

If we suppose that it exists an external session module which sets
PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of
session_id("foo"); session_start(); would have left you with a NULL
session_id, then a new one would have been created after goto new_session.

I hope someone would have discovered this had they actually implemented such
a module.

I take this as a sign that no such module exists, but I may of course be
wrong in that.

It could be possible that uses current code to prevent session
adoption. To do that, ps_module should keep state variable to avoid
infinite goto loops.

Current code (+ PECL's sqlite/memcache/msession/session_pgsql and
memcached ps_module) is not written to use this goto loop such way.
When read failure happens, these ps_module simply goes to infinate
loop. Therefore, new code does not hart any.

However, I found that msession is using PS_MOD_SID macro. It should
add validate_id() to work, but it's just getting ID using msession
function and use php_session_create_id() when it fails. So using
PS_MOD macro is also ok for msession.

I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod
may
be in for a nasty surpris because of this.

Remember, this is not just about the return value of hash functions, as
this
is used to validate session_ids set with session_id() as well.

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If by 'user' you mean any PHP script, I disagree.

If user really want to set session ID, they can explicitly disable
use_strict_mode.

For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.

BTW, one module that is exploiting adoptive session is session_pgsql
which is written by me.

The purported goal of the patch was to thwart attempts to choose ones own
session_id by sending fake/stale cookies. To also make session_id() unusable
from PHP scripts, or to thwart the script-writers ability to choose the
session_id seems like a serious bug.

Almost all application shouldn't do this, unless they really
understand what they are doing.

If users (script writers) really wants to create session ID by their
own, they can use user save handlers or they can simply turn it off by
ini_set('session.use_strict_mode', false).

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

Of course, but I think we need to find out the semantics, and the expected
use case / protection level this new feature is supposed to have.

With my current understanding of the patch, and my use of PHP, it seems
mostly useless. But, I stress again that I have just read the code, I have
not actually used it yet.

Please try to use it. There shouldn't be any problems for almost all
applications. I haven't check Shuhosin and it's patch, but I think it
has same code. Applications that requires some kind of static or
somewhat static session ID should be many.

According to google code search, there are some code that actually
using session_id() to set ID.

e.g.
if ((isset($s)) && ($s == 'contact')) {
if (session_id() != 'contact') {
/* Retrieve the value of the hidden field in
the contact module */
session_id('contact');
session_cache_limiter('nocache');
session_start();
}
}

or code that generate ID by their own like session_id(md5(uniqid()).

BTW, the default would be use_strict_session=1 for trunk and
use_strict_session=0 for PHP_5_4 (and PHP_5_3?) Current users are not
affected as long as they are using the default. Users should know the
risk of session adoption/fixation, though.

"session_id('contact')" / "session_id(md5(uniqid())" are bad code, but
they have plenty time to fix it. I don't think having session_id() is
bad, but it could be abused. Prohibiting setting session ID under
strict mode would be better for security, IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Ferenc Kovacs — view source — reply

unread

Hi Daniel,

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Daniel K. dk@uw.no:

Yasuo Ohgaki wrote:

2011/12/2 Yasuo Ohgaki yohgaki@ohgaki.net:

Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.

Does CODING_STANDARDS mention this?

I hope not, this should be obvious.

Depends on editor or IDE setting/behavior, but I don't have problem with
this.

Since Daniel mentioned that he cannot disable strict session,

I did no such thing. from where did you get that idea?

Because you wrote this.

You cut out the essential lines before:

It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)

I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails and additionally sets
PS(invalid_session_id)

Where I proposition that the existing logic (which you removed in your
patch) could not have done any looping. Because, then the code below
would
not have worked.

This could never work with:

session_id("foo");
session_start();

could it?

No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this
must
have been added for the benefit of external modules once upon a time.

However, the current code seems to never have worked for anyone.

If we suppose that it exists an external session module which sets
PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of
session_id("foo"); session_start(); would have left you with a NULL
session_id, then a new one would have been created after goto
new_session.

I hope someone would have discovered this had they actually implemented
such
a module.

I take this as a sign that no such module exists, but I may of course be
wrong in that.

It could be possible that uses current code to prevent session
adoption. To do that, ps_module should keep state variable to avoid
infinite goto loops.

Current code (+ PECL's sqlite/memcache/msession/session_pgsql and
memcached ps_module) is not written to use this goto loop such way.
When read failure happens, these ps_module simply goes to infinate
loop. Therefore, new code does not hart any.

However, I found that msession is using PS_MOD_SID macro. It should
add validate_id() to work, but it's just getting ID using msession
function and use php_session_create_id() when it fails. So using
PS_MOD macro is also ok for msession.

I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor
sod
may
be in for a nasty surpris because of this.

Remember, this is not just about the return value of hash functions, as
this
is used to validate session_ids set with session_id() as well.

With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.

If by 'user' you mean any PHP script, I disagree.

If user really want to set session ID, they can explicitly disable
use_strict_mode.

For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.

BTW, one module that is exploiting adoptive session is session_pgsql
which is written by me.

The purported goal of the patch was to thwart attempts to choose ones own
session_id by sending fake/stale cookies. To also make session_id()
unusable
from PHP scripts, or to thwart the script-writers ability to choose the
session_id seems like a serious bug.

Almost all application shouldn't do this, unless they really
understand what they are doing.

If users (script writers) really wants to create session ID by their
own, they can use user save handlers or they can simply turn it off by
ini_set('session.use_strict_mode', false).

If user would like to use adoptive session, user may set
session.use_strict_mode=0.

Of course, but I think we need to find out the semantics, and the
expected
use case / protection level this new feature is supposed to have.

With my current understanding of the patch, and my use of PHP, it seems
mostly useless. But, I stress again that I have just read the code, I
have
not actually used it yet.

Please try to use it. There shouldn't be any problems for almost all
applications. I haven't check Shuhosin and it's patch, but I think it
has same code. Applications that requires some kind of static or
somewhat static session ID should be many.

According to google code search, there are some code that actually
using session_id() to set ID.

e.g.
if ((isset($s)) && ($s == 'contact')) {
if (session_id() != 'contact') {
/* Retrieve the value of the hidden field in
the contact module */
session_id('contact');
session_cache_limiter('nocache');
session_start();
}
}

or code that generate ID by their own like session_id(md5(uniqid()).

BTW, the default would be use_strict_session=1 for trunk and
use_strict_session=0 for PHP_5_4 (and PHP_5_3?) Current users are not
affected as long as they are using the default. Users should know the
risk of session adoption/fixation, though.

"session_id('contact')" / "session_id(md5(uniqid())" are bad code, but
they have plenty time to fix it. I don't think having session_id() is
bad, but it could be abused. Prohibiting setting session ID under
strict mode would be better for security, IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--

wouldn't it be better if we push the session id validation to the
application level?
we should provide a hook both to the C api and to
the session_set_save_handler.
of course we can additionally change the default range of valid characters
for the default session handler implementation, but it would still possible
for the application developer to change or extend that.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Ferenc,

2011/12/2 Ferenc Kovacs tyra3l@gmail.com:

wouldn't it be better if we push the session id validation to the
application level?
we should provide a hook both to the C api and to
the session_set_save_handler.
of course we can additionally change the default range of valid characters
for the default session handler implementation, but it would still possible
for the application developer to change or extend that.

It's possible with session_set_save_handler(), but users should
implement all save handlers. session_set_save_handler() could be
modified just to add validation function and choose any chars except
chars invalidated by php_session_initialize()

/* check session name for invalid characters */
if (PS(id) && strpbrk(PS(id), "\r\n\t <>'\"\\")) {
	efree(PS(id));
	PS(id) = NULL;
}

For example, we may do

bool session_set_save_handler(SESSION_SET_VALIDATE_ID,
"my_validation_id_function");

There are many possible implementations.
Any comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

I haven't mentioned, but I'll file CVE for this because this is
exploitable venerability. Unless it is filed by CVE, not many people
would know the risk of adoption/fixation. There may be the entry for
this, though.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source — reply

unread

Hi!

If user really want to set session ID, they can explicitly disable
use_strict_mode.

For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.

I'm not sure I understand - why prohibiting the setting of the session
ID is necessary? I understand the idea of the original patch - if
somebody could set your session ID for you, without knowledge of either
the user or the app, it is bad since third party knows this ID and thus
can use it. However, I do not understand why it is bad for the app to
set the session ID - after all, session ID comes from the app anyway,
what's the problem here? Why we have to deny benefits of the protection
that this patch claims to provide against injecting session by the third
party to the applications that set the session from inside? I do not
understand the link between one and the other, can you please explain?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

If user really want to set session ID, they can explicitly disable
use_strict_mode.

For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.

I'm not sure I understand - why prohibiting the setting of the session ID is
necessary? I understand the idea of the original patch - if somebody could
set your session ID for you, without knowledge of either the user or the
app, it is bad since third party knows this ID and thus can use it. However,
I do not understand why it is bad for the app to set the session ID - after
all, session ID comes from the app anyway, what's the problem here? Why we
have to deny benefits of the protection that this patch claims to provide
against injecting session by the third party to the applications that set
the session from inside? I do not understand the link between one and the
other, can you please explain?

For example, it is easy to find cases with google code search, that
users are setting ID while they really should do is
session_regenerate_id(). These kind of mistakes would be better to be
prevented under strict mode, IMHO.

We can say, shooting your own foot is your responsibility, but I think
raising error would be more user friendly.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source — reply

unread

Hi!

For example, it is easy to find cases with google code search, that
users are setting ID while they really should do is
session_regenerate_id(). These kind of mistakes would be better to be
prevented under strict mode, IMHO.

I'm not sure how that would help in this case - so the set would be
rejected, then the users will turn the strict mode off to make their
code work and thus lose the protection it provides. How that improves
anything? Setting session ID and protection against adoption are two
different things, why you need to turn off the latter to get the former
working?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

For example, it is easy to find cases with google code search, that
users are setting ID while they really should do is
session_regenerate_id(). These kind of mistakes would be better to be
prevented under strict mode, IMHO.

I'm not sure how that would help in this case - so the set would be
rejected, then the users will turn the strict mode off to make their code
work and thus lose the protection it provides. How that improves anything?
Setting session ID and protection against adoption are two different things,
why you need to turn off the latter to get the former working?

Since the patch sets INI_ALL for session.use_strict_mode, users may
disable strict_mode for specific code. They don't have to disable
strict mode for whole application.

It's possible allow user defined session id, but as far as I searched
on google, users are just misused or abused session_id($newid).

Since there are many places that users could shooting their own foot,
I don't mind to allow session_id($newid). It's far more important
provide protection for decent code.

Should I go ahead to change this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source — reply

unread

Hi!

My main concern with this change is that it is binary incompatible with
existing session implementation, which means it would be hard to get it
into 5.3 and 5.4. While I understand sometimes adding handlers is
inevitable, in this case I'm not sure why it is required. So far the
only reason I understand was "If we do this, users cannot know if
ps_modules is new one that prevents adoption or not, especially for
third party ps_modules." but this is definitely a documentation issue
and even with new API you can not really know that - you just know
there's a handler but you can not know the module implements it properly

maybe it just always returns OK.

Maybe we can make separate binary-compatible patch for 5.3/5.4 branches
and patch with new APIs for trunk? The user-space handlers for 5.3/5.4
would still have to do the checks (though stock modules will have the
checks built-in) but for trunk they could do it by defining validation
callbacks.

I also still not sure why default SID creation function was changed to 3
identical cloned functions. I do not think it's the right way - if some
macro is not allowing us to leave it as is, we can have another macro,
but I do not see how splitting one standard SID creation function into a
number of identical clones is beneficial. I think it needs to be changed.

For user module, with the patch if you add validation handler you also
have to add create_sid handler - which most users don't even want to
change. We should allow for using standard SID creation functions there
(maybe by passing NULL there? or by having some function that implements
php_session_create_id, maybe even better), and we shouldn't check
validate_sid when we mean create_sid - if we allow to set create_sid, we
should also support case where we set create_sid but not validate_sid.
Also, all other modules when SID creation fails set invalid_sid and
return NULL, while new user module SID creation returns "" and doesn't
set invalid_sid. Why is that?

We also probably need to deal then with the question what should be done
when create_sid fails (I'd say we can not do much but issue fatal error
but maybe there are any better ideas) if we allow it to fail. We don't
seem to check invalid_sid now after create_sid, so if we're setting it,
we must check it.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

My main concern with this change is that it is binary incompatible with
existing session implementation, which means it would be hard to get it into
5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in
this case I'm not sure why it is required. So far the only reason I
understand was "If we do this, users cannot know if ps_modules is new one
that prevents adoption or not, especially for third party ps_modules." but
this is definitely a documentation issue and even with new API you can not
really know that - you just know there's a handler but you can not know the
module implements it properly - maybe it just always returns OK.

Programmer has freedom to do bad thing, but it's important for better
security to have dedicated API for validation. From my experience,
it's much easier let programmers to use API, but to code properly.

Programmers may use strict session using methods that are written in
wiki page. However, most of applications don't have proper protection.
One reason would be lack of API, I suppose.

Maybe we can make separate binary-compatible patch for 5.3/5.4 branches and
patch with new APIs for trunk? The user-space handlers for 5.3/5.4 would

If we care about binary API compatibility, how about make

PS_MOD_SID2/PS_MOD_FUNCS_SID2

macros? Then we can forget about ABI.

still have to do the checks (though stock modules will have the checks
built-in) but for trunk they could do it by defining validation callbacks.

I also still not sure why default SID creation function was changed to 3
identical cloned functions. I do not think it's the right way - if some
macro is not allowing us to leave it as is, we can have another macro, but I
do not see how splitting one standard SID creation function into a number of
identical clones is beneficial. I think it needs to be changed.

Ok. I'll make a default char validation function for this and make it
call from mod_*.c

If ps_module writers would like to use special chars for whatever
reason, they may use their own function.

For user module, with the patch if you add validation handler you also have
to add create_sid handler - which most users don't even want to change. We
should allow for using standard SID creation functions there (maybe by
passing NULL there? or by having some function that implements
php_session_create_id, maybe even better), and we shouldn't check
validate_sid when we mean create_sid - if we allow to set create_sid, we
should also support case where we set create_sid but not validate_sid.
Also, all other modules when SID creation fails set invalid_sid and return
NULL, while new user module SID creation returns "" and doesn't set
invalid_sid. Why is that?

We also probably need to deal then with the question what should be done
when create_sid fails (I'd say we can not do much but issue fatal error but
maybe there are any better ideas) if we allow it to fail. We don't seem to
check invalid_sid now after create_sid, so if we're setting it, we must
check it.

Thank you. I'll take care of this.
If script or ps_module returns empty SID, it should generate SID by
using default SID creation function.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source — reply

unread

Hi!

If we care about binary API compatibility, how about make

PS_MOD_SID2/PS_MOD_FUNCS_SID2

macros? Then we can forget about ABI.

I'm sorry, I don't understand how any macros would help anything. Adding
stuff to the structure would break binary compatibility, how the macros
would help it?

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

If we care about binary API compatibility, how about make

PS_MOD_SID2/PS_MOD_FUNCS_SID2

macros? Then we can forget about ABI.

I'm sorry, I don't understand how any macros would help anything. Adding
stuff to the structure would break binary compatibility, how the macros
would help it?

Since it is just adding new separate structure to session module, but
PHP itself checks API version number. So users cannot use the same
binary module anyway. I should have said API, not ABI.

Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

2011/12/4 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

If we care about binary API compatibility, how about make

PS_MOD_SID2/PS_MOD_FUNCS_SID2

macros? Then we can forget about ABI.

I'm sorry, I don't understand how any macros would help anything. Adding
stuff to the structure would break binary compatibility, how the macros
would help it?

Since it is just adding new separate structure to session module, but
PHP itself checks API version number. So users cannot use the same
binary module anyway. I should have said API, not ABI.

Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID.

Oops, I was thinking to add new structure, but I forget to do.
Anyway, if we care about ABI, we should add new structure or should
have other solutions.

--
Yasuo Ohgaki

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hold on,

I'll check the source more carefully.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Stas Malyshev — view source — reply

unread

Hi!

Since it is just adding new separate structure to session module, but
PHP itself checks API version number. So users cannot use the same
binary module anyway. I should have said API, not ABI.

Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID.

That's the problem. All versions in 5.3.x and 5.4.x lines should be
binary compatible, means no changes that aren't binary compatible can go
into 5.3 or 5.4.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

Since it is just adding new separate structure to session module, but
PHP itself checks API version number. So users cannot use the same
binary module anyway. I should have said API, not ABI.

Now it should compile with msession which uses
PHP_MOD_SID/PS_MOD_FUNCS_SID.

That's the problem. All versions in 5.3.x and 5.4.x lines should be binary
compatible, means no changes that aren't binary compatible can go into 5.3
or 5.4.

Sounds fair.

===PHP 5.4-dev (php-src-5.4) compiled sqlite with PHP 5.5-dev (trunk)===

[yohgaki@dev php-src]$ ./php -d
extension=/home/yohgaki/svn/oss/php.net/pecl/sqlite/trunk/modules/sqlite.so
-m
[PHP Modules]
<snip>
sockets
SPL
SQLite <==== loaded
standard
sysvmsg
sysvsem
sysvshm
tokenizer
xml
xmlreader
xmlwriter
zlib

[Zend Modules]

It loads, but it's segfaulting for session tests.

==24315== Process terminating with default action of signal 4 (SIGILL)
==24315== Illegal opcode at address 0x5BF56AA
==24315== at 0x5BF56AA: ??? (in
/home/yohgaki/ext/svn/oss/php.net/pecl/sqlite/trunk/modules/sqlite.so)
==24315== by 0x556C26: php_session_initialize (session.c:485)
==24315== by 0x557304: php_session_start (session.c:1470)
==24315== by 0x5578E8: zif_session_start (session.c:1923)
==24315== by 0x6FF63B: zend_do_fcall_common_helper_SPEC
(zend_vm_execute.h:642)
==24315== by 0x6F3EDF: execute (zend_vm_execute.h:410)
==24315== by 0x68363E: zend_execute_scripts (zend.c:1272)
==24315== by 0x6276A6: php_execute_script (main.c:2414)
==24315== by 0x72DF44: do_cli (php_cli.c:983)
==24315== by 0x72E6B7: main (php_cli.c:1356)

Let's see what I can do for it. Please wait for a while.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

If we care about binary API compatibility, how about make

PS_MOD_SID2/PS_MOD_FUNCS_SID2

macros? Then we can forget about ABI.

I'm sorry, I don't understand how any macros would help anything. Adding
stuff to the structure would break binary compatibility, how the macros
would help it?

Macro is for source level compatibility for msession or like, not for
binary level.

AFAIK, if we just add entry at end of structure and there is no
alignment issue, then we can keep binary compatibility. There may be
platforms that may causes problems, though. It there is, please let me
know.

I'll think about ABI for PHP 5.3/5.4, but isn't better to have current
patch for PHP 5.4.0? Then we don't need additional memory
initialization/checks for 5.4.

I compiled sqilte module to test it, but valgrind reports memory leaks
:( I guess I should use memcache for testing.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stats,

I updated the patch to address discussion.

https://gist.github.com/1379668

2011/12/4 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

My main concern with this change is that it is binary incompatible with
existing session implementation, which means it would be hard to get it into
5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in
this case I'm not sure why it is required. So far the only reason I
understand was "If we do this, users cannot know if ps_modules is new one
that prevents adoption or not, especially for third party ps_modules." but
this is definitely a documentation issue and even with new API you can not
really know that - you just know there's a handler but you can not know the
module implements it properly - maybe it just always returns OK.

I updated the patch and fixed them all.

There is no API incompatibility also, since I added
PS_MOD_SID2/PS_MOD_FUNCS_SID2. Thanks Daniel K.

I also still not sure why default SID creation function was changed to 3
identical cloned functions. I do not think it's the right way - if some
macro is not allowing us to leave it as is, we can have another macro, but I
do not see how splitting one standard SID creation function into a number of
identical clones is beneficial. I think it needs to be changed.

I consolidated key validation function into session.c.

Session ID was created by php_session_create_id() function by default.
So there weren't 3 creation function, but key validation functions.

I hope you feel comfortable with new patch.

For user module, with the patch if you add validation handler you also have
to add create_sid handler - which most users don't even want to change. We
should allow for using standard SID creation functions there (maybe by
passing NULL there? or by having some function that implements
php_session_create_id, maybe even better), and we shouldn't check
validate_sid when we mean create_sid - if we allow to set create_sid, we
should also support case where we set create_sid but not validate_sid.
Also, all other modules when SID creation fails set invalid_sid and return
NULL, while new user module SID creation returns "" and doesn't set
invalid_sid. Why is that?

We also probably need to deal then with the question what should be done
when create_sid fails (I'd say we can not do much but issue fatal error but
maybe there are any better ideas) if we allow it to fail. We don't seem to
check invalid_sid now after create_sid, so if we're setting it, we must
check it.

Thank you for catching this.

I added fallback to php_session_create_id() with SHA1 hash as hash
function. Even if users/ps_module writers did something wrong in their
code, it always has valid PS(id). It may be better to raise notice or
warning error, but I don't add it. Any comments?

I think the patch is much better than the original thanks to discussion.

Any comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Strict session?

Index: ext/session/php_session.h

Index: ext/session/tests/session_commit_variation4.phpt

/* Index: ext/session/tests/session_id_basic.phpt

/* Index: ext/session/tests/session_write_close_variation4.phpt

/* Index: ext/session/mod_files.c

Index: ext/session/session.c

Index: ext/session/mod_mm.c

/* Index: ext/session/mod_user.c

Index: ext/session/php_session.h

Index: ext/session/mod_user_class.c

Index: ext/session/php_session.h

Index: ext/session/session.c

That's the problem. All versions in 5.3.x and 5.4.x lines should be binary compatible, means no changes that aren't binary compatible can go into 5.3 or 5.4.

===PHP 5.4-dev (php-src-5.4) compiled sqlite with PHP 5.5-dev (trunk)===

[Zend Modules]

/*
Index: ext/session/tests/session_id_basic.phpt

/*
Index: ext/session/tests/session_write_close_variation4.phpt

/*
Index: ext/session/mod_files.c

/*
Index: ext/session/mod_user.c

That's the problem. All versions in 5.3.x and 5.4.x lines should be
binary compatible, means no changes that aren't binary compatible can go
into 5.3 or 5.4.