Duplicate session headers not removed in PHP 5.3

Bug #1570744 reported by Aaron Wells on 2016-04-15
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mahara
High
Unassigned
15.04
High
Unassigned
15.10
High
Unassigned
16.04
High
Unassigned
16.10
High
Aaron Wells

Bug Description

See also Bug 1570179.

It turns out that our method clear_duplicate_cookies() doesn't work in PHP 5.3, because the behavior of session headers is different there than in the versions of PHP we tested on. We need to rewrite the function to work properly in PHP 5.3, as long as we claim to support 5.3.

CVE References

Reviewed: https://reviews.mahara.org/6348
Committed: https://git.mahara.org/mahara/mahara/commit/83ec33f245b645e58d797fb1b2316d11e369119d
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit 83ec33f245b645e58d797fb1b2316d11e369119d
Author: Aaron Wells <email address hidden>
Date: Fri Apr 15 20:12:17 2016 +1200

Bug 1570744: Fixing session bugs

This patch does 2 things:

1. It loads the session much earlier during init.php. We wind
up creating one on *every* script load anyway, due to LiveUser's
constructor. Sometimes it gets created earlier if other code
tries to use it before then, which adds some unpredictability
to things. Moving it up to the top of init.php reduces that
unpredictability.

2. It turns out that in PHP 5.3, using header_remove('Set-Cookie')
to only doesn't remove session headers. But header_remove()
(with no params) to remove *all* cookies does remove them. So
I'm changing remove_duplicate_cookies() to use that instead.

3. Also in PHP 5.3, session headers are visible in headers_list().
In situations where your session id changes (due to session_destroy()
and session_regenerate_id()), our use of array_unique() meant we
would preserve the old and new session IDs and send both back
to the browser. This patch makes remove_duplicate_cookies() aware
of the current session ID, and it only preserves that one.

Change-Id: I7a90b8692a5f97429415aa9a17451a44cd2109dd
behatnotneeded: Covered by existing tests

Reviewed: https://reviews.mahara.org/6353
Committed: https://git.mahara.org/mahara/mahara/commit/abd93f7c3ba0940d9327f9dfd9fc487f8d948499
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit abd93f7c3ba0940d9327f9dfd9fc487f8d948499
Author: Aaron Wells <email address hidden>
Date: Mon Apr 18 17:24:49 2016 +1200

Correcting typoes in cookie-issuing code

Bug 1570744: Accidentally used set_cookie() instead of
setcookie(). This makes the cookie break if you use
the $cfg->cookieprefix setting.

behatnotneeded: Covered by existing tests

Change-Id: Idec3676222e3ff4eb22f7925de6bec10cfa35755

Mahara Bot (dev-mahara) wrote :

Patch for "16.04_STABLE" branch: https://reviews.mahara.org/6368

Mahara Bot (dev-mahara) wrote :

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6369

Mahara Bot (dev-mahara) wrote :

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6370

Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6371

Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6372

Reviewed: https://reviews.mahara.org/6367
Committed: https://git.mahara.org/mahara/mahara/commit/0ea25ccb6532f9cc3ba5b8c35e0688f52358816f
Submitter: Robert Lyon (<email address hidden>)
Branch: 16.04_STABLE

commit 0ea25ccb6532f9cc3ba5b8c35e0688f52358816f
Author: Aaron Wells <email address hidden>
Date: Fri Apr 15 20:12:17 2016 +1200

Bug 1570744: Fixing session bugs

This patch does 2 things:

1. It loads the session much earlier during init.php. We wind
up creating one on *every* script load anyway, due to LiveUser's
constructor. Sometimes it gets created earlier if other code
tries to use it before then, which adds some unpredictability
to things. Moving it up to the top of init.php reduces that
unpredictability.

2. It turns out that in PHP 5.3, using header_remove('Set-Cookie')
to only doesn't remove session headers. But header_remove()
(with no params) to remove *all* cookies does remove them. So
I'm changing remove_duplicate_cookies() to use that instead.

3. Also in PHP 5.3, session headers are visible in headers_list().
In situations where your session id changes (due to session_destroy()
and session_regenerate_id()), our use of array_unique() meant we
would preserve the old and new session IDs and send both back
to the browser. This patch makes remove_duplicate_cookies() aware
of the current session ID, and it only preserves that one.

Change-Id: I7a90b8692a5f97429415aa9a17451a44cd2109dd
behatnotneeded: Covered by existing tests
(cherry picked from commit 83ec33f245b645e58d797fb1b2316d11e369119d)

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6371
Committed: https://git.mahara.org/mahara/mahara/commit/ebae1e236a1e54b3e8a4437b46d7d35c07b22555
Submitter: Aaron Wells (<email address hidden>)
Branch: 15.04_STABLE

commit ebae1e236a1e54b3e8a4437b46d7d35c07b22555
Author: Aaron Wells <email address hidden>
Date: Fri Apr 15 20:12:17 2016 +1200

Bug 1570744: Fixing session bugs

This patch does 2 things:

1. It loads the session much earlier during init.php. We wind
up creating one on *every* script load anyway, due to LiveUser's
constructor. Sometimes it gets created earlier if other code
tries to use it before then, which adds some unpredictability
to things. Moving it up to the top of init.php reduces that
unpredictability.

2. It turns out that in PHP 5.3, using header_remove('Set-Cookie')
to only doesn't remove session headers. But header_remove()
(with no params) to remove *all* cookies does remove them. So
I'm changing remove_duplicate_cookies() to use that instead.

3. Also in PHP 5.3, session headers are visible in headers_list().
In situations where your session id changes (due to session_destroy()
and session_regenerate_id()), our use of array_unique() meant we
would preserve the old and new session IDs and send both back
to the browser. This patch makes remove_duplicate_cookies() aware
of the current session ID, and it only preserves that one.

Change-Id: I7a90b8692a5f97429415aa9a17451a44cd2109dd
behatnotneeded: Covered by existing tests
(cherry picked from commit 83ec33f245b645e58d797fb1b2316d11e369119d)

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6369
Committed: https://git.mahara.org/mahara/mahara/commit/6d469bd61156ceabdfee10291d0af6b096b2309d
Submitter: Aaron Wells (<email address hidden>)
Branch: 15.10_STABLE

commit 6d469bd61156ceabdfee10291d0af6b096b2309d
Author: Aaron Wells <email address hidden>
Date: Fri Apr 15 20:12:17 2016 +1200

Bug 1570744: Fixing session bugs

This patch does 2 things:

1. It loads the session much earlier during init.php. We wind
up creating one on *every* script load anyway, due to LiveUser's
constructor. Sometimes it gets created earlier if other code
tries to use it before then, which adds some unpredictability
to things. Moving it up to the top of init.php reduces that
unpredictability.

2. It turns out that in PHP 5.3, using header_remove('Set-Cookie')
to only doesn't remove session headers. But header_remove()
(with no params) to remove *all* cookies does remove them. So
I'm changing remove_duplicate_cookies() to use that instead.

3. Also in PHP 5.3, session headers are visible in headers_list().
In situations where your session id changes (due to session_destroy()
and session_regenerate_id()), our use of array_unique() meant we
would preserve the old and new session IDs and send both back
to the browser. This patch makes remove_duplicate_cookies() aware
of the current session ID, and it only preserves that one.

Change-Id: I7a90b8692a5f97429415aa9a17451a44cd2109dd
behatnotneeded: Covered by existing tests
(cherry picked from commit 83ec33f245b645e58d797fb1b2316d11e369119d)

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6372
Committed: https://git.mahara.org/mahara/mahara/commit/857af9058ed8a1298347f22fce675d0b0294cbf1
Submitter: Aaron Wells (<email address hidden>)
Branch: 15.04_STABLE

commit 857af9058ed8a1298347f22fce675d0b0294cbf1
Author: Aaron Wells <email address hidden>
Date: Mon Apr 18 17:24:49 2016 +1200

Correcting typoes in cookie-issuing code

Bug 1570744: Accidentally used set_cookie() instead of
setcookie(). This makes the cookie break if you use
the $cfg->cookieprefix setting.

behatnotneeded: Covered by existing tests

Change-Id: Idec3676222e3ff4eb22f7925de6bec10cfa35755

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6370
Committed: https://git.mahara.org/mahara/mahara/commit/0184cbf64065ed48bdeedbc882db46e78cc0b5f2
Submitter: Aaron Wells (<email address hidden>)
Branch: 15.10_STABLE

commit 0184cbf64065ed48bdeedbc882db46e78cc0b5f2
Author: Aaron Wells <email address hidden>
Date: Mon Apr 18 17:24:49 2016 +1200

Correcting typoes in cookie-issuing code

Bug 1570744: Accidentally used set_cookie() instead of
setcookie(). This makes the cookie break if you use
the $cfg->cookieprefix setting.

behatnotneeded: Covered by existing tests

Change-Id: Idec3676222e3ff4eb22f7925de6bec10cfa35755

Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6368
Committed: https://git.mahara.org/mahara/mahara/commit/2db0fd90652927e656c67939c400307e1990ff8c
Submitter: Aaron Wells (<email address hidden>)
Branch: 16.04_STABLE

commit 2db0fd90652927e656c67939c400307e1990ff8c
Author: Aaron Wells <email address hidden>
Date: Mon Apr 18 17:24:49 2016 +1200

Correcting typoes in cookie-issuing code

Bug 1570744: Accidentally used set_cookie() instead of
setcookie(). This makes the cookie break if you use
the $cfg->cookieprefix setting.

behatnotneeded: Covered by existing tests

Change-Id: Idec3676222e3ff4eb22f7925de6bec10cfa35755

Aaron Wells (u-aaronw) wrote :

This bug isn't present in Mahara 1.10, because it was a regression introduced by our session changes in 15.04.

no longer affects: mahara/1.10
Robert Lyon (robertl-9) on 2016-10-21
Changed in mahara:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers