[issue28563] Arbitrary code execution in gettext.c2py

Changes by Donald Stufft : -- pull_requests: +1039 ___ Python tracker ___ ___

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: Yes, you are right, the stacklevel should be 4. Thank you for your report and testing Tim. Committed the patch without any deprecation. Will add a deprecation in separate issue. -- stage: patch review -> resolved status: open -> closed

[issue28563] Arbitrary code execution in gettext.c2py

Roundup Robot added the comment: New changeset 5b6cde995b3a by Serhiy Storchaka in branch '3.3': Issue #28563: Make plural form selection more lenient and accepting https://hg.python.org/cpython/rev/5b6cde995b3a New changeset 6ca91a14a555 by Serhiy Storchaka in branch '2.7': Issue #28563: Make

[issue28563] Arbitrary code execution in gettext.c2py

Tim Graham added the comment: Yes, that fixes the second test. Current warning (with stacklevel=3): /home/tim/code/cpython/Lib/gettext.py:454: DeprecationWarning: Plural value must be an integer, got 1.29 tmsg = self._catalog[(msgid1, self.plural(n))] Possibly the stacklevel should instead

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: What about following patch? -- Added file: http://bugs.python.org/file45482/gettext-non-integer-plural-2.patch ___ Python tracker

[issue28563] Arbitrary code execution in gettext.c2py

Tim Graham added the comment: Thanks, that does fix that first test. There is one more that still fails: $ python -Wall tests/runtests.py humanize_tests.tests.HumanizeTests.test_i18n_intword Testing against Django installed in '/home/tim/code/django/django' with up to 3 processes Creating

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: Proposed patch makes gettext accepting non-integer numbers. But DeprecationWarning is emitted if this number have fractional part (because in this case the result of current code can be different from the result of old code). I think Django tests should be

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: GNU gettext only allows the plural value(n) to be an integer(unsigned long int). Django deliberately converts the value to long in filesizeformat. Any reason not to use int? -- ___ Python tracker

[issue28563] Arbitrary code execution in gettext.c2py

Tim Graham added the comment: Hi, this broke a couple tests with Django because it's passing number as a float rather than an integer. For example: == ERROR: test_localized_formats

[issue28563] Arbitrary code execution in gettext.c2py

Changes by Serhiy Storchaka : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue28563] Arbitrary code execution in gettext.c2py

Roundup Robot added the comment: New changeset e0cc3fadd7b3 by Serhiy Storchaka in branch '2.7': Issue #28563: Fixed possible DoS and arbitrary code execution when handle https://hg.python.org/cpython/rev/e0cc3fadd7b3 New changeset 7e66c5dc4218 by Serhiy Storchaka in branch '3.3': Issue #28563:

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: > What a comment you need Xiang? Isn't existing comment enough? Serhiy, I mean the case a number starting with 0, e.g. 0123. The plural form is a C expression and in C 0123 is an octal number. c2py now interprets it as a decimal number. --

[issue28563] Arbitrary code execution in gettext.c2py

Carl Ekerot added the comment: Looks good to me. It behaves as intended on every input I can think of. -- ___ Python tracker ___

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: What a comment you need Xiang? Isn't existing comment enough? -- ___ Python tracker ___

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: LGTM. And I expect there could be a comment about the special decimal number. -- ___ Python tracker ___

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: Just for reference: https://www.gnu.org/software/gettext/manual/gettext.html#Plural-forms http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural.y Yet one security issue is that too deep recursion can cause MemoryError or even a crash

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: > Sorry Xiang, but your patch looks overcomplicated to me. Too much methods, > decorators, classes, too much strange names. It's fine. That's a Pratt parser. Yes, the names are strange. Your patch looks more simpler. I left several comments. --

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: Sorry Xiang, but your patch looks overcomplicated to me. Too much methods, decorators, classes, too much strange names. Here is simpler implementation with using only one recursive function. -- Added file:

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: > Perhaps it should give a DeprectationWarning and delegate to _Plural? I hold a conservative opinion about this. c2py in my mind should be a inner help method. It's not documented so if there are users using it, they are risking changes. And as reported here,

[issue28563] Arbitrary code execution in gettext.c2py

Carl Ekerot added the comment: Judging by the code, this seems to be a much more rigid implementation. I've only run the unit tests and some variations of my initial examples, and everything seems to work as intended. Will look at it more closely this afternoon. One thing that caught my

[issue28563] Arbitrary code execution in gettext.c2py

