New Session library

[narfbg]

It can be found in the feature/session branch and is the last thing I want finished before the 3.0 release. To everybody reading this: please test it and give some (useful) feedback. :) Tests with non-mysql DBs in particular would be really useful.

Implemented features:

• Locking (this is mandatory, no drivers without locking will be implemented; locking for non-MySQL/Postgre DBs though is done through sysvsem, which means there's no locking on Windows)
• Lazy writes (only write data to storage if it was changed)
• 'files' & 'database' drivers

Planned/todo features & other stuff that might get implemented:

• Redis & Memcache(d) drivers
• Not sure if extending the library works at this time, but this certainly needs more work
• Optional session data encryption
• Other drivers?
• Tests? (we've had issues with PHPUnit burping with "headers already sent" messages when we send cookies)
• PHP should validate incoming session IDs (they need to be all alphanumeric chars), but still - double check that to avoid SQL injections
• Automatic regeneration of IDs
• session.use_strict_mode functionality

BC breaks:

• Flash/temp/user-data can collide & are all actual $_SESSION vars
• Changed table structure for database storage
• No cookie storage (no, it will not happen, you must use server-side storage)
• No user-agent matching - it's user input and therefore ineffective and pointless
• Database storage can't be used with a persistent connection (to avoid deadlock and to allow concurrency)

Issues/PRs that this resolves/supersedes:

• 1344

• 1375

• 1780

• 1940

• 2476

• 2702

• 2923

[aanbar]

Could you please provide the new database schema, And update the config file so it reflects the new changes.

[narfbg]

This should allow configuring the driver: 34b1ef5c13882c4a7827be71e82503ee47d4c271 Other config changes are not relevant, the worst thing that could happen is having an extra setting that does nothing. ;)

Here's a working DB schema, for MySQL:

CREATE TABLE `ci_sessions` (
  `id` varchar(40) NOT NULL,
  `ip_address` varchar(45) NOT NULL,
  `timestamp` int(11) NOT NULL,
  `data` blob NOT NULL,
  PRIMARY KEY (`id`),
  KEY `ip_address` (`ip_address`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Note: If you're using IP address matching, you'll have to make the primary key a composite of id, ip_address.

[GDmac]

In the past i looked into MySQL lock, there might be issues when using persistent connections. I debugged with viewing mysql server processes in SequelPro and use a bit of sleep() to see the processes pop up during refresh intervals.

[narfbg]

Good thing you can't use a persistent connection with the library then. :)

[narfbg]

Turns out locking was broken and is fixed by the above referenced commit.

[aanbar]

Message: Undefined variable: result

Filename: drivers/Session_files_driver.php

Line Number: 198

[a-h-abid]

set_flashdata() has issue. It is overwriting any provided value with value 'new' in current request, 'old' on next request.

Edit: also the method flashdata() doesn't seem to return any of values. Always returning null.

[narfbg]

Try now, but note that the only issue is what you've mentioned in your "Edit" line. Flash/temp data is userdata, what you're seeing as 'new', 'old' are just markers.

[a-h-abid]

Working Now.

[saada]

:+1: Really exciting, guys! Cannot wait!

[narfbg]

Has anybody tested extending the library? And if so - in what scenarios?

[ivantcholakov]

I saw this error once, then it disappeared:

A PHP Error was encountered Severity: Warning Message: filemtime() [function.filemtime]: stat failed for ac584bae09e0ba6ee999bb4f9a4f9924fc3202ae152fa28f Filename: drivers/Session_files_driver.php Line Number: 264

[narfbg]

Yes, it's natural that it doesn't appear often, because gc() is triggered by chance and doesn't run on every request. I'd have to dig into php-src to see what can be causing this ... that error message is far from descriptive. Could be a simple permission issue though.

[ivantcholakov]

@narfbg Just a thought, you may ignore it: What about the 'files' session driver to be renamed as 'filesystem' driver? For tutorials and articles maybe it would sound better - you may choose between the 'database' driver and the 'filesystem' driver.

[ivantcholakov]

About the method gc(), the 'files' driver:

Some Linux distributions have a cron task that deletes old session files. Within a tiny fraction of time, between the check is_file() and the filemtime() call, the target file could be deleted by the external cron task. It is very low probability to happen, but not zero. Maybe this caused the warning message from filemtime().

I suppressed possible errors from filemtime() and unlink() assuming that the old session file might be deleted at any moment while the method gc() runs. This is a wild speculation, it may be wrong.

[narfbg]

That would be very dumb on the distro maintainers' part, even if noble, so I highly doubt this is the cause. Where did you get that info from?

[ivantcholakov]

I googled a little: http://stackoverflow.com/questions/654310/cleanup-php-session-files http://www.laurencegellert.com/2012/08/php-sessions-not-being-deleted-on-ubuntu-server/ http://serverfault.com/questions/422723/ubuntus-garbage-collection-cron-job-for-php-sessions-takes-25-minutes-to-run-w http://serverfault.com/questions/138214/are-php-session-files-ever-deleted http://budiwijaya.net/tips-to-delete-sessions-file-of-php-in-centos-5-5.html

It seems to me that Debian/Ubuntu ditributions have such configuration. CentOS is likely not to have such a cron task by default, but who knows, it could be added by the maintainers. I am not sure, I am just guessing.

[narfbg]

A few years back distro maintainers knew what they were doing ... I'll think about how to get around this, there has to be a better way than error suppression.

[kakysha]

@narfbg why stop supporting cookie driver? It is rather handy to store session that belongs to this only user on the client side, isnt it? If the reason is the ability to modify sessions by users - this can be resolved by cookie ecnryption, isn't it? Especially with the new Encryption lib. For me I dont see any valuable reason to drop cookie support. But if you will do it - I will implement Redis driver then, because right now I rely on cookie driver and dont want to use db (read-only slaves problem) / files (speed performance) :)

[narfbg]

It's simple: trusting client-side data both as a storage and authentication mechanism is not secure.

Encryption allows us to get away with doing that while still maintaining a level of security (yet still not as safe as server-side storage), but encryption is:

• hard to implement & deploy properly (key management included)
• performance-intensive (relatively, we're talking sessions here)
• not widely available; you'd have to have either mcrypt or openssl extensions, possibly a newer (than the general 5.2.4 requirement) PHP version too

A possible bug in the crypto implementation would also mean a security flaw in sessions too. Also, cookies have a size limit, must be sent to the client prior to other output, etc. It might've made sense in the old days where hosting providers would charge you for absolutely everything (and thus, cookies were a cheap hack), but in today's world it doesn't.

I have a pre-alpha Redis driver, so you don't need to write one from scratch. Just don't want to publish it until it's at least runnable ... You'll be welcome to review it afterwards. :)

You do also have the option of using file-system storage + tmpfs, which is blazing fast. :)

As I've previously noted, what I really need help with is hands-on tests of the more ... custom use cases - extending the library, using your own drivers, etc.

[kakysha]

@narfbg agree, ok :) Cant actually test that things you are interested in because dont really need it, but do need Redis driver for my production environment, so, on this way I can help. Maybe you can gist it if it is not be published in the near future?)

