Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Additional data stored in session cookies #1344

Closed
narfbg opened this Issue · 16 comments

6 participants

@narfbg
Owner

I just read this question on stackoverflow: http://stackoverflow.com/questions/10512833/why-does-pyrocms-store-so-much-data-in-its-cookie/10513223 and to be honest - I can't think of a reason to really store the IP address, user-agent string, etc. in the cookie itself.
Is this by design and/or can we change that?

@philsturgeon @derekjones @pkriete @ericbarnes

@toopay

The only solution i can think of, was to support/provide "other drivers" for Session storage (that share same API with the current session driver, so switching driver would not break anyone's app) and dev could choose whether to store the session on local storage or server-side. Hard to imagine, to deprecating/remove that behaviour completely.

@philsturgeon

Is this related to what you're doing @alexbilbie?

@alexbilbie

Is the intention of the IP address, UA etc not used for matching to detect session cookie hijacking?

@alexbilbie

In terms of whether or not a user agent and IP address are private information, I'm in contact with the UK Information Commissioners Office and am awaiting an answer from them.

@dchill42

The merge of #353 has implemented what @toopay suggested, so any developer can plug in a variant driver with alternate means of hijack detection and/or data storage. Plus, it includes a native PHP session driver which reduces the cookie to a session ID only.

@narfbg
Owner

... which reduces security and doesn't resolve the issue in the original Session library. At the time I posted this I didn't realize that with no server-side storage, there's no way of checking on those attributes. So I guess the question is - can we avoid storing IP addresses and UA strings in both cookies and the database at the same time?

@dchill42

Off the top of my head, I'd suggest only including ip_address and user_agent if the sess_match_ip and sess_match_useragent config items (respectively) are enabled. Thus, if the user doesn't elect to rely on those items matching (for better or for worse), they aren't stored at all. By the same token, we wouldn't break existing apps which may be expecting those values to be present in the session data (or at least only require them to update config).

That would be a very simple change to make in both of the (now) existing drivers. Is there any consensus on that solution?

@narfbg narfbg referenced this issue
Closed

Develop #1888

@alexbilbie

@dchill42 I've noticed that on line 158 of Session_native.php that what you've suggested is the case anyway

@dchill42

I may have thought that was a good enough idea to include in the native driver. ;-)

In the cookie driver (which I left as-is as much as possible), that implementation would take a different form, however. Right now, it stores the ip_address and user_agent in the cookie, regardless of whether matching is enabled for either, nor whether the database is in use. The change would be to only store them in the actual cookie if it's necessary - matching enabled and no db. We would also have to change line 401 of Session_cookie.php to only use those values for validating the cookie if they've been included by the aforementioned logic.

@narfbg
Owner

The more I look at this and the more I wonder why does this data has to ever be stored in the cookie, even with no server-side storage used. The IP address and the user-agent string both exist with every HTTP request.

@alexbilbie

But if they weren't stored how you match them? I suppose you could store a hash?

@narfbg
Owner

That's one of the reasons why I've always thought CI's Session class is weird - I've never seen another sessions implementation that doesn't use any kind of server-side storage. A hash that represents somekind of a fingerprint is not a bad idea, but still kind of pointless.
The only thing that we can really rely on is the encryption key, if encryption is used at all. Everybody else can just build their own cookie and since we have nothing on our side to actually track it, we can only say if it's valid or not.

@dchill42

That's true - without encryption, the IP and UA in the cookie are pointless. It's the equivalent of accepting a passport from someone's own private island nation. Of course the credentials check out if you made them yourself and control the validation source. You have to have a protected server token in the cookie or have credentials stored on the server in order to have an objective validity test.

This was referenced
@jim-parry
Owner

This sounds like it has been satisfactorily addressed, i.e. moved/deferred to the new session library (#3073). Is that the case? Can this issue be closed?

@narfbg
Owner

It will be closed when #3073 is closed.

@jim-parry jim-parry removed the Dead Issue? label
@narfbg
Owner

Fixed by #3073.

@narfbg narfbg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.