ext/session: fix cookie_lifetime overflow#21704
Conversation
When session.cookie_lifetime was set to a value larger than maxcookie, OnUpdateCookieLifetime returned SUCCESS without updating the internal long value, causing ini_get() string and PS(cookie_lifetime) to go out of sync. Now the value is properly clamped to maxcookie with both the string and internal long updated consistently, and a warning is emitted.
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
While at it could you fix the way we parse the string as well? As this probably allows non numeric strings and float strings.
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Getting there, minor comments. Thanks for tackling this :)
ext/session/session.c
Outdated
| if (type == 0) { | ||
| php_error_docref(NULL, E_WARNING, "Invalid value for CookieLifetime"); | ||
| return FAILURE; | ||
| } else if (type == IS_DOUBLE && oflow == 0) { | ||
| php_error_docref(NULL, E_WARNING, "CookieLifetime must be an integer"); | ||
| return FAILURE; | ||
| } |
There was a problem hiding this comment.
Maybe move this into an if (UNEXPECTED(type != IS_LONG)) {} block?
There was a problem hiding this comment.
Done, also bit simplified the flow.
ext/session/session.c
Outdated
| } | ||
| zend_long v = lval; | ||
| if (oflow < 0 || v < 0) { | ||
| php_error_docref(NULL, E_WARNING, "CookieLifetime cannot be negative"); |
There was a problem hiding this comment.
Maybe this error could be updated to the more standard wording of must be between 0 and maxcookie?
ext/session/session.c
Outdated
| } else if (oflow > 0 || v > maxcookie) { | ||
| php_error_docref(NULL, E_WARNING, "CookieLifetime value too large, value was set to the maximum of " ZEND_LONG_FMT, maxcookie); | ||
| zend_long *p = ZEND_INI_GET_ADDR(); | ||
| *p = maxcookie; | ||
| entry->value = zend_long_to_str(maxcookie); | ||
| return SUCCESS; |
There was a problem hiding this comment.
Did this use to actually update the INI setting or was it just returning SUCCESS even if it was failing? :|
There was a problem hiding this comment.
Yes, currently it doesn't update the value when overflows, but returns success.
ext/session/session.c
Outdated
| } else if (lval > maxcookie) { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT ", value clamped to maximum", maxcookie); | ||
| zend_long *p = ZEND_INI_GET_ADDR(); | ||
| *p = maxcookie; | ||
| entry->value = zend_long_to_str(maxcookie); | ||
| return SUCCESS; | ||
| } |
There was a problem hiding this comment.
I think it was a bug before for it to return SUCCESS, so I would rather have it return FAILURE and warn then silently change behaviour.
Effectively just change the prior if condition to if (lval < 0 || lval > maxcookie) {
There was a problem hiding this comment.
Sure. It changes logic and may break some implementations, but it's fine to me.
ext/session/session.c
Outdated
| if (oflow != 0) { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie); | ||
| } else { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be an integer"); |
There was a problem hiding this comment.
The usual error message is something along the line of must be of type int.
There was a problem hiding this comment.
Applied + created an addition to CODING_CONVENTIONS.md in #21761
jorgsowa
force-pushed
the
fix/session-cookie-lifetime-overflow
branch
from
b99050e to
83991b6
Compare
2 participants
When
session.cookie_lifetimewas set to a value larger than maxcookie,OnUpdateCookieLifetimereturned SUCCESS without updating the internal long value, causing ini_get() string and PS(cookie_lifetime) to go out of sync.Now the value is properly clamped to maxcookie with both the string and internal long updated consistently, and a warning is emitted.
Edit:
Added validation of value of maxcookie, only numeric strings are allowed.