Bug #44994 for CGI-Session: CGI-Session generate new session-ID forcibly

Mon Apr 13 03:21:28 2009 m-uchino [...] yetipapa.com - Ticket created

Subject:

CGI-Session generate new session-ID forcibly

I hope that CGI-Session generate new session-ID forcibly. And, I called new($dsn, undef, \%dsn_args). But, CGI-Session try to create new query-object and get cookie/query-string. This behavior is different from the old version(mybe 4.0?). OLD VERSION: new($dsn, $sid, \%dsn_args) ==> Session-ID is $sid. new($dsn, $query, \%dsn_args) ==> CGI-Session try to get session-ID by $query. new($dsn, undef, \%dsn_args) ==> CGI-Session generate new session-ID. CURRENT VERSION: new($dsn, $sid, \%dsn_args) ==> Session-id is $sid. new($dsn, $query, \%dsn_args) ==> CGI-Session try to get session-ID by $query. new($dsn, undef, \%dsn_args) ==> CGI-Session try to get session-ID by NEW query-object. I gave CGI-Session empty-string to solve this problem. I think that this should be added to a document. (Or, is this already included in a document?)

Mon Apr 13 14:56:59 2009 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #44994] CGI-Session generate new session-ID forcibly
Date:	Mon, 13 Apr 2009 14:56:29 -0400
To:	bug-CGI-Session [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

On Mon, 13 Apr 2009 03:21:29 -0400 "m-uchino@yetipapa.com via RT" <bug-CGI-Session@rt.cpan.org> wrote: Show quoted text

> Mon Apr 13 03:21:28 2009: Request 44994 was acted upon. > Transaction: Ticket created by m-uchino@yetipapa.com > Queue: CGI-Session > Subject: CGI-Session generate new session-ID forcibly > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: m-uchino@yetipapa.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=44994 > > > > I hope that CGI-Session generate new session-ID forcibly. And, I called > new($dsn, undef, \%dsn_args). > But, CGI-Session try to create new query-object and get cookie/query-string. > > This behavior is different from the old version(mybe 4.0?). > > OLD VERSION: > new($dsn, $sid, \%dsn_args) ==> Session-ID is $sid. > new($dsn, $query, \%dsn_args) ==> CGI-Session try to get session-ID by > $query. > new($dsn, undef, \%dsn_args) ==> CGI-Session generate new session-ID. > > CURRENT VERSION: > new($dsn, $sid, \%dsn_args) ==> Session-id is $sid. > new($dsn, $query, \%dsn_args) ==> CGI-Session try to get session-ID by > $query. > new($dsn, undef, \%dsn_args) ==> CGI-Session try to get session-ID by > NEW query-object. > > I gave CGI-Session empty-string to solve this problem. > I think that this should be added to a document. > (Or, is this already included in a document?)

Thanks for the report. Could you contribute a new automated test which illustrates the problem? As you say, the test should pass when run against some old version, but fail with the current version, and giving CGI::Session an empty string should fix it. Mark

Mon Apr 13 14:56:59 2009 The RT System itself - Status changed from 'new' to 'open'

Tue Apr 14 02:54:50 2009 m-uchino [...] yetipapa.com - Correspondence added

Thank you for your reply. I'm sorry, I do not have the experience that wrote automated test. However, I do not know why this was changed. I read a source code, and I found that CGI::Session generates new ID if it was empty-string. Therefore, I do not know this behavior is intentional or accidental. Because the CGI::Session does not check empty-string. 1. At line 691. _set_query_or_sid is called by load() that is called by new(). 2. At line 783. The argument is not an object(it is empty-string), then the argument is put to _CLAIMED_ID. 3. At line 704. The empty-string is 'defined data', then the new query-object is not generated. 4. At line 715. But empty-string is FALSE, then return to new(). 4. At line 71. _SESSION_ID is FALSE because load() did not set it. 5. At line 80. New ID is generated. I think that this should be made clear. And problem may be caused in the future if not exhibited in the source code and the document because source code may be updated without being careful to empty-string. (Sorry, my English is poor.)

Tue Apr 14 09:25:14 2009 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #44994] CGI-Session generate new session-ID forcibly
Date:	Tue, 14 Apr 2009 09:25:00 -0400
To:	bug-CGI-Session [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

> I think that this should be made clear. And problem may be caused in the > future if not exhibited in the source code and the document because > source code may be updated without being careful to empty-string. > > (Sorry, my English is poor.)

Thank you for the detailed response. We will look into further and see how we can improve the code and documentation. I appreciate your effort at English. It is much better than my Japanese. Mark

Sat Apr 18 21:51:46 2009 RSAVAGE [...] cpan.org - Correspondence added

Hi I've traced thru the list of line numbers supplied by OP (Original Poster), and I agree that at line 80 a new ID is generated. However, I'm confused by this earlier remark: "new($dsn, undef, \%dsn_args) ==> CGI-Session try to get session-ID by NEW query-object." I do not see how a new query-object is generated by the code starting a line 80. So, I'd say the new version behaves in the same way as the old version. There is a problem in the POD in the docs for new(): Nothing is said as to what happens when the expression $query||$sid evaluates to false. Can OP please tell us why they think a new query object is created. Once we settle that problem, then I will know how to update the POD. Also note: Since I'm not one of the original developers of this module, I do not get all the postings via RT. Feel free to copy me explicitly on each posting, otherwise I'll only see them when I happen to log on to RT. My email address is ron@savage.net.au.

Sat Apr 18 23:01:13 2009 m-uchino [...] yetipapa.com - Correspondence added

Thank you for your reply, and thank you for your checking my report. When empty-string was specified, the CGI::Session behaves as above. When undef was specified, the CGI::Session behaves as follows. 1. At line 691. _set_query_or_sid is called by load() that is called by new(). 2. At line 783. The argument is not an object(it is undef), then the argument is put to _CLAIMED_ID. 3. At line 704. The undef is NOT 'defined data', then query() called, and the new query-object is generated in query(). * empty-string is 'defined data'. 4. At line 715. If ID not found by this query-object, then return to new(). The old version doesn't support generation new CGI-object. Therefore old version generates new ID even if whichever is specified(empty-string or undef). I think that, it is convenient that a new version generates new CGI-object, but the programmer must know this information when he wants safe new ID. The programmer may not notice that it is not safe new ID.

Sat Apr 18 23:46:27 2009 RSAVAGE [...] cpan.org - Correspondence added

Hi On Sat Apr 18 23:01:13 2009, m-uchino@yetipapa.com wrote: Show quoted text

> 3. > At line 704. > The undef is NOT 'defined data', then query() called, and the new > query-object is generated in query(). > * empty-string is 'defined data'.

Yes, you are correct. I did not appreciate that the call to query() at line 705 would generate a new CGI object. Of course, if you are trying to use CGI::Simple or some other module, instead of CGI, that creates another problem :-(. Show quoted text

> The old version doesn't support generation new CGI-object. Therefore old > version generates new ID even if whichever is specified(empty-string or > undef). > I think that, it is convenient that a new version generates new > CGI-object, but the programmer must know this information when he wants > safe new ID.

OK. Here's what I suggest as a change to the POD: -------------- A special situation arises when the expression $query||$sid evaluates to false: =over 4 =item A new CGI object is created Of course, if you are trying to use CGI::Simple or another module, instead of CGI, this might create a new problem, since this new object is stored inside the current CGI::Session object. Also, early versions of this module did not do this, so this change in behaviour might be confusing. =item This new object is asked to provide a SID Of course, the SID returned by this new object will be empty. =item The current CGI::Session object then generates a new SID This is what you wanted. However, if you already have, or later create, a CGI - or similar - object, you need to be aware that the current CGI::Session object is now using its own CGI object. This will almost certainly lead to confusion. =back So, the recommended way to avoid this is to always call new() with either a query object or a sid, so that the expression $query||$sid is never false. --------- I've changed my own SVN version of the code to contain this patch. However, before checking it in I will wait for comment from other developers. As for the module creating a object of type CGI and not of some other type, there's nothing we can do, because when the user does not provide a query object, we have no way of knowing what type of object they prefer.

Sun Apr 19 03:11:36 2009 m-uchino [...] yetipapa.com - Correspondence added

RT-Send-CC:

ron [...] savage.net.au

Thank you for your response. I worry about security. I think that this method to let the CGI::Session generate new SID forcibly by specifying an empty-string is written in POD is better. Similarly I think that the comment is written in a source code is better for that somebody does not change this behavior that CGI::Session generate new SID if it is empty-string. Because, whether using CGI::Simple or CGI, we must sometimes let the CGI::Session generate new SID. For example, member login page. The attacker is going to steal the secret information of the user. Therefore he is going to steal SID. You say "Of course, the SID returned by this new object will be empty.", but it return SID if it finds the SID. Therefore, the attacker can easily steal the information of the target. For example, follows. 1. The attacker accesses the member's page. He may need an account or may not need it. Anyway, he lets CGI::Session make his session. 2. The attacker can get SID of his own by reading HTTP header. Set-Cookie: CGISESSID=xxxSIDxxx; path=/ SID is "xxxSIDxxx". 3. The attacker guides the target to this URL by some ways. https://host/members/login.cgi?CGISESSID=xxxSIDxxx 4. The taget uses the session of the attacker if the target does not have COOKIE. He will log in then if a login page is displayed. And the session becomes his session. However, the SID is the same. 5. The attacker can use the session that the USER-ID of the target was set. Or the attacker may steal COOKIE of the target. If CGI::Session tries that it gets SID from COOKIE, the target is dangerous in the same way. If a system checks that a user has already logged in, the attacker will guide a target after logging out. If a system deletes a session if a user logs out, this attack will fail. Anyway, the method how we easily avoid this attack is to let the CGI::Session create new SID forcibly when a user logged in.