OWASP / phpsec

OWASP PHP Security Project - THIS PROJECT IS INACTIVE AND MAY CONTAIN SECURITY FLAWS
197 stars 103 forks source link

auto removal of outdated session and their data in some time interval #71

Closed rash805115 closed 10 years ago

rash805115 commented 10 years ago

Right now, if a user stores some session data and never accesses it, then it will be present there forever until the user tries to access it again, at which point, the user sees that the session has expired, and deletes that session.

But if the user never accesses that data, then it is unnecessarily storing that data. So, we can remove that data monthly or weekly. For this, a PHP script needs to be written

abiusx commented 10 years ago

There should be a timeout as part of the data, I believe.


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 16, 2013, at 2:52 AM, Rahul Chaudhary notifications@github.com wrote:

Right now, if a user stores some session data and never accesses it, then it will be present there forever until the user tries to access it again, at which point, the user sees that the session has expired, and deletes that session.

But if the user never accesses that data, then it is unnecessarily storing that data. So, we can remove that data monthly or weekly. For this, a PHP script needs to be written

— Reply to this email directly or view it on GitHub.

shivamdixit commented 10 years ago

Hi

Can we use an event scheduler like cron and automatically execute a query say in every one week to delete the sessions for which last activity is more than 7 days before ?

On Wed, Oct 16, 2013 at 8:05 PM, AbiusX notifications@github.com wrote:

There should be a timeout as part of the data, I believe.


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 16, 2013, at 2:52 AM, Rahul Chaudhary notifications@github.com wrote:

Right now, if a user stores some session data and never accesses it, then it will be present there forever until the user tries to access it again, at which point, the user sees that the session has expired, and deletes that session.

But if the user never accesses that data, then it is unnecessarily storing that data. So, we can remove that data monthly or weekly. For this, a PHP script needs to be written

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-26423033 .

Cheers, Shivam

abiusx commented 10 years ago

We sure can, but its not a good idea to require cronjobs for a web application to work properly. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 16, 2013, at 11:36 AM, Shivam Dixit notifications@github.com wrote:

Hi

Can we use an event scheduler like cron and automatically execute a query say in every one week to delete the sessions for which last activity is more than 7 days before ?

On Wed, Oct 16, 2013 at 8:05 PM, AbiusX notifications@github.com wrote:

There should be a timeout as part of the data, I believe.


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 16, 2013, at 2:52 AM, Rahul Chaudhary notifications@github.com wrote:

Right now, if a user stores some session data and never accesses it, then it will be present there forever until the user tries to access it again, at which point, the user sees that the session has expired, and deletes that session.

But if the user never accesses that data, then it is unnecessarily storing that data. So, we can remove that data monthly or weekly. For this, a PHP script needs to be written

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-26423033 .

Cheers, Shivam — Reply to this email directly or view it on GitHub.

mebjas commented 10 years ago

I have a suggestion, whenever the newSession($userId) function (session library) is called for a particular userID if we delete all rows for that userId prior to insertion we automatically delete all previous sessions stored for that id. the code looks like this **

      public function newSession($userID)
        {
           if ( ($userID == null) || ($userID == "") )
                  throw new NullUserException("ERROR: UserID cannot be null.");
          $this->userID = $userID;
          $this->session = randstr(128); //generate a new random string for the session ID of length 32.
          $time = time(); //get the current time.
          SQL("DELETE FROM SESSION WHERE `USERID` = ?",array($this->userID));   //delete all previous session data for this user
          SQL("INSERT INTO SESSION (`SESSION_ID`, `DATE_CREATED`, `LAST_ACTIVITY`, `USERID`) VALUES (?, ?, ?, ?)", array($this->session, $time, $time, $this->userID));
          $this->updateUserCookies();
          return $this->session;
      }

**

rash805115 commented 10 years ago

Suppose there is the case that user is using the website from two different computers. Then he has two sessions with the same userID. Now in one computer the session expires, then in process of generating a new session, you are removing the session and its data for the second computer also!!

On Sun, Oct 27, 2013 at 1:39 PM, minhaz notifications@github.com wrote:

I have a suggestion, whenever the newSession($userId) function (session library) is called for a particular userID if we delete all rows for that userId prior to insertion we automatically delete all previous sessions stored for that id. the code looks like this **

  public function newSession($userID)
    {
       if ( ($userID == null) || ($userID == "") )
              throw new NullUserException("ERROR: UserID cannot be null.");
      $this->userID = $userID;
      $this->session = randstr(128); //generate a new random string for the session ID of length 32.
      $time = time(); //get the current time.
      SQL("DELETE FROM SESSION WHERE `USERID` = ?",array($this->userID));   //delete all previous session data for this user
      SQL("INSERT INTO SESSION (`SESSION_ID`, `DATE_CREATED`, `LAST_ACTIVITY`, `USERID`) VALUES (?, ?, ?, ?)", array($this->session, $time, $time, $this->userID));
      $this->updateUserCookies();
      return $this->session;
  }

**

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27174292 .

Regards, Rahul Chaudhary Ph - 412-519-9634

abiusx commented 10 years ago

Its simpler than that. Supposed that a user leaves and never comes again. The session record will stay forever. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 1:42 PM, Rahul Chaudhary notifications@github.com wrote:

Suppose there is the case that user is using the website from two different computers. Then he has two sessions with the same userID. Now in one computer the session expires, then in process of generating a new session, you are removing the session and its data for the second computer also!!

On Sun, Oct 27, 2013 at 1:39 PM, minhaz notifications@github.com wrote:

I have a suggestion, whenever the newSession($userId) function (session library) is called for a particular userID if we delete all rows for that userId prior to insertion we automatically delete all previous sessions stored for that id. the code looks like this \

