[TYPO3-core] Regression in 4.5.23

[Helmut Hummel]

Hi.

On 22.02.13 00:09, Steffen Müller wrote:

> 3) Shit happens.

True. Nobody should be blamed. Instead we should learn and adjust things 
for the future.

 > In this
> case however, applying the patch did not fail

I cannot verify this. Cherry-Picking the change from the 4.6 branch into 
the 4.5 branch (4.5.22) ends up with a conflict.

> and there were not enough
> tests to detect a failure.

The changed methods are small and have few dependencies. Writing a test 
for that would have been perfectly doable.

> Unhappy coincidence.

Coincidences happen. In this case, we had many things in place to avoid 
this error.

We have a good base of unit tests and an evironment that executes the 
tests automatically for 4-7, 6-0 and master.
It is prefectly reasonable that a conflicting backport *should* get a 
full review.


To better avoid such errors in the future, we must:

* Enforce a full review when a backport patch conflicts or needs any
   other manual interaction
* Enforce unit tests for every bugfix that is targeted for backporting

(When unit tests are not possible because of old, monolythic code, we 
must create functional tests. Chirstian Kuhn and me have the plan to set 
up a reasonable environment for such tests in the next weeks.)

If a bugfix that involves backend UI, Christian Trabold started working 
on UAT based on Cucumber and got pretty far. Unfortunately I had no time 
to pick that up until now and probably won't do if I do not get help 
from anybody.

> Expected behaviour: The session cookie id changes with each request when
> no session data payload exists. When there's payload, the id is kept.

There is no need to renew the session id on every request. This is just 
a side-effect of a session-fixation problem we fixed a long time ago.

> Actual behaviour (before patch): The session cookie does not change in
> one case, although no real payload exists. This case occurs, whenever
> session data payload has been present and was removed afterwards. The
> reason: session handling failed to clear the payload, but instead left
> some garbage as payload. This garbage prevented the session cookie id
> from changing.

I think we talked about that, but I can't really remember what I said to 
you besides I do not consider this critical. Here again the reasons why 
I think so:

The scenario where the fix for that could mitigate a security problem is 
very unlikely to happen.

It involves:
* session cookie/id stealing (additional exploits or physical access to 
the PC needed)
* IP and user agent forging
* access the session within the timeout
* critical/private data is added to the user session after it has once 
been empty (and the session id got stolen while session was empty)


A real security improvement for the session handling would be to renew 
the session id every time the access level changes:

E.g.
1. No user logged in -> session id is enforced to be generated from the
    server (we have that in current state)
2. Session data is stored -> If it is data that is worth protecting,
    the session id needs to be changed, if not, data can be stored using
    the same id
3. A user loggs in -> priviledge level changed: session id needs to
    change

We have no API to change the session ID (while keeping the data at the 
same time).
The session handling and API in general is... not optimal.

If someone wants to pick up, it would be great. Such changes cannot be 
backported of course ;)

Kind regards,
Helmut

-- 
Helmut Hummel
Release Manager TYPO3 6.0
TYPO3 Core Developer, TYPO3 Security Team Member

TYPO3 .... inspiring people to share!
Get involved: typo3.org