[ivantcholakov]

@narfbg

"Database storage can't be used with a persistent connection (to avoid deadlock and to allow concurrency)"

Maybe it would be better within the file config/database.php the configuration setting 'pconnect' to be set to FALSE. And within the comments in the file, a note for this setting to be added - "Database storage can't be used with a persistent connection (to avoid deadlock and to allow concurrency)".

When people play with the framework for the first time, they likely would not read the documentation so deeply, so let the default settings to be as safe as it is possible.

[narfbg]

pconnect is already FALSE in this branch. This will be noted in the Session library docs though, not the database config file.

[filhocodes]

@narfbg I added the file path in this commit of my fork and, after that, filemtime stops generate a warning in the GC function of files driver.

[narfbg]

Oh, great ... seems like it's not as complicated as we thought ... I've cherry-picked your commit. Thanks!

[filhocodes]

:+1:

[Quiss]

I understand because @marcossffilho Soon CI 3.0? =)

[gsuberland]

I'm in the process of writing a CSPRNG wrapper that, in 99% of installations, will provide cryptographic strength random numbers out of the box. It defaults first to /dev/urandom if available, then to OpenSSL's PRNG, and finally to MCrypt's PRNG. There's an option to enable insecure fallback to a solution based on microtime() and mt_rand(), using SHA1 for mixing, with various server information and a configurable key value (similar to the encryption key from CI_Encrypt) for good measure, but that isn't enabled by default as it isn't guaranteed safe. The individual sources can be enabled or disabled at will. If all prerequisites fail, it refuses to initialise.