public function newSession($userID) { if ( ($userID == null) || ($userID == "") ) throw new NullUserException("ERROR: UserID cannot be null."); $this->userID = $userID; $this->session = randstr(128); //generate a new random string for the session ID of length 32. $time = time(); //get the current time. SQL("DELETE FROM SESSION WHERE USERID = ?",array($this->userID)); //delete all previous session data for this user SQL("INSERT INTO SESSION (SESSION_ID, DATE_CREATED, LAST_ACTIVITY, USERID) VALUES (?, ?, ?, ?)", array($this->session, $time, $time, $this->userID)); $this->updateUserCookies(); return $this->session; }

\

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27174292 .

Regards, Rahul Chaudhary Ph - 412-519-9634 — Reply to this email directly or view it on GitHub.

mebjas commented 10 years ago

OK in that case we can delete those session data which were created before a certain fixed period of time, while creating a new session row in db. But considering what Abbas mentioned, its going to leave one row per such user which is wrong. Any other good alternative?

abiusx commented 10 years ago

time based sweeping. Every 10 minutes, sweep all sessions that are expired. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 2:01 PM, minhaz notifications@github.com wrote:

OK in that case we can delete those session data which were created before a certain fixed period of time, while creating a new session row in db. But considering what Abbas mentioned, its going to leave one row per such user which is wrong. Any other good alternative

— Reply to this email directly or view it on GitHub.

mebjas commented 10 years ago

How will this functionality be implemented, if we are not using an event scheduler?

rash805115 commented 10 years ago

actually in jFramework I saw what Abbas did there....he made a function that works with probability. When probability is higher than the base probability, the function does the sweeping and removing outdated records...that approach is much easier and effective..

On Sun, Oct 27, 2013 at 2:04 PM, AbiusX notifications@github.com wrote:

time based sweeping. Every 10 minutes, sweep all sessions that are expired. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 2:01 PM, minhaz notifications@github.com wrote:

OK in that case we can delete those session data which were created before a certain fixed period of time, while creating a new session row in db. But considering what Abbas mentioned, its going to leave one row per such user which is wrong. Any other good alternative

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27174912 .

Regards, Rahul Chaudhary Ph - 412-519-9634

mebjas commented 10 years ago

but when is this function called? @rash805115

abiusx commented 10 years ago

That is one way of doing it, always calling this function when checking for a session (on every app launch):

if (random()<0.1) //%10 chance do the stuff here

but that is not very wise, and yet its not bad, since in an app with a lot of requests, it happens a lot of times, but on an app with very few requests (one every 5 minutes) it happens too few.

the better way would be to store the last time we sweeped using the options library, and check the current time with that, and if the difference is considerable (e.g 10 minutes), do the sweep operation.

-A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 2:11 PM, Rahul Chaudhary notifications@github.com wrote:

actually in jFramework I saw what Abbas did there....he made a function that works with probability. When probability is higher than the base probability, the function does the sweeping and removing outdated records...that approach is much easier and effective..

On Sun, Oct 27, 2013 at 2:04 PM, AbiusX notifications@github.com wrote:

time based sweeping. Every 10 minutes, sweep all sessions that are expired. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 2:01 PM, minhaz notifications@github.com wrote:

OK in that case we can delete those session data which were created before a certain fixed period of time, while creating a new session row in db. But considering what Abbas mentioned, its going to leave one row per such user which is wrong. Any other good alternative

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27174912 .

Regards, Rahul Chaudhary Ph - 412-519-9634 — Reply to this email directly or view it on GitHub.

rash805115 commented 10 years ago

Oh....the function is called in the constructor itself, but it will not run every time...its running depends on the probability which in-turn depends on time....

On Sun, Oct 27, 2013 at 2:23 PM, minhaz notifications@github.com wrote:

but when is this function called? @rash805115 https://github.com/rash805115

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27175523 .

Regards, Rahul Chaudhary Ph - 412-519-9634

mebjas commented 10 years ago

ok waiting for its implementation in this library :+1:

abiusx commented 10 years ago

Why wait? go ahead and do it yourself :D


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 2:39 PM, minhaz notifications@github.com wrote:

ok waiting for its implementation in this library

— Reply to this email directly or view it on GitHub.

shivamdixit commented 10 years ago

On Sun, Oct 27, 2013 at 11:54 PM, AbiusX notifications@github.com wrote:

the better way would be to store the last time we sweeped using the options library, and check the current time with that, and if the difference is considerable (e.g 10 minutes), do the sweep operation.

Abbas, please correct me if I am going on wrong track. We are going to store* the _Time we last "sweeped" in the database itself,_ so to find the difference of current time and last sweep time we will have to make acall to database

  • and then do comparison, and then again we will have to make a request to db to sweep data if difference is less. Instead we can directly sweep the data everytime, it will require only one request to database.

Cheers, Shivam

abiusx commented 10 years ago

database operations are not like that. When you access some record frequently, it will be stored on the memory. On top of that, sweeping sessions is a heavy operation, involving many records and sometimes updating times. If the performance is an issue, the chance model (random chance) would be handy, but I strongly doubt that it will. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 3:18 PM, Shivam Dixit notifications@github.com wrote:

On Sun, Oct 27, 2013 at 11:54 PM, AbiusX notifications@github.com wrote:

the better way would be to store the last time we sweeped using the options library, and check the current time with that, and if the difference is considerable (e.g 10 minutes), do the sweep operation.

Abbas, please correct me if I am going on wrong track. We are going to store* the _Time we last "sweeped" in the database itself,_ so to find the difference of current time and last sweep time we will have to make acall to database

  • and then do comparison, and then again we will have to make a request to db to sweep data if difference is less. Instead we can directly sweep the data everytime, it will require only one request to database.

Cheers, Shivam — Reply to this email directly or view it on GitHub.

mebjas commented 10 years ago

@shivamdixit deleting those entries from db which might have exceeded the timeLimit would take some time (and resources) and if this request is processed for every call to functions like newSession() or other it will be a consuming process. Making call after 10minutes or specific time seems a lighter process than that!

paulocmguerreiro commented 10 years ago

Why not call this function at Session::destroySession , and use it to erase this session data and all that are expired. This way whenever a user logouts we will use this to clean up the sessions. This could be done with only one db query. The time it will take to execute is not an issue, because the user is disconnecting his sessions, so if he waits a couple seconds more he won't complaint . Even so, i think it will be fast because theres one a few expired sessions (i beblieve most of the users normally logout :) )

rash805115 commented 10 years ago

ok..so this approach has couple of meanings: 1) the user logs out and he deletes all his previous sessions which are expired. Pretty Good. But the problem still is there. Thousands of customers come and their sessions are set. And then they dont come again. So, their sessions will be in DB forever.

2) the user logs out, and he runs the function that will clear out all of the expired sessions. Good again. Until you consider this: 1000 users logs out at once (which happens in real scenario), then this function will be called 1000 times, even though our work was done in the first time.

On Sun, Oct 27, 2013 at 6:11 PM, Paulo Guerreiro notifications@github.comwrote:

Why not call this function at Session::destroySession , and use it to erase this session data and all that are expired. This way whenever a user logouts he will use this to clean up the sessions. This could be done with only one db query. The time it will take to execute is not an issue, because the user is disconnecting his sessions, so if he waits a couple seconds more he won't complaint . Even so, i think it will be fast because theres one a few expired sessions (i beblieve most of the users normally logout :) )

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27180833 .