Changes by Serhiy Storchaka : -- assignee: -> serhiy.storchaka stage: -> patch review ___ Python tracker ___

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: gettext_c2py_v2.patch implements a simple C expression parser. More tests are included. Carl, hope you are willing to test it. -- Added file: http://bugs.python.org/file45373/gettext_c2py_v2.patch ___ Python tracker

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: Christian, I think our patches are quite similar in function. They only allow limited tokens. > I consider it a superior solution and a fix for more generic attacks Mine now still allows **. But it can be easily fixed. But both our patches still translate a C

[issue28563] Arbitrary code execution in gettext.c2py

Christian Heimes added the comment: Argh, sorry. I meant to write "The gettext module might be vulnerable to more than f-string attacks.". May I suggest that you have a look at my old patch? It uses an AST visitor to inspect the AST of a gettext plural expression. It allows only a limited set

[issue28563] Arbitrary code execution in gettext.c2py

Carl Ekerot added the comment: > The gettext module might be vulnerable to f-string attacks It is. See the example in the first comment: gettext.c2py('f"{os.system(\'sh\')}"')(0) This vulnerability seems to be solved in Xiang's patch. The DoS aspect is interesting though, since there's no

[issue28563] Arbitrary code execution in gettext.c2py

Christian Heimes added the comment: The gettext module might be vulnerable to f-string attacks, too. Also see #18317. -- nosy: +christian.heimes ___ Python tracker

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: > '1?2:3?4:5' -> '(2 if 1 else 3)?4:5' -> '(4 if (2 if 1 else 3) else 5' This is not right. It's right associative so it should be 1?2:(3?4:5) -> 1?2:(4 if 3 else 5) -> 2 if 1 else (4 if 3 else 5) > It would be nice to make c2py() working with any expressions,

[issue28563] Arbitrary code execution in gettext.c2py

Carl Ekerot added the comment: Verified gettext.c2py with gettext_c2py.patch applied agains the plural forms actually used in localization, listed over at http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html. I tested all of the none-trivial forms, and

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: > How does it work for '1?2:3?4:5'? '1?2:3?4:5' -> '(2 if 1 else 3)?4:5' -> '(4 if (2 if 1 else 3) else 5' But there are other problems. Precedence of some operators is different in C and Python. Chained comparison in Python cause different result that in C

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: > gettext.c2py("n()")(lambda: os.system("sh")) > gettext.c2py("1()")(0) Empty parentheses should be disallowed. Function calls are not allowed in plural expression. And non-integer argument should be disallowed either, just as Serhiy's example shows. > What if

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: > It doesn't solve the case when an identifier or number is used as a function: In the first case we should convert an argument to integer. ns = {} exec('''if True: def func(arg): n = int(arg) return {}

[issue28563] Arbitrary code execution in gettext.c2py

Serhiy Storchaka added the comment: > But there is still a problem. Both patched and original c2py fail to handle > nested ternary operator. They respect the right associative but fails to > handle expressions like '1?2?3:4:5'. What if make repeated replacements with regular expression

[issue28563] Arbitrary code execution in gettext.c2py

Carl Ekerot added the comment: It doesn't solve the case when an identifier or number is used as a function: >>> import os >>> gettext.c2py("n()")(lambda: os.system("sh")) $ 0 >>> gettext.c2py("1()")(0) Traceback (most recent call last): File "", line 1, in File

[issue28563] Arbitrary code execution in gettext.c2py

Xiang Zhang added the comment: gettext_c2py.patch tries to avoid the problem. It still uses eval but manually parse the expression using tokens extracted from gettext. Tests are passed. But there is still a problem. Both patched and original c2py fail to handle nested ternary operator. They

[issue28563] Arbitrary code execution in gettext.c2py

Changes by Xiang Zhang : -- nosy: +xiang.zhang ___ Python tracker ___ ___ Python-bugs-list

[issue28563] Arbitrary code execution in gettext.c2py

Changes by Serhiy Storchaka : -- nosy: +serhiy.storchaka priority: normal -> high ___ Python tracker ___

[issue28563] Arbitrary code execution in gettext.c2py

Changes by SilentGhost : -- nosy: +loewis ___ Python tracker ___ ___ Python-bugs-list

[issue28563] Arbitrary code execution in gettext.c2py

New submission from Carl Ekerot: The c2py-function in the gettext module is seriously flawed in many ways due to its use of eval to create a plural function: return eval('lambda n: int(%s)' % plural) My first discovery was that nothing prevents an input plural string that resembles a