yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Spam sessions to DB #18061

Closed Yuriy-Svetlov closed 4 years ago

Yuriy-Svetlov commented 4 years ago

What steps will reproduce the problem?

Send GET request or may be any request with any ID session.

What is the expected result?

That the query will not be inserted into the database table "session".

What do you get instead?

In DB will be inserted the query in table "session".

Additional info

Q A
Yii version 2.0.15.1
PHP version 7.3
Operating system Win 8.1
samdark commented 4 years ago

Why wouldn't it be inserted if the session is actually started?

Yuriy-Svetlov commented 4 years ago

@samdark

Why wouldn't it be inserted if the session is actually started?

But this session do`not exist in DB. This session must not opened. But it starts.

samdark commented 4 years ago

How do you pass session ID via GET?

Yuriy-Svetlov commented 4 years ago

@samdark

aaa

// Iteration 1

  1. No need login on a site. (clear all sessions for site)
  2. Open developer console in the browser.
  3. Create session cookie: set any value (as example: ln06etc8od44p7ilv07ek6shef3dvjt477) for session cookie name "PHPSESSID".
  4. Refresh the browser page.
  5. Look in database. This session will be written in the database.

// Iteration 2, 3, 4, 5 .... Change value of session cookie name "PHPSESSID", and refresh the browser page.

All sessions will be recorded in the database.

samdark commented 4 years ago

The behavior you are observing is normal and controlled by session.use_strict_mode php.ini setting. It is 0 by default, when changed to 1, PHP doesn't create a session with a user-specified ID.

samdark commented 4 years ago

I've added a mention of the setting to the guide.

Yuriy-Svetlov commented 4 years ago

@samdark Thanks you very much!

Yuriy-Svetlov commented 4 years ago

@samdark

My test.php

<?php

phpinfo();

session.use_strict_mode | 1 | 1

Nothing has changed.

Too same occur.

I use Yii2, and sessions are written to the database when I change them in the browser and reload page.

samdark commented 4 years ago

What's your PHP version? It should be at least 5.5.2. I've tested with 7.4.3.

Yuriy-Svetlov commented 4 years ago

@samdark

I have PHP Version 7.2.22.

Yuriy-Svetlov commented 4 years ago

I checked again

I have PHP Version 7.2.22.

session.use_strict_mode | 1 | 1

Yuriy-Svetlov commented 4 years ago

@samdark Can you check on PHP version 7.2.22.?

samdark commented 4 years ago

Reproduced bad behavior with 7.2.22.

samdark commented 4 years ago

That's a bit weird because 7.4 works w/o it.

Anyway, it seems to be connected with the fact that DB session handler should implement validate_sid. Problem is that it's PHP 7.0+ only.

Yuriy-Svetlov commented 4 years ago

@samdark

I use before connecting user by session:

$id_session = $_COOKIE['PHPSESSID'];

if (file_exists($session_path."/sess_".$id_session) === true) {
}
samdark commented 4 years ago

@semiromid why do you check files if database handler is used?

samdark commented 4 years ago

Not sure how to handle it overall :(

Yuriy-Svetlov commented 4 years ago

@semiromid why do you check files if database handler is used?

This I showed an example how I use validation the storage of sessions in files. I gave an example of the algorithm. I think that need to do the same as I showed, but the checking should be done in database.

samdark commented 4 years ago

Where would you do it?

Yuriy-Svetlov commented 4 years ago

@samdark

Where would you do it?

This is due to:

Note* This class is not from Yii2. class PHPSessionXHandler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {

https://www.php.net/manual/ru/function.session-set-save-handler.php

I have not yet implemented storing sessions in a database, due to lack of time.

I don't know exactly where to do this in the handler. When I do this in my code, then I will know where it can be done in Yii2.

Maybe need to look at: yii\web\DbSession or yii\web\Session

    public function open()
    {
        if ($this->getIsActive()) {
            return;
        }

        $this->registerSessionHandler();

        $this->setCookieParamsInternal();

        // Before session_start need validate that the session exists in db. I do not know exactly. Maybe I'm wrong, what it need do here.
        YII_DEBUG ? session_start() : @session_start();

        if ($this->getIsActive()) {
            Yii::info('Session started', __METHOD__);
            $this->updateFlashCounters();
        } else {
            $error = error_get_last();
            $message = isset($error['message']) ? $error['message'] : 'Failed to start session.';
            Yii::error($message, __METHOD__);
        }
    }

I need time to figure this out.

samdark commented 4 years ago

While code seems to be good, I need to think more about how to release it with least issues possible.

@yiisoft/reviewers suggestions are welcome.

bicf commented 4 years ago

I come later to discussion, so maybe I can be off-side. Reading the discussion I just think: why delegate to persistent system (DB, file,...) to check if the session exists? Should not be easier (and more performance) implement some sort of CRC validation of the VALUE of the cookie?

samdark commented 4 years ago

HMAC-signing of cookies is an option, yes. That would solve the issue but in some cases such as when JavaScript should be able to read cookies that won't work.

ivan-redooc commented 4 years ago

when JavaScript should be able to read cookies that won't work.

Why "won't work"?

The sessionId could be created by the concatenation the random string and the hash

samdark commented 4 years ago

@rhertogh what do you think?

bicf commented 4 years ago

Digging the documentation, I found create_sid and validate_sid callback of session_set_save_handler.

The minor is version compatibility, only since 7.0.0: 7.0.0 | Added the optional validate_sid and update_timestamp parameters. 5.5.1 | Added the optional create_sid parameter.

rhertogh commented 4 years ago

Signing the session ID won't work because a valid signed ID would be valid "forever" (actually until the signing key is changed, which isn't something you want to do unless you would like to invalidate all cookies). This means that signing only offers minimal protection since:

  1. An attacker can easily obtain valid ID by just requesting a page from the server.
  2. When a user signs out the session is destroyed, but that ID would still be valid according to it's signature.
  3. When a session expires the ID would still be valid according to it's signature.

Adding a timestamp to the signature would limit some of those problems but would introduce new problems by itself. A session ID might have to be changed during a valid session which might cause serous issues when multiple concurrent requests are made while one of them triggers the the session ID change.

bicf commented 4 years ago

@rhertogh the goal of the validation is to understand if the session id (SID) is forged or generated by the application (genuine).

So when a user signs out the session is destroyed or a session expires, the SID is genuine but no data is stored in persistent system (DB/file/....).

Yes, an attacker can easily obtain valid ID by just requesting a page from the server, but this happens in any case. At some point you need to insert the SID in DB before the check is_the_SID_stored_in_DB().

rhertogh commented 4 years ago

@bicf I can understand from the opening post of this issue it might look like that the problem is the genuineness of the SID, but that isn't the problem.

The real problem is session fixation, please check this RFC: https://wiki.php.net/rfc/strict_sessions So custom storage classes must adhere to this ini directive otherwise strict session ID mode is effectively disabled: https://www.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode

And from https://www.php.net/manual/en/sessionupdatetimestamphandlerinterface.validateid.php:

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

Therefore just checking if a SID is genuine isn't enough. It would leave the application open to attacks with genuine but invalid SID. By checking if a session exists the problem of 'custom' session id's is solved but that is just a bonus, it was never the main purpose. The main point is that the underlying attack surface is mitigated by respecting the strict mode directive.

bicf commented 4 years ago

@rhertogh I get the point, you're right.