Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90928
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.160.180 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <CAGa2bXYGpV=P-680ig1LOkYWsSRS-Ya2rYVa3wo3vrxdT=zwiw@mail.gmail.com>
References: <CAGa2bXbg_kebBVg3tSh4uFHZ6nQXRbjUXNQa0JZx9sezQqb5kg@mail.gmail.com>
 <CAGa2bXYZXkCJwFKVwaNfV+OEw0FzeFFbtxLm0zRmFVE_b4xsMw@mail.gmail.com>
 <CAGa2bXbRJMZFcXfZc9o7YnAhkDRfYKPjO+10k-eKXdoGNTREbg@mail.gmail.com> <CAGa2bXYGpV=P-680ig1LOkYWsSRS-Ya2rYVa3wo3vrxdT=zwiw@mail.gmail.com>
Date: Tue, 26 Jan 2016 16:01:55 +0900
Message-ID: <CAGa2bXbAH7iDFQ8HbvLKSbq9_ZUJLM1fn6aOWdz4+gYdtXVKjw@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [RFC Discussion] Precise Session Management
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi all,

On Tue, Jan 26, 2016 at 11:22 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> On Fri, Jan 22, 2016 at 10:32 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>> On Fri, Jan 22, 2016 at 10:19 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>>>
>>> https://github.com/php/php-src/pull/1734
>>>
>>> Few things are missing still, but it's good enough to review basic features.
>>
>> Please note that if you execute run-tests.php with this patch,
>> it causes failures on other branches/versions' session tests.
>>
>> To remove these test errors, you can do
>>
>> rm -f /tmp/sess_*
>> rm -f /tmp/u_sess*
>>
>> This issue will be fixed by the time when patch is completed.
>
> Other than test dependency issue, all issues and proposals are
> fixed/implemented. Test dependency will be fixed before merge.
>
> https://wiki.php.net/rfc/precise_session_management
> https://github.com/php/php-src/pull/1734
>
> Session will be more reliable and secure with this.
> Please review and comment. If there is no issues, I'll start
> vote shortly.

I didn't pay attention that current session_id() allows any chars for session ID
while session module and save handlers explicitly disallow characters for
security reasons. e.g. Session module silently removes HTML special chars,
files save handler does not allow PATH special chars.
Current situation is not good at all.

Since this RFC is about preciseness of session management, I would like to
change session_id() validates against default allowed chars as follows.
(As well as enabling already written session_create_id() function)
This patch is against the PR.

Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler,
but it should be checked always by session module. Since the function
only allows
chars used by ID, I would like to add "_" a valid char. "_" should be
very safe char.


These changes for session ID would be the largest BC of this RFC.
Any comments for this change?


