Everything

php

Home » Project » Newsgroups » php.internals » header() removes all header of the same name. »

header() removes all header of the same name.

php.internals

Yasuo Ohgaki

9 years ago

Hi all,

I understand why header() is made to remove all headers of the same
name. This is needed in some cases, but it does not work well for some
cases.

We need to decide what to do with
https://bugs.php.net/bug.php?id=72997

There is 2 issues.
- header() removes all headers of the same name including 'Set-Cookie'
- header() ignores replace flag. (This one is easy to fix)

Since header() enables 'replace flag' by default, it removes all
'Set-Cookie' headers sent previously by default. It can easily disturb
security related cookies to work. i.e. Session ID cookie, Auto Login
cookie. This bug would be very hard to find for normal users, too.

Restoring older behavior (Removing only one header) cannot be a
resolution because it can still disturb security related cookies.

Possible resolutions:

- Prohibit 'Set-Cookie' for header() and force users to use setcookie()
- Mitigate by disabling replace flag by default. (This is not a good idea, IMO)

Both resolution requires BC, but this is better to be fixed ASAP.

Non-BC resolution could be:
- "Ask users to use setcookie() always for 'Set-Cookie'".

I would like to prohibit 'Set-Cookie' by header() because it may
remove session ID cookie as well as auto login cookie, etc. If we
leave released version as it is now, I would like to prohibit
'Set-Cookie' by header() in PHP 7.1.

Problem with this may be that user cannot modify 'Set-Cookie' header
line as user want.

$ php -r 'setcookie("REMEMBERME=value; expires=Sat, 03-Sep-2020
05:38:43 GMT; path=/; domain=aaa");'
PHP Warning:  Cookie names cannot contain any of the following '=,;
\t\r\n\013\014' in Command line code on line 1


Comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stephen Reay

9 years ago

(Apologies for the dupe, re-sending for the list.)

If the replace flag was fixed, isn’t this then just a case of making sure userland sets replace to false if they want existing set-cookie headers retained?

Removing the ability to write a custom Set-Cookie header introduces a bigger problem than the current one, IMO.

Yasuo Ohgaki

9 years ago

Hi all,

On Tue, Oct 18, 2016 at 4:31 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I understand why header() is made to remove all headers of the same
> name. This is needed in some cases, but it does not work well for some
> cases.
>
> We need to decide what to do with
> https://bugs.php.net/bug.php?id=72997
>
> There is 2 issues.
>   - header() removes all headers of the same name including 'Set-Cookie'
>   - header() ignores replace flag. (This one is easy to fix)
>
> Since header() enables 'replace flag' by default, it removes all
> 'Set-Cookie' headers sent previously by default. It can easily disturb
> security related cookies to work. i.e. Session ID cookie, Auto Login
> cookie. This bug would be very hard to find for normal users, too.
>
> Restoring older behavior (Removing only one header) cannot be a
> resolution because it can still disturb security related cookies.
>
> Possible resolutions:
>
>  - Prohibit 'Set-Cookie' for header() and force users to use setcookie()
>  - Mitigate by disabling replace flag by default. (This is not a good idea, IMO)
>
> Both resolution requires BC, but this is better to be fixed ASAP.
>
> Non-BC resolution could be:
>   - "Ask users to use setcookie() always for 'Set-Cookie'".
>
> I would like to prohibit 'Set-Cookie' by header() because it may
> remove session ID cookie as well as auto login cookie, etc. If we
> leave released version as it is now, I would like to prohibit
> 'Set-Cookie' by header() in PHP 7.1.
>
> Problem with this may be that user cannot modify 'Set-Cookie' header
> line as user want.
>
> $ php -r 'setcookie("REMEMBERME=value; expires=Sat, 03-Sep-2020
> 05:38:43 GMT; path=/; domain=aaa");'
> PHP Warning:  Cookie names cannot contain any of the following '=,;
> \t\r\n\013\014' in Command line code on line 1
>
>
> Comments?

An idea for session ID protection.

Following code results in lost session always.

<?php
session_start();
session_regenerate_id();
header('Set-Cookie: something');
?>

header() function removes all header of the same name, e.g.
Set-Cookie, Expires, etc, by sapi_remove_header(). This could be very
hard to find the cause.


This risk can be removed w/o much BC. Only BC is when user is
intentionally trying to delete session ID cookie manually. This would
be very rare. We can add code that excludes session ID cookie in
sapi_remove_header().
http://lxr.php.net/xref/PHP-MASTER/main/SAPI.c#593

To do that, we can search string like following
Set-Cookie: PHPSESSID=xxxxxxx