In 2.2.0 the session ID is generated from an insecure PRNG that is known to have limited output. My proposition is that, once properly reviewed and accepted, the CSPRNG library is used to generate session IDs in the new Session class. Furthermore, it would be ideal for use in generating CSRF tokens and other secure identifiers. The library will export a range of convenient methods suitable for these purposes, as well as others.

Let me know if this is desirable, or if you have any concerns.

[narfbg]

Thanks for the effort @gsuberland, but no ... if you had looked at the feature/session branch, you'd know that such a library would be unnecessary.

[gsuberland]

Hi @narfbg.

I did look at the feature/session branch, but didn't see a reference to where session IDs were being generated, so assumed that part either hadn't been written yet or was being recycled from the 2.2.0 code. Are you planning on utilising PHP's native session ID generation?

As for CSRF tokens and other secure identifiers, what are the plans for migrating them away from the weak mt_rand() based generation code from 2.2.0?

[narfbg]

You didn't read carefully, it seems.;-)

The whole point here is to use PHP's built-in session capabilities instead of the custom piece of crap that is in previous CI versions, so yes - PHP itself is now responsible about session ID generation.

On CSRF... It's the most basic type of protection and anybody can beat it regardless of the PRNG in use, you simply don't need to predict anything to do that.

[narfbg]

In other words, there are no plans for CSRF. You're welcome to improve it, but it certainly doesn't mandate a CSPRNG library, nor is randomness it's weakest spot. :-)

[gsuberland]

It seems I did miss that out. My apologies.

As for your comment on CSRF protection, could you elabourate please? As far as I can see, the tokens seem to be properly checked, so bypassing the checks seems unlikely unless the tokens can be guessed or predicted in some way. Surely, if the CSRF protection implementation was so broken that "anybody can beat it regardless", then it needs a complete overhaul? Or are you referring to some property of token-based CSRF protection in general that I have missed?

[narfbg]

I'm referring to the fact that the token is stored in a user-supplied cookie ... Even if that wasn't the case, the nature of it all (that token must somehow be tied to a browser, which always comes down to a cookie) is such that predicting the token is an effort far greater than what you'd otherwise need to beat the CSRF mechanism.

Anyway, as I said - you're welcome to improve it if you wish to, I just don't see it as something that requires strong randomness.

[gsuberland]

I think you're missing a key point regarding what the attacker has access to.

The threat model for CSRF is a malicious party causing a user unwittingly to perform an action under the context of their own session. In order to trigger such an exploit against a protected form, an attacker must either (a) know a valid CSRF protection token or (b) be able to bypass the CSRF checks in some way.