diff --git a/ext/session/session.c b/ext/session/session.c
index 7b3203a..fdc6a91 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -566,7 +566,7 @@ PHPAPI int php_session_valid_key(const char *key) /* {{{ */

     /* Somewhat arbitrary length limit here, but should be way more than
        anyone needs and avoids file-level warnings later on if we
exceed MAX_PATH */
-    if (len == 0 || len > 128) {
+    if (len == 0 || len > PHP_SESSION_KEY_MAX_LEN) {
         ret = FAILURE;
     }

@@ -2460,31 +2460,35 @@ static PHP_FUNCTION(session_save_path)
    Return the current session id. If newid is given, the session id
is replaced with newid */
 static PHP_FUNCTION(session_id)
 {
-    zend_string *name = NULL;
+    zend_string *id = NULL;
     int argc = ZEND_NUM_ARGS();

-    if (zend_parse_parameters(argc, "|S", &name) == FAILURE) {
+    if (zend_parse_parameters(argc, "|S", &id) == FAILURE) {
         return;
     }

     if (PS(id)) {
-        /* keep compatibility for "\0" characters ???
-         * see: ext/session/tests/session_id_error3.phpt */
-        size_t len = strlen(ZSTR_VAL(PS(id)));
-        if (UNEXPECTED(len != ZSTR_LEN(PS(id)))) {
-            RETVAL_NEW_STR(zend_string_init(ZSTR_VAL(PS(id)), len, 0));
-        } else {
-            RETVAL_STR_COPY(PS(id));
-        }
+        RETVAL_STR_COPY(PS(id));
     } else {
         RETVAL_EMPTY_STRING();
     }

-    if (name) {
+    if (id) {
         if (PS(id)) {
             zend_string_release(PS(id));
         }
-        PS(id) = zend_string_copy(name);
+        if (php_session_valid_key(ZSTR_VAL(id)) == FAILURE) {
+            zend_string *new_id;
+            /* Set random ID for security reason. */
+            php_error_docref(NULL, E_WARNING,
+                             "Session ID cannot exceed session max length %d "
+                             "and/or contain special characters. Only
aphanumeric, ',', '-' are allowed "
+                             "Random session ID is set",
+                             PHP_SESSION_KEY_MAX_LEN);
+            PS(id) = php_session_create_id(NULL);
+        } else {
+            PS(id) = zend_string_copy(id);
+        }
     }
 }
 /* }}} */
@@ -2519,7 +2523,6 @@ static PHP_FUNCTION(session_regenerate_id)

 /* {{{ proto void session_create_id([string prefix])
    Generate new session ID. Intended for user save handlers. */
-#if 0
 /* This is not used yet */
 static PHP_FUNCTION(session_create_id)
 {
@@ -2531,34 +2534,34 @@ static PHP_FUNCTION(session_create_id)
     }

     if (prefix && ZSTR_LEN(prefix)) {
-        if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) {
-            /* E_ERROR raised for security reason. */
-            php_error_docref(NULL, E_WARNING, "Prefix cannot contain
special characters. Only aphanumeric, ',', '-' are allowed");
-            RETURN_FALSE;
+        if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE
+            || ZSTR_LEN(prefix) > PHP_SESSION_KEY_MAX_LEN/2) {
+            /* Do not use prefix for security reason. */
+            php_error_docref(NULL, E_WARNING,
+                             "Prefix cannot exceed session max length %d "
+                             "and/or contain special characters. Only
aphanumeric, ',', '-' are allowed",
+                              PHP_SESSION_KEY_MAX_LEN/2);
         } else {
             smart_str_append(&id, prefix);
         }
     }

-    if (PS(session_status) == php_session_active) {
-        new_id = PS(mod)->s_create_sid(&PS(mod_data));
-    } else {
-        new_id = php_session_create_id(NULL);
-    }
+    /* Should call the default always to avoid problems with user handler */
+    new_id = php_session_create_id(NULL);

     if (new_id) {
         smart_str_append(&id, new_id);
         zend_string_release(new_id);
     } else {
         smart_str_free(&id);
-        php_error_docref(NULL, E_WARNING, "Failed to create new ID");
+        /* E_RECOVERBELE_ERROR raised for security reason. */
+        php_error_docref(NULL, E_RECOVERABLE_ERROR, "Failed to create new ID");
         RETURN_FALSE;
     }
     smart_str_0(&id);
     RETVAL_NEW_STR(id.s);
     smart_str_free(&id);
 }
-#endif
 /* }}} */

 /* {{{ proto string session_cache_limiter([string new_cache_limiter])
@@ -2901,6 +2904,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_session_id, 0, 0, 0)
     ZEND_ARG_INFO(0, id)
 ZEND_END_ARG_INFO()

+ZEND_BEGIN_ARG_INFO_EX(arginfo_session_create_id, 0, 0, 0)
+    ZEND_ARG_INFO(0, prefix)
+ZEND_END_ARG_INFO()
+
 ZEND_BEGIN_ARG_INFO_EX(arginfo_session_regenerate_id, 0, 0, 0)
     ZEND_ARG_INFO(0, delete_old_session)
 ZEND_END_ARG_INFO()
@@ -2989,6 +2996,7 @@ static const zend_function_entry session_functions[] = {
     PHP_FE(session_module_name,       arginfo_session_module_name)
     PHP_FE(session_save_path,         arginfo_session_save_path)
     PHP_FE(session_id,                arginfo_session_id)
+    PHP_FE(session_create_id,         arginfo_session_create_id)
     PHP_FE(session_regenerate_id,     arginfo_session_regenerate_id)
     PHP_FE(session_decode,            arginfo_session_decode)
     PHP_FE(session_encode,            arginfo_session_void)

--
Yasuo Ohgaki
yohgaki@ohgaki.net