The only issue is we need session global, i.e. PS(session_name), at
least. It's not nice to have dependency from SAPI.c to session, but it
protects session ID from removed by users by mistake.

Any comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stephen Reay

9 years ago

Hi Yasuo,

I still have an issue with that. I believe the correct behaviour here is (assuming the `replace` argument to header() is honoured) what you’re seeing. Yes, it might be *unexpected* for new users, but its also *expected* by millions of current users/projects.

I would suggest perhaps a warning on the header() docs page, and perhaps an example to avoid the issue on the Session handling page.

Leaving it as-is, with improved docs means all functionality is *possible* with the right arguments.

Changing to your proposal means advanced use-cases are *impossible* with any arguments.

I realise you’re trying to remove WTF cases, but I don’t think removing advanced capabilities is the way to do that.

Cheers

Stephen

Yasuo Ohgaki

9 years ago

Hi Stephen,

On Wed, Oct 19, 2016 at 12:18 PM, Stephen Reay <php-lists@koalephant.com> wrote:

> I still have an issue with that. I believe the correct behaviour here is (assuming the `replace` argument to header() is honoured) what you’re seeing. Yes, it might be *unexpected* for new users, but its also *expected* by millions of current users/projects.
>
> I would suggest perhaps a warning on the header() docs page, and perhaps an example to avoid the issue on the Session handling page.
>
> Leaving it as-is, with improved docs means all functionality is *possible* with the right arguments.
>
> Changing to your proposal means advanced use-cases are *impossible* with any arguments.
>
>
> I realise you’re trying to remove WTF cases, but I don’t think removing advanced capabilities is the way to do that.

Yes. Even framework developer(?) seems to have current behavior.

In general, users shouldn't touch session ID. In case of user really
want to modify session ID cookie, following could be done.

<?php
ob_start();
session_start();
header_remove('Set-Cookie');
header('Set-Cookie: PHPSESSID=xxxx something');
?>

Make header_remove() able to delete 'Set-Cookie' header. (Current behavior)
Make header() able to send 'Set-Cookie' header. (Current behavior, but
not remove session ID cookie)

This allows users to send arbitrary session ID cookie when it is
needed really needed, while avoiding accidental session ID cookie
removal.

What do you think?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago

Hi Stephen,

On Wed, Oct 19, 2016 at 1:34 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

>> I realise you’re trying to remove WTF cases, but I don’t think removing advanced capabilities is the way to do that.
>
> Yes. Even framework developer(?) seems to have current behavior.

To be honest, I didn't have idea what's wrong with user provided code
when I have a look at the bug report. I had to experiment a little to
understand what is going on.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago

Hi all,

On Wed, Oct 19, 2016 at 1:34 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

>
> On Wed, Oct 19, 2016 at 12:18 PM, Stephen Reay <php-lists@koalephant.com> wrote:
>> I still have an issue with that. I believe the correct behaviour here is (assuming the `replace` argument to header() is honoured) what you’re seeing. Yes, it might be *unexpected* for new users, but its also *expected* by millions of current users/projects.
>>
>> I would suggest perhaps a warning on the header() docs page, and perhaps an example to avoid the issue on the Session handling page.
>>
>> Leaving it as-is, with improved docs means all functionality is *possible* with the right arguments.
>>
>> Changing to your proposal means advanced use-cases are *impossible* with any arguments.
>>
>>
>> I realise you’re trying to remove WTF cases, but I don’t think removing advanced capabilities is the way to do that.
>
> Yes. Even framework developer(?) seems to have current behavior.
>
> In general, users shouldn't touch session ID. In case of user really
> want to modify session ID cookie, following could be done.
>
> <?php
> ob_start();
> session_start();
> header_remove('Set-Cookie');
> header('Set-Cookie: PHPSESSID=xxxx something');
> ?>
>
> Make header_remove() able to delete 'Set-Cookie' header. (Current behavior)
> Make header() able to send 'Set-Cookie' header. (Current behavior, but
> not remove session ID cookie)
>
> This allows users to send arbitrary session ID cookie when it is
> needed really needed, while avoiding accidental session ID cookie
> removal.
>
> What do you think?

Another idea for session ID cookie and Set-Cookie header protection.

Since we have setcookie() function, how about to have cookie
dedicated functions for cookie header manipulation.

I'm about to create new feature request as follows:
---------------------
Protect session ID and other cookies from header(), header_remove()
---------------------
header() removes any previously defined headers.
header('Set-Cookie: something') / header_remove() deletes session ID
and other Set-Cookie headers. Cookies should be protected from
header()/header_remove().

Instead, create new cookie functions

