[Tickets #7931] Re: Left Logout button throws "malicious request"

[bugs at horde.org]

DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.

Ticket URL: http://bugs.horde.org/ticket/7931
------------------------------------------------------------------------------
  Ticket             | 7931
  Updated By         | moreda at allenta.com
  Summary            | Left Logout button throws "malicious request"
  Queue              | Horde Base
  Version            | 3.3.3
  Type               | Bug
  State              | Feedback
  Priority           | 2. Medium
  Milestone          |
  Patch              |
  Owners             | Horde Developers
+New Attachment     | fix-lock-in-transactions-for-mssql.diff
------------------------------------------------------------------------------


moreda at allenta.com (2009-09-24 12:54) wrote:

Hello.

I have been struggling with a problem similar this for a few days:  
basically a non-very-deterministic behavior of the two logout links  
with the horde_logout_token in the main page. My scenario is Horde  
3.3.5 using MS SQL Server as session data storage through PEAR's DB  
abstraction layer.

Maybe this could give some ideas to others with the same problem, and  
I think it could be at least a bug report and fix for my specific  
situation.

Finally it seems to turn out that it is a race condition problem  
during the situations of simultaneous update of the session_data,  
because of DB transaction mechanisms that are not so obvious in their  
inner working in PEAR DB. The first thing that I noticed is that the  
problem arose in reloads of the main page, which has two frames, each  
one with a logout button with a horde_logout_token (those values are  
stored in the session_data field as "horde_form_secrets"). If you  
reload first one frame, then the other, everything works as expected  
(slow down one of the participants in the race so no problems appear  
:-) ).

PEAR DB uses the concept of autocommit as a way of start automatically  
a transaction, but the real transaction (issuing the "BEGIN  
TRANSACTION" to the DB) starts just with a so called "manipulation  
query" (INSERT, UPDATE, DELETE). That means that the intended atomic  
process:

  1. Read session data from DB
  2. Update session data in DB

is not actually a transaction, because the BEGIN TRANSACTION starts in  
the 2nd step.
That leads to a situation with no atomicity and hence a really nice  
"race condition" with not so really nice results.

As I didn't plan to fix anything in PEAR DB, I found a workaround that  
could be acceptable, even if it's not the most elegant way.

I forced a "manipulation query" in the _read() function at  
lib/Horde/SessionHandler/sql.php just before the 1st step of obtaining  
the session data, in order to force the begin of the transaction:

         /* Force manipulation query to effectively begin the transaction */
         $table = $this->_params['table'];
         $updatestring = 'session_id=session_id';
         $wherestring = 'session_id = ?';
         $query = sprintf('UPDATE %s SET %s WHERE %s',
                           $table,
                           $updatestring,
                           $wherestring);
         $values[] = $id;
         $result = $this->_write_db->query($query, $values);
         if (is_a($result, 'PEAR_Error')) {
             Horde::logMessage($result, __FILE__, __LINE__, PEAR_LOG_ERR);
             return false;
         }

This makes transaction work and no more race condition appeared in my  
scenario so far.
See patch attached.