[VOTE] RFC: Multibyte Char Handling

12 years ago by Nikita Popov — view source — reply

unread

Hi all,

This RFC is to fix CVE-2014-1239.

https://wiki.php.net/rfc/multibyte_char_handling

Adding required functions to mbstring and provide them
from PHP 5.3 and up and replacing mbstring to mbstring-ng
for future releases.

Thank you for voting!

This RFC conflates the addition of a multibyte version of addslashes (in
response to quoted CVE) with the replacement of the mbstring extension by a
completely different implementation (and an incomplete one at that). Those
two things have very little to do with each other and should not be covered
in the same RFC and/or vote.

Nikita

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Nikita,

This RFC conflates the addition of a multibyte version of addslashes (in
response to quoted CVE) with the replacement of the mbstring extension by a
completely different implementation (and an incomplete one at that). Those
two things have very little to do with each other and should not be covered
in the same RFC and/or vote.

The root cause of this issue is lack of multibyte aware functions that
relates to security.

I've wrote the RFC to compile current mbstring by default at first, but it
was
withdrawn. The reason why is that mbstring is using LGPLed libraries.
As long as it is loaded as shared module, there would not be issue.
However, if these are compiled and used statically, LGPL will be
effective.

To avoid this issue, mbstring would be better to replaced by mbstring-ng
and move mbstring to PECL for future release.

I'll work on mbstring-ng so that it has all mbstring features. Until then,
we may have it as EXPERIMENTAL.

Although, it may seem different issue. Compilation of mbstring by
default is needed to resolve the issue. Therefore, I've made a
single RFC to accomplish the objective.

Does this sound reasonable to you?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Jakub Zelenka — view source — reply

unread

Hi,

Hi Nikita,

On Sun, Jan 26, 2014 at 9:38 AM, Nikita Popov nikita.ppv@gmail.com
wrote:

This RFC conflates the addition of a multibyte version of addslashes (in
response to quoted CVE) with the replacement of the mbstring extension
by a
completely different implementation (and an incomplete one at that).
Those
two things have very little to do with each other and should not be
covered
in the same RFC and/or vote.

The root cause of this issue is lack of multibyte aware functions that
relates to security.

I've wrote the RFC to compile current mbstring by default at first, but it
was
withdrawn. The reason why is that mbstring is using LGPLed libraries.
As long as it is loaded as shared module, there would not be issue.
However, if these are compiled and used statically, LGPL will be
effective.

To avoid this issue, mbstring would be better to replaced by mbstring-ng
and move mbstring to PECL for future release.

I'll work on mbstring-ng so that it has all mbstring features. Until then,
we may have it as EXPERIMENTAL.

Although, it may seem different issue. Compilation of mbstring by
default is needed to resolve the issue. Therefore, I've made a
single RFC to accomplish the objective.

Does this sound reasonable to you?

Regards,

I have been looking a bit into the mbstring-ng. I forked it from moriyoshi
and fixed some compilation issues (for php-master).

https://github.com/bukka/mbstring-ng/compare/next

I also run

$ find ./tests/ -type f -exec sed -i 's/mb_/mb2_/g' {} ;

and then test it a bit. Most of the tests are failing. It hasn't been
update for 5 years so there some runtime issues and there are some missing
functions. It looks that it will require quite a lot of work. I will try to
have a look later if I get time... :)

Thought that it could help a bit when you start working on it. ;)

Regards

Jakub

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Jakub,

Thought that it could help a bit when you start working on it. ;)

It's 5 years old patch. I'll work on it before put it into php-src.
Implementation of mbstring-ng is subject to be changed including
names. It must be a copy of current mbstring.

If you would like to help the work, I appreciate it!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Derick Rethans — view source — reply

unread

Hi Nikita,

This RFC conflates the addition of a multibyte version of addslashes (in
response to quoted CVE) with the replacement of the mbstring extension by a
completely different implementation (and an incomplete one at that). Those
two things have very little to do with each other and should not be covered
in the same RFC and/or vote.

The root cause of this issue is lack of multibyte aware functions that
relates to security.

Yes, and the only way to address that properly is to make PHP support
Unicode. This proposal adds layers of hacks to go around it - and
although there is a possibility for broken data, I don't think it's
worth doing.

I've wrote the RFC to compile current mbstring by default at first,
but it was withdrawn. The reason why is that mbstring is using LGPLed
libraries. As long as it is loaded as shared module, there would not
be issue. However, if these are compiled and used statically, LGPL
will be effective.

To avoid this issue, mbstring would be better to replaced by mbstring-ng
and move mbstring to PECL for future release.

I'll work on mbstring-ng so that it has all mbstring features. Until then,
we may have it as EXPERIMENTAL.

We already have intl - why add another mbstring like function thing?

cheers,
Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Derick,

I've wrote the RFC to compile current mbstring by default at first,
but it was withdrawn. The reason why is that mbstring is using LGPLed
libraries. As long as it is loaded as shared module, there would not
be issue. However, if these are compiled and used statically, LGPL
will be effective.

To avoid this issue, mbstring would be better to replaced by mbstring-ng
and move mbstring to PECL for future release.

I'll work on mbstring-ng so that it has all mbstring features. Until
then,
we may have it as EXPERIMENTAL.

We already have intl - why add another mbstring like function thing?

It does not have mbstring features. We need compatible module that
has no license issues. It would be better to create a new module for
this purpose. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Dan Ackroyd — view source — reply

unread

Hi,

This vote is not listed as 'In voting phase' on the RFC home page -
https://wiki.php.net/rfc

Also, as Nikic mentioned, this RFC has changed from the original
request to merging a different library into PHP, 3 days ago. Voting on
such a change in such a short time period seems wrong.

cheers
Dan

Hi all,

This RFC is to fix CVE-2014-1239.

https://wiki.php.net/rfc/multibyte_char_handling

Adding required functions to mbstring and provide them
from PHP 5.3 and up and replacing mbstring to mbstring-ng
for future releases.

Thank you for voting!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Dan,

This vote is not listed as 'In voting phase' on the RFC home page -
https://wiki.php.net/rfc

Thank you. Updated

Also, as Nikic mentioned, this RFC has changed from the original

request to merging a different library into PHP, 3 days ago. Voting on
such a change in such a short time period seems wrong.

I forgot a line for mbstring-ng implementation.
I've fixed voting date and extended 4 days.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Dan Ackroyd — view source — reply

unread

Compile mbstringp-ng as default compiled module, when mbstring-ng is ready.

How can people know what they're voting on when the module is
apparently not ready for use?

The referenced RFC (https://wiki.php.net/rfc/altmbstring) is at least
slightly ambigous, and which parts of it are going to be included in
the 'Multibyte Char Handling' vote ?

cheers
Dan

Hi Dan,

This vote is not listed as 'In voting phase' on the RFC home page -
https://wiki.php.net/rfc

Thank you. Updated

Also, as Nikic mentioned, this RFC has changed from the original
request to merging a different library into PHP, 3 days ago. Voting on
such a change in such a short time period seems wrong.

I forgot a line for mbstring-ng implementation.
I've fixed voting date and extended 4 days.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Dan Ackroyd — view source — reply

unread

Sorry, for rapid posting but I realise I should have been explicit in
my last message. The RFC being voted on has "Backward Incompatible
Changes: None."

The referenced RFC which apparently would be included has:

"Removed (deprecated) functions and reasons behind it

mb_check_encoding() – Not that usable as it is advertised, period.
First of all, validation in terms of encoding is just as same as
filtering through the converter supplied with the same value for the
input and output encoding. Thus just use mb_convert_encoding().
mb_convert_case() – Use mb_strtoupper(), mb_strtolower() and mb_strtotitle()
mb_convert_kana() – This can't be standard-compliant. In addition,
part of the functionality is already covered by Normalizer of intl
extension, so we need to carefully consider what is actually needed
here again.
mb_convert_variables() – This can be implemented as a script.
mb_decode_mimeheader() and mb_encode_mimeheader() – Non-standard compliancy.
mb_decode_numericentity() – Removed in favor of html_entity_decode().
mb_encode_numericentity() – Removed in favor of htmlentities() and
htmlspecialchars().
mb_encoding_aliases() – Just unnecessary.
mb_ereg_match() – Use mb_ereg()
mb_ereg_search(), mb_ereg_search_getpos(), mb_ereg_search_getregs(),
mb_ereg_search_init(), mb_ereg_search_pos(), mb_ereg_search_regs() and
mb_ereg_search_setpos() – I rarely heard a script that actively uses
these functions. They involve an internal state that is not visible to
users, and thus it most likely causes confusion when used across the
function calls. Need to be reimplemented as a class.
mb_eregi() – Use mb_regex_options() and mb_ereg()
mb_eregi_replace() – I wonder why this function was added in the first
place because giving 'i' option to mb_ereg_replace() works in the same
way.
mb_detect_order(), mb_get_info(), mb_http_input(), mb_http_output(),
mb_language() and mb_substitute_character() – ini_set() and ini_get()
are your friends, I guess…
mb_regex_encoding() – It is really confusing that the current mbstring
allows two different encoding defaults for regex functions and the
rest. Those settings are unified in the alternative version and so
this is no longer necessary.
mb_send_mail() – The behavior of this function relies on the
pseudo-locale setting called “mbstring.language” that supports just a
limited set of possible locales. As not everyone can benefit from the
function and most significant applications implement their own mail
functions, I suppose this is no longer wanted.
mb_strrchr() – Use mb_strrpos().
mb_strrichr() – Use mb_strripos()."

None is not the same as a huge number of function changes.

cheers
Dan
Ack

Compile mbstringp-ng as default compiled module, when mbstring-ng is ready.

How can people know what they're voting on when the module is
apparently not ready for use?

The referenced RFC (https://wiki.php.net/rfc/altmbstring) is at least
slightly ambigous, and which parts of it are going to be included in
the 'Multibyte Char Handling' vote ?

cheers
Dan

Hi Dan,

This vote is not listed as 'In voting phase' on the RFC home page -
https://wiki.php.net/rfc

Thank you. Updated

Also, as Nikic mentioned, this RFC has changed from the original
request to merging a different library into PHP, 3 days ago. Voting on
such a change in such a short time period seems wrong.

I forgot a line for mbstring-ng implementation.
I've fixed voting date and extended 4 days.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Dan,

Sorry, for rapid posting but I realise I should have been explicit in
my last message. The RFC being voted on has "Backward Incompatible
Changes: None."

The referenced RFC which apparently would be included has:

"Removed (deprecated) functions and reasons behind it

mb_check_encoding() – Not that usable as it is advertised, period.
First of all, validation in terms of encoding is just as same as
filtering through the converter supplied with the same value for the
input and output encoding. Thus just use mb_convert_encoding().
mb_convert_case() – Use mb_strtoupper(), mb_strtolower() and
mb_strtotitle()
mb_convert_kana() – This can't be standard-compliant. In addition,
part of the functionality is already covered by Normalizer of intl
extension, so we need to carefully consider what is actually needed
here again.
mb_convert_variables() – This can be implemented as a script.
mb_decode_mimeheader() and mb_encode_mimeheader() – Non-standard
compliancy.
mb_decode_numericentity() – Removed in favor of html_entity_decode().
mb_encode_numericentity() – Removed in favor of htmlentities() and
htmlspecialchars().
mb_encoding_aliases() – Just unnecessary.
mb_ereg_match() – Use mb_ereg()
mb_ereg_search(), mb_ereg_search_getpos(), mb_ereg_search_getregs(),
mb_ereg_search_init(), mb_ereg_search_pos(), mb_ereg_search_regs() and
mb_ereg_search_setpos() – I rarely heard a script that actively uses
these functions. They involve an internal state that is not visible to
users, and thus it most likely causes confusion when used across the
function calls. Need to be reimplemented as a class.
mb_eregi() – Use mb_regex_options() and mb_ereg()
mb_eregi_replace() – I wonder why this function was added in the first
place because giving 'i' option to mb_ereg_replace() works in the same
way.
mb_detect_order(), mb_get_info(), mb_http_input(), mb_http_output(),
mb_language() and mb_substitute_character() – ini_set() and ini_get()
are your friends, I guess…
mb_regex_encoding() – It is really confusing that the current mbstring
allows two different encoding defaults for regex functions and the
rest. Those settings are unified in the alternative version and so
this is no longer necessary.
mb_send_mail() – The behavior of this function relies on the
pseudo-locale setting called “mbstring.language” that supports just a
limited set of possible locales. As not everyone can benefit from the
function and most significant applications implement their own mail
functions, I suppose this is no longer wanted.
mb_strrchr() – Use mb_strrpos().
mb_strrichr() – Use mb_strripos()."

None is not the same as a huge number of function changes.

I just didn't want to touch 5 year old RFC.
As I wrote in parent RFC, the implementation is subject to be changed.

The objective of this RFC is killing the vulnerability completely.
It's better to have road map for it.

As I wrote, there is license difficulty to compile current mbstring by
default.
There is mbstring-ng, but it's incomplete. This RFC is only proposing
feasible option.

I'm going to copy all mbstring features to mbstring-ng, but there may be
some compatibility issue. e.g. Non character encoding handling.

There will be another vote when we replace mbstring and mbstring-ng
actually, since this RFC only proposes the way to go. I don't think
this RFC is the approval for replacement.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Dan,

I just didn't want to touch 5 year old RFC.
As I wrote in parent RFC, the implementation is subject to be changed.

I've updated altmbstring RFC as this RFC name implies. Original author
was intended to provide alternative to mbstring.

In contrast, my RFC is intended to replace mbstring by mbstring-ng.
There are technical difficulties to copy all of mbstring feature to
mbstring-ng,
we may evaluate them and decide migration procedure when mbstring-ng is
ready.

Choices would be

provide mbstring-ng as alternative of mbstring
- keep mbstring-ng in php-src, move mbstring to PECL
- keep both mbstring-ng and mbstring in php-src
replace mbstring by mbstring-ng and move mbstring to PECL as
mbstring-legacy.

I'm not sure which one is the best. We may try to find out by providing
mbstring-ng
as EXPERIMENTAL module.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Dan Ackroyd — view source — reply

unread

Yasuo wrote:

In contrast, my RFC is intended to replace mbstring by mbstring-ng.

So is this what the current vote is on? If so the "Backward
Incompatible Changes: None." is wrong.

If the vote is only on adding the functions:

mb_escape_shell_arg()
mb_secape_shell_cmd()
mb_fget_csv()
mb_addslashes()
mb_var_export()
mb_stripslashes()

then all of the other suggestions about removing mbstring and
integrating mbstring-ng should not be in this RFC.

cheers
Dan

12 years ago by Christopher Jones — view source — reply

unread

Hi Dan,

I just didn't want to touch 5 year old RFC.
As I wrote in parent RFC, the implementation is subject to be changed.

I've updated altmbstring RFC as this RFC name implies. Original author
was intended to provide alternative to mbstring.

In contrast, my RFC is intended to replace mbstring by mbstring-ng.
There are technical difficulties to copy all of mbstring feature to
mbstring-ng,
we may evaluate them and decide migration procedure when mbstring-ng is
ready.

Choices would be

provide mbstring-ng as alternative of mbstring

keep mbstring-ng in php-src, move mbstring to PECL

keep both mbstring-ng and mbstring in php-src

replace mbstring by mbstring-ng and move mbstring to PECL as
mbstring-legacy.

I'm not sure which one is the best. We may try to find out by providing
mbstring-ng
as EXPERIMENTAL module.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Since this RFC (https://wiki.php.net/rfc/multibyte_char_handling)
depends on another unfinalized one
(https://wiki.php.net/rfc/altmbstring), surely it is too premature to
be voting on https://wiki.php.net/rfc/multibyte_char_handling now?

The situation is just confusing and counter productive.

I quote from the RFC:

 Compile mbstringp-ng as default compiled module, when mbstring-ng
 is ready. See following FRC [sic] for mbstring-ng details.

 Alternative implementation of mbstring using ICU [the previous
 sentence is a link to https://wiki.php.net/rfc/altmbstring]

 Until mbstring-ng is ready, mbstring-ng is provided as
 EXPERIMENTAL module.

 mbstring-ng implementation is subject to be changed.

Chris

P.S. Am I the only one getting confused by a plethora of RFCs and
ideas and last minute changes? If an RFC is changed materially, then
the vote should be stopped and restarted after an appropriate time for
discussion.

P.P.S. Some confusion could be avoided if the second part of #18
(about including the URL in emails) was followed from:
https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process
This is a common issue with RFC discussions on internals.

--
christopher.jones@oracle.com http://twitter.com/ghrd
Free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Christopher,

On Wed, Jan 29, 2014 at 5:24 AM, Christopher Jones <
christopher.jones@oracle.com> wrote:

Since this RFC (https://wiki.php.net/rfc/multibyte_char_handling)
depends on another unfinalized one
(https://wiki.php.net/rfc/altmbstring), surely it is too premature to
be voting on https://wiki.php.net/rfc/multibyte_char_handling now?

The situation is just confusing and counter productive.

I asked comments, but people tends to read RFC actually when
vote is started...

I don't mind at all change status to under discussion, though.

Since compiling current mbstring by default is not good idea for some
users, it would be nice to have load map for this issue. Making mbstring-ng
mature is impossible in short term. We do need some experiments and
feedbacks if it's really possible.

Do you have any suggestion for this?

I quote from the RFC:

Compile mbstringp-ng as default compiled module, when mbstring-ng
is ready. See following FRC [sic] for mbstring-ng details.

Alternative implementation of mbstring using ICU [the previous
sentence is a link to https://wiki.php.net/rfc/altmbstring]

Until mbstring-ng is ready, mbstring-ng is provided as
EXPERIMENTAL module.

mbstring-ng implementation is subject to be changed.
Chris

P.S. Am I the only one getting confused by a plethora of RFCs and
ideas and last minute changes? If an RFC is changed materially, then
the vote should be stopped and restarted after an appropriate time for
discussion.

I agree. Let's discuss!

P.P.S. Some confusion could be avoided if the second part of #18
(about including the URL in emails) was followed from:
https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process
This is a common issue with RFC discussions on internals.

I'll add more threads to the RFC. Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

Since there are comments that there should be 2 RFCs.
Here is 2 RFCs for multibyte encoding handling security issues.

Short term resolution - add functions to mbstrings
https://wiki.php.net/rfc/multibyte_char_handling

Long term resolution - compile mbstring-ng as default (replace mbstring if
possible)
https://wiki.php.net/rfc/altmbstring

I would like to start vote soon.
Please comment now.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Hi all,

This RFC is to fix CVE-2014-1239.

https://wiki.php.net/rfc/multibyte_char_handling

Adding required functions to mbstring and provide them
from PHP 5.3 and up and replacing mbstring to mbstring-ng
for future releases.

Thank you for voting!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

Since there are comments that there should be 2 RFCs.
Here is 2 RFCs for multibyte encoding handling security issues.

Short term resolution - add functions to mbstrings
https://wiki.php.net/rfc/multibyte_char_handling

Long term resolution - compile mbstring-ng as default (replace mbstring if
possible)
https://wiki.php.net/rfc/altmbstring

I would like to start vote soon.
Please comment now.

Thank you.

This is the last notice before reopen vote.
Please comment if there is.
Since the RFC has changed, users who vote already need
to vote again.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Stas Malyshev — view source — reply

unread

Hi!

This is the last notice before reopen vote.
Please comment if there is.
Since the RFC has changed, users who vote already need
to vote again.

One note: PHP 5.3 is now only for critical security issues. This is not
a critical security issue, it is an API improvement, so I doubt it can
get into 5.3. 5.4 would be OK though if accepted.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Stas,

On Wed, Feb 5, 2014 at 10:24 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

This is the last notice before reopen vote.
Please comment if there is.
Since the RFC has changed, users who vote already need
to vote again.

One note: PHP 5.3 is now only for critical security issues. This is not
a critical security issue, it is an API improvement, so I doubt it can
get into 5.3. 5.4 would be OK though if accepted.

I can agree as users may work round by themselves. They may make
their own secure functions if it is needed.

Changed.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net