cookie_set() - Set cookie header (setcookie() alias)
cookie_set_raw() - Set cookie header (setrawcookie alias)
cookie_custom() - Set cookie with custom style.
(The same as header(sprintf('Set-Cookie:
%s', something));
cookie_remove() - Remove all cookie header. $name parameter is cookie
name to be deleted.

Protect Set-Cookie headers from header() and header_remove()
----------------------

This implementation is cleaner because core to session
dependency is not required. It is also good to have naming standard
confirming cookie function names. i.e. Cookie functions should be
named cookie_*() according to CODING_STANDARDS.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Andrew Faulds

9 years ago

Hi Yasuo,

I don't think we should do anything about this beyond maybe warning the
user in the manual. header() is a generic function for setting headers,
it would be surprising if it had different behaviour for cookies or
session cookies. It is possible that use of this function in this way
may be deliberate.

Thanks.

--
Andrea Faulds
https://ajf.me/

Stas Malyshev

9 years ago

Hi!

> There is 2 issues.
>   - header() removes all headers of the same name including 'Set-Cookie'
>   - header() ignores replace flag. (This one is easy to fix)

We have the flag, so if it doesn't work it should be fixed. Also, one
should use setcookie() for cookies, usually.

> Possible resolutions:
>
>  - Prohibit 'Set-Cookie' for header() and force users to use setcookie()
>  - Mitigate by disabling replace flag by default. (This is not a good idea, IMO)

I don't think we should do either.

> I would like to prohibit 'Set-Cookie' by header() because it may
> remove session ID cookie as well as auto login cookie, etc. If we
> leave released version as it is now, I would like to prohibit
> 'Set-Cookie' by header() in PHP 7.1.

I don't think it's a good idea. If somebody is using header(), it should
work like header() works. If you don't like how it works, use setcookie.

> Problem with this may be that user cannot modify 'Set-Cookie' header
> line as user want.
>
> $ php -r 'setcookie("REMEMBERME=value; expires=Sat, 03-Sep-2020
> 05:38:43 GMT; path=/; domain=aaa");'
> PHP Warning:  Cookie names cannot contain any of the following '=,;
> \t\r\n\013\014' in Command line code on line 1

You are using setcookie() wrong here. See: http://php.net/setcookie

--
Stas Malyshev
smalyshev@gmail.com

Yasuo Ohgaki

9 years ago

Hi Stas,

I posted an an idea for preventing accidental cookie deletion.
'Set-Cookie' is a HTTP header, but provide dedicated functions for it. I pasted
it with a little modification.
What do you think?

Bottom line is I would like to prevent lost session ID  by header()
in the future.

Implement cookie_*() functions in 7.x, then prohibit 'Set-Cookie' for
header() in 8.x

On Thu, Oct 20, 2016 at 1:39 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:

>> There is 2 issues.
>>   - header() removes all headers of the same name including 'Set-Cookie'
>>   - header() ignores replace flag. (This one is easy to fix)
>
> We have the flag, so if it doesn't work it should be fixed. Also, one
> should use setcookie() for cookies, usually.

Another idea for session ID cookie and Set-Cookie header protection.

Since we have setcookie() function, how about to have cookie
dedicated functions for cookie header manipulation.

I'm about to create new feature request as follows:
---------------------
Protect session ID and other cookies from header(), header_remove()
---------------------
header() removes any previously defined headers.
header('Set-Cookie: something') / header_remove() deletes session ID
and other Set-Cookie headers. Cookies should be protected from
header()/header_remove().

Instead, create new cookie functions

cookie_set() - Set cookie header (setcookie() alias)
cookie_set_raw() - Set cookie header (setrawcookie alias)
cookie_custom() - Set cookie with custom style.
(The same as header(sprintf('Set-Cookie:
%s', $something));
cookie_list() - Mostly the same as headers_list()
cookie_remove([string $name]) - Mostly the same as header_remove()
Remove cookie header. $name parameter is cookie name to be deleted.

Protect Set-Cookie headers from header() and header_remove()
----------------------

This implementation is cleaner because core to session
dependency is not required. It is also good to have naming standard
confirming cookie function names. i.e. Cookie functions should be
named cookie_*() according to CODING_STANDARDS.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stephen Reay

9 years ago

Hi All,

Just to make my earlier point of view crystal clear: As a purely userland party and someone maintaining a PHP framework, I don’t think it’s acceptable to limit which headers header()/header_remove() can operate on, particularly when the problem you’re trying to ‘solve’ is simply incorrect use of the functions available. It *is* possible to achieve any outcome desired with *correct* use of the header, session and cookie functions (and assuming the $replace argument to header() works correctly).

I still believe the way to solve this issue is with better information about usage, not by removing existing functionality.

So, please do *not* consider this to be an acceptable solution.

Cheers

Stephen