The safest model is to store the validation token in the session variables, which ensures that the token is tied to the current session, and the attacker cannot gain access to the token value through that route, nor can he change that value. As long as tokens from the user's session aren't leaked in any other way (e.g. XSS, though at that point you're screwed anyway) then you're safe. Note that, due to the same origin policy (SOP) it isn't possible for a page to embed the target site in an iframe and read the inner document from the outer document. The SOP also prevents the same trick in the context of Ajax.

I'm guessing your concern is that an attacker can change the CSRF token. This isn't actually a valid attack vector. The cookie mechanism that CI current uses is known as the "double sumbit cookie" model. The crux of its security is that, again due to SOP, other sites cannot set a cookie with a domain scope outside its origin, e.g. "microsoft.com" can't set a cookie for "google.com" or vice versa. An attacker might modify the cookie in his own browser, but that only allows him to exploit himself, which is meaningless in the context of CSRF. For a valid attack, the attacker must be able to set the CSRF cookie for the target domain in the target user's browser, which isn't possible without secondary vulnerabilities.

As such, the weak point at the moment in fact is the lack of strong randomness.

[narfbg]

I'm not missing anything, my point is that SOP is, unfortunately, rarely implemented while at the same time the only system immune to XSS is one that's not on the web. I'm sure you're well aware of the latter and let's be realistic - that's what anybody would try to exploit first.

I get what you mean of course, and I already said this twice: you're welcome to improve it, I won't reject such contribution. Continuing this conversation further is pointless, not least because this issue isn't about CSRF at all.

[gsuberland]

You're very much mistaken about SOP. It's been part of all modern browsers for years. You should certainly read up on it if you're working on this framework.

As far as XSS goes, I don't agree. I did personal research in the security field for the last decade, and spent the last 18 months working as a professional pentester, and I can honestly say that many new sites aren't vulnerable to XSS. For example, ASP.NET's MVC framework actually makes it hard work to introduce an XSS vulnerability, as all content is contextually escaped for the target view. Even huge business management applications that I have tested have come out clean for XSS, and they have thousands of input fields. People are getting it right.

Furthermore, your assertion seems odd. Why would you not provide a good quality standard security measure, just on the off-chance that an XSS is found? That's not the way to do security.

Anyway, I'll be writing the CSPRNG library anyway, as it's useful to applications, and can be used for CSRF protection too. I might go and issue a patch to allow for both double-submit and session-based protection while I'm at it; just depends how much time I have.

[narfbg]

I don't get it, what's your goal here really? To convince me to commit it myself? To convince me that it's you who knows better? Or that I should start caring more about that? Trying to teach me something? I welcome your report and I'd welcome your fix for it, but I don't appreciate you continuosly talking down on me, especially after I asked you to stop this conversation.

Fixing it would've wasted you (and me) less time than trying to convince me into anything. Disagree all you want, but as some previous contributor said it: XSS is a cat & mouse game. Even if people are getting it right, every browser release is a potential new XSS flaw.

If you're a pentester, I understand that it's probably part of your job to educate people about security practices, but the fact is - I didn't write that piece of code. If I had written it today, I wouldn't use mt_rand(). I've barely had time to work on this (sessions) high-priority task in the past 3 months and call me irresponsible, but I simply don't care about anything else right now.

I'll say this for the FORTH time: YOU'RE WELCOME TO IMPROVE IT! If I had reported a similar issue in another project, I'd patch it myself and I'd be happy to appear in their commit log.

[purplebeanie]

Hi @narfbg great work on the feature/session. Worked straight away after making the db changes, changing the sess_driver to database and setting pconnect to false (the app was initially developed on CI 2).

[narfbg]

Just added an untested version of the Redis driver.

@kakysha + other volunteers: Would you give some feedback on it? :)

[ivantcholakov]

A PHP Error was encountered

Severity: Warning

Message: Redis::setTimeout() expects exactly 2 parameters, 3 given

Filename: drivers/Session_redis_driver.php

Line Number: 156

[ivantcholakov]

The error disappeared.

[narfbg]

Just added a Memcached (distributed version only) driver as well. I'll add an ext/memcache (non-distrubuted) version when we stabilize this one.

[narfbg]

@espradley You had #2429 opened about Redis & Memcached sessions some time ago, so I'm linking you in case you want to try this out. :)

[mrtomtom]

This looks fantastic -- Is there a rough timeline for when it will be closed off, looks like last issue before 3.0 is released? :)

[narfbg]

There's no timeline, there's just stuff to be done and I can't do all of it.

With all the "when is CI 3 ready?" questions, I'd expect more help from the community with this, yet I can't even get a few people to test this thing ...

[orionstar]

@narfbg I tried out the feature/session branch and it works for me in a test app. Exactly what kind of feedback do you need about it?

[narfbg]

@orionstar I need somebody who works with Redis & Memcached to test it, and possibly to try to extend the library.

[kakysha]

@narfbg will test Redis driver and try to extend it today.

[Shakespeare2000]

Can the feature/session branch be used for testing the database driver? If so, I'm running into two problems:

• _get_lock() (Session_database_driver.php, line 119) is missing an argument ($session_id I think)
• unset_userdata gives warning: Illegal offset type in unset (Session.php, line 570). Shouldn't $_SESSION[$k] be unset, instead of $_SESSION[$key]?