php-gettext

Use of eval too unrestrictive

Bug #1606184 reported by Данило Шеган
268
This bug affects 3 people
Affects Status Importance Assigned to Milestone
php-gettext
Triaged
Critical
Данило Шеган

Bug Description

php-gettext code that parses the plural forms header relies on eval() and only filters out some known-bad characters before passing the value from a MO file in directly to eval().

It was assumed that PO and MO files would come from trusted translators, without any attempts to potentially exploit the service being translated. While there was still no case where this has been exploited, we should move to the same parsing logic gettext C library uses:

  * http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural-exp.c
  * http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural-exp.h
  * http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural.y

This has been reported by Jean-Marie Bourbon as affecting NagVis, which has since fixed it by commenting out all the plural forms code: https://github.com/NagVis/nagvis/commit/4fe8672a5aec3467da72b5852ca6d283c15adb53

Jean-Marie was also kind enough to start a CVE 2016-6175 for this issue.

See original description

CVE References

Revision history for this message
Данило Шеган (danilo) wrote :
Данило Шеган (danilo)
description: updated
information type: Private Security → Public Security
Revision history for this message
Michal Čihař (nijel) wrote :

Removing usage of eval() was one of reasons why we've forked php-gettext at phpMyAdmin, see https://github.com/phpmyadmin/motranslator/. It is not a drop in replacement, but can provide compatible API.

Revision history for this message
Sunil Mohan Adapa (f-su7il-g) wrote :

Applications such as Tiny Tiny RSS continue to use the php-gettext library. Due this issue, they are are at risk of not getting included in next release of Debian and FreedomBox.

To address this, I have implemented a parser for the plurals expressions instead of using the eval() method as discussed in this bug as solution. This patch is under the same license as php-gettext (GPLv2 or higher).

- A simple operator-precedence parser that prioritizes simplicity and readability. Avoid using eval() for evaluating plural expressions.
  - Fixes CVE-2016-6175.
  - Fixes upstream bug https://bugs.launchpad.net/php-gettext/+bug/1606184
  - Fixes Debian bug https://bugs.debian.org/851771

- Grammar for parsing code is same as the grammar for GNU gettext library: http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural.y

- Extensive tests for various locales with help from Unicode's plurals rules. Tests for invalid syntax and expression parsing.

Please consider applying the patch and making a new release of php-gettext. Many thanks to the authors for the original implementation.

Revision history for this message
Sunil Mohan Adapa (f-su7il-g) wrote :

Also tests need to be updated to run with recent versions of phpunit.

Revision history for this message
Sunil Mohan Adapa (f-su7il-g) wrote :

It would be nice to have these patches applied as Debian is planning to drop php-gettext for the next release Bullseye due this security issue.

Revision history for this message
Sunil Mohan Adapa (f-su7il-g) wrote :

The earlier patch contains a silly mistake I made. This version fixes the problem.

By the way, Debian package for php-gettext now carries this plural parser.

To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.
Subscribing...

Other bug subscribers

Remote bug watches

  • debbugs #851771
    [done grave jessie-ignore buster-ignore security stretch-ignore patch wheezy-ignore upstream] Edit

Bug watches keep track of this bug in other bug trackers.