Fix escaping of $ and reverts old language file format

[dryabov]

The recent change in the language file format (from the "normal" to "raw" parser) in Joomla 4.4.1/5.0.1 resulted in broken backwards compatibility:

• Escaped backslashes are not handled
• Multiline values are not supported
• Single quoted strings are not supported

There are many related discussions and attempts to "fix" new behaviour: #42416 #42425 #42432 #42440 #42441 #42455 #42456

This patch fixes the escaping of the $ character in the Language Override and reverts the old language file format.

For thorough testing, I'd recommend the following steps:

1. Install 5.0.0 -> install patch -> confirm that no errors occur.
2. Install 5.0.1 -> install patch -> confirm that no errors occur.
3. Install 5.0.0 -> create a language override with ${test} substring (invisible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.
4. Install 5.0.1 -> create a language override with ${test} substring (visible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.

PS. But using a raw ini parser (without post-processing language strings) is a good idea, so I have started a feature request discussion (please join): #42462

[brianteeman]

this reverts a security fix

[dryabov]

this reverts a security fix

It provides another solution via proper escaping of saved strings in the Language Override component.

[dryabov]

And a complete transition to the raw parser is proposed for Joomla 6.0, with a description of how to do this correctly by carefully converting language strings to the new format.

[SniperSister]

The current filter does not properly filter overrides while saving the string. Prefixing the dollar in the string with a slash will lead to another slash being added while saving the string, leading to the escaping of the slashes but not of the dollar char.

[dryabov]

Yes, I forgot that addcslashes doesn't escape backslashes automatically. Fixed now.

[SniperSister]

Yes, I forgot that addcslashes doesn't escape backslashes automatically. Fixed now.

Thank you!

I played around with various payloads but was unable to inject a working placeholder - so from my side this solution is ok

[dryabov]

I've just fixed the issue with automatically regenerating language override files on upgrade (to make sure payloads are removed if they exist). So the patch is ready for all kinds of testing.

[dryabov]

@bembelimen What do you think of this patch? JSST has confirmed that this patch fixes the vulnerability that was attempted to be fixed in versions 4.4.1 and 5.0.1. At the same time, this patch fully restores the original language file parser, i.e. all compatibility issues are resolved. Also note that the 5.0.1 patch negatively impacts performance, and some people are now wasting their time creating compatibility workarounds that affect performance even more, so the sooner this patch is merged, the better.

PS. I would also be interested to hear your opinion on the proposal to actually switch to the raw ini parser in the next version of Joomla: #42462.

[wilsonge]

We still need to fix new language overrides being created. Currently this retrospectively fixes all existing language strings only.

Other than that +1 from me

[dryabov]

We still need to fix new language overrides being created. Currently this retrospectively fixes all existing language strings only.

New overrides are fixed by addcslashes($string, '"$\\') (i.e. all $ are properly escaped).

[bembelimen]

Hi @dryabov ,

thank you for your PR. One thing is to consider: what about 3rd party translations of either core or extensions? I think you have to consider them, too.

[dryabov]

One thing is to consider: what about 3rd party translations of either core or extensions? I think you have to consider them, too.

Yes, I discussed it with David Jardin. Installing 3rdparty extensions requires Super Administrator role, so it's not a problem. However Language Overrides requires Administrator role, that's why it was considered a vulnerability.

[bembelimen]

If we get two tests, including the migration step (e.g. screenshots before/after), I'm happy to merge.

[richard67]

If we get two tests, including the migration step (e.g. screenshots before/after), I'm happy to merge.

Maybe it would be easier to get tests if there were some testing instructions. However, the author has removed all sections related to that which were provided by the pull request template.

[dryabov]

For thorough testing, I'd recommend the following steps:

1. Install 5.0.0 -> install patch -> confirm that no errors occur.
2. Install 5.0.1 -> install patch -> confirm that no errors occur.
3. Install 5.0.0 -> create a language override with ${test} substring (invisible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.
4. Install 5.0.1 -> create a language override with ${test} substring (visible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.

[richard67]

For thorough testing, I'd recommend the following steps:

1. Install 5.0.0 -> install patch -> confirm that no errors occur.

2. Install 5.0.1 -> install patch -> confirm that no errors occur.

3. Install 5.0.0 -> create a language override with `${test}` substring (invisible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.

4. Install 5.0.1 -> create a language override with `${test}` substring (visible in the Languages Overrides after saving) -> install patch -> the substring is visible in the Languages Overrides.

@dryabov Could you add that also to the description (initial post) of this PR? Thanks in advance.

[dryabov]

@dryabov Could you add that also to the description (initial post) of this PR? Thanks in advance.

Done.

[bembelimen]

Hello @dryabov ,
we had a very long and heated discussion about your PR in the maintainer team and at the end we decided that we will not change the "RAW mode" of the language file parser because of many reasons. One of the reasons is your issue here, raw mode is the better way and should be the way forward and now after 6 weeks (or 12, as this PR would not make it in the upcoming release), it's easier to just fix the 3rd party extension.

Thank you very much für your idea, code and discussion, really appreciate it.

[dryabov]

Hmm, the RAW mode is only better if it is not accompanied by post-processing, but currently Joomla runs str_replace on an array with a huge number of strings, which is very, very slow. Did you make an estimate of how many language strings are included in Joomla and the most popular extensions before you adopted the original 5.0.1 patch? I thought Joomla was designed with maximum performance in mind....
Another problem is that I now have to add support to JEDChecker to identify strings in the old format that cannot be read by the new parser, which is not so easy. And until then, extension developers don't have the ability to automatically detect a whole class of related errors.

[bembelimen]

Would you look into the str_replace, how we could overcome that problem in an easy way?

[dryabov]

Okay, if this is the final decision, then you have two options:

1. In addition to de-escaping the double quotes, it's essential to handle \\ and \$ the same way to maintain backwards compatibility (unfortunately, in the case of 5.0.1 patch there is no easy-way to maintain backwards compatibility without str_replace).
2. Or, write a post in the Joomla News channel about the backwards compatibility break starting with versions 4.4.1 and 5.0.1, because now the \ and $ characters in language files must be encoded differently (without escaping). It's important for all extension developers to review their language files accordingly. In addition, developers of platforms such as Crowdin and similar services should develop the feature to export strings in the new format.

[dryabov]

Would you look into the str_replace, how we could overcome that problem in an easy way?

As far as I know from the PHP sources, using str_replace is the fastest option, because there is a short-circuit check that the substring is actually in the string. An alternative is strtr, but it has no short-circuit check and does not support processing of array of strings, and iterating the array at VM level (i.e. foreach) is obviously much slower. For the same reason, strip(c)slashes functions are not an option. It's also obvious that preg_replace will be even slower because it uses PCRE bytecode instead of pure C code (PCRE has a good JIT compiler, but anyway pure C is faster).

So there is no alternative, and you have a choice between current 5.0.1 code

$strings = str_replace('\"', '"', $strings);

which breaks backwards compatibility by not stripping escaped \ and $, and

$strings = str_replace(
        array('\"', '\$', '\\\\'),
        array('"',  '$',  '\\'),
        $strings);

which at least partially maintains backwards compatibility, but is obviously slower due to the additional replacements.