Regards, Rahul Chaudhary Ph - 412-519-9634

abiusx commented 10 years ago

In most applications, nobody logs out. People just leave it to expire.


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 27, 2013, at 7:59 PM, Rahul Chaudhary notifications@github.com wrote:

ok..so this approach has couple of meanings: 1) the user logs out and he deletes all his previous sessions which are expired. Pretty Good. But the problem still is there. Thousands of customers come and their sessions are set. And then they dont come again. So, their sessions will be in DB forever.

2) the user logs out, and he runs the function that will clear out all of the expired sessions. Good again. Until you consider this: 1000 users logs out at once (which happens in real scenario), then this function will be called 1000 times, even though our work was done in the first time.

On Sun, Oct 27, 2013 at 6:11 PM, Paulo Guerreiro notifications@github.comwrote:

Why not call this function at Session::destroySession , and use it to erase this session data and all that are expired. This way whenever a user logouts he will use this to clean up the sessions. This could be done with only one db query. The time it will take to execute is not an issue, because the user is disconnecting his sessions, so if he waits a couple seconds more he won't complaint . Even so, i think it will be fast because theres one a few expired sessions (i beblieve most of the users normally logout :) )

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27180833 .

Regards, Rahul Chaudhary Ph - 412-519-9634 — Reply to this email directly or view it on GitHub.

mebjas commented 10 years ago

How is this method implemented in php? Deleting expired session data?

paulocmguerreiro commented 10 years ago

@Rahul, i don't have experience in db with that much simultaneous users. (the max i have had it's with 30 simultaneous users :( ) (but i will search online to read about it)

I imagine if there's 1000 users ready to logout, how much are online (in this case the software is not the only issue, because everything you do some how migth become a bottleneck problem).

My point is (i'm assuming you cannot afford not to erase user session and data on his logout, due to security)

Why, not, instead of calling

SQL("DELETE FROM SESSION WHERE SESSION_ID = ?", array($this->session));

Why not (this will need an extra INDEX on Column LAST_ACTIVITY to speed up the query) :

SQL("DELETE FROM SESSION WHERE SESSION_ID = ? OR ''LAST_ACTIVITY' < ?", array($this->session),$now - $maxallowedtimeout);

And while we are at it, let's related also with SESSION_DATA and use only 1 query. SQL("DELETE FROM 'SESSION', 'SESSION_DATA' WHERE......);

I haven't mentioned in the earlier post, but having this run on a probabilty is a good idea.

@abiusx users need to be educated to logout,

PS: updated the query in this (it was incorrect)

rash805115 commented 10 years ago

doesn't matter if 1000 users logs out simultaneously or just 2. The thing is that once one user logs out, the cleanup process will run (which is expensive). Then another user logs out just in a second, then the process runs again..its just extra overhead that is not practical.

As you said, the probability idea is also good and what Abbas suggested is also good (Read previous emails for detail). Anyways, just a heads up that the problem is now solved. Its just the matter of implementation which I will do as soon as I can..

On Mon, Oct 28, 2013 at 6:51 AM, Paulo Guerreiro notifications@github.comwrote:

@Rahul https://github.com/Rahul, i don't have experience in db with that much simultaneous users. (the max i have had it's with 30 simultaneous users :( ) (but i will search online to read about it)

I imagine if there's 1000 users ready to logout, how much are online (in this case the software is not the only issue, because everything you do some how migth become a bottleneck problem).

My point is (i'm assuming you cannot afford not to erase user session and data on his logout, due to security)

Why, not, instead of calling

SQL("DELETE FROM SESSION WHERE SESSION_ID = ?", array($this->session));

Why not (this will need an extra INDEX on Column LAST_ACTIVITY to speed up the query) :

SQL("DELETE FROM SESSION WHERE SESSION_ID = ? AND 'LAST_ACTIVITY' < ?", array($this->session),$now - $maxallowedtimeout);

And while we are at it, let's related also with SESSION_DATA and use only 1 query. SQL("DELETE FROM 'SESSION', 'SESSION_DATA' WHERE......);

I haven't mentioned in the earlier post, but having this run on a probabilty is a good idea.

@abiusx https://github.com/abiusx users need to be educated to logout,

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/71#issuecomment-27202315 .

Regards, Rahul Chaudhary Ph - 412-519-9634

mebjas commented 10 years ago

I think this bug was fixed and this issue should be closed now!