Use htmlspecialchars on trans parameters and deprecate _raw parameter #31900
Conversation
mflasquin
commented
Mar 23, 2023
mflasquin
commented
| Questions | Answers |
|---|---|
| Branch? | develop |
| Description? | Use htmlspecialchars on trans parameters and deprecate _raw parameter, next decision of this PR #30415 |
| Type? | improvement |
| Category? | BO |
| BC breaks? | yes |
| Deprecations? | yes |
| How to test? | See #30415 and #28877 and #29959 and #29940 |
| Fixed ticket? | Fixes #30539 |
| Related PRs | #30415 |
| Sponsor company | Prestashop SA |
prestonBot
added
develop
lartist
added
the
Waiting for QA
There was a problem hiding this comment.
Hello @mflasquin ,
Checked all of the issues mentioned in the PR : #28877 and #29959 and #29940
One issue still remains : #29959
The rest looks as expected.
Could you check please ?
Thanks!
florine2623
added
Waiting for author
|
Hi @florine2623, I can't reproduce the issue : Can you make another test ? Or we can discuss about it directly |
mflasquin
added
Waiting for QA
florine2623
added
Waiting for author
|
As discussed with @mflasquin , The issue needs more investigation. Thanks! |
mflasquin
dismissed stale reviews from lartist and matthieu-rolland
via
999fef8
mflasquin
removed
the
Waiting for author
mflasquin
added
the
Waiting for QA
There was a problem hiding this comment.
Hello @mflasquin ,
The issues are well fixed , It's QA approved ✔️
Automated tests --> OK
Thank you for you PR 🚀
djoelleuch
added
QA ✔️
|
QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge. |
|
Thanks @mflasquin |
|
Doesn't this PR introduce a security issue, now translations are not HTML escaped, so translations are XSS vulnerable I believe it should be the opposite, the translation output should be html escaped but parameters should not, like this parameters can contain html but translations never can |
| $trans = $transMethod->invoke($newObject, '<a href="test">%d Succesful deletion "%s"</a>', [10, '<b>stringTest</b>'], 'Admin.Notifications.Success'); | ||
| $this->assertEquals('<a href="test">10 Succesful deletion "<b>stringTest</b>"</a>', $trans); | ||
|
|
||
| $trans = $transMethod->invoke($newObject, '<a href="test">%d Succesful deletion "%s"</a>', [10, '<b>stringTest</b>'], 'Admin.Notifications.Success'); | ||
| $this->assertEquals('<a href="test">10 Succesful deletion "<b>stringTest</b>"</a>', $trans); | ||
| $trans = $transMethod->invoke($newObject, '<a href="test">%d Succesful deletion "%s"</a>', [10, htmlspecialchars('<b>stringTest</b>')], 'Admin.Notifications.Success'); | ||
| $this->assertEquals('<a href="test">10 Succesful deletion "<b>stringTest</b>"</a>', $trans); |
There was a problem hiding this comment.
This test should normally look like this for XSS to be mitigated, so that when you want to output html tags, you can do so in parameters
$trans = $transMethod->invoke($newObject, '<a href="test">%d Succesful deletion "%s"</a>', [10, '<b>stringTest</b>'], 'Admin.Notifications.Success');
$this->assertEquals('<a href="test">10 Succesful deletion "<b>stringTest</b>"</a>', $trans);Like this there is no need for _raw and translations can't by themselves inject html unless the parameters are not escaped, I'd still probably escape each parameter by default and provide a parameter to allow parameters to not be escaped making it much safer
There was a problem hiding this comment.
@Tofandel This PR is the final result of years long discussion 😄 see #30415 (review)
Yes it introduces a security issue but this is because it fixes a design issue. A necessary security step was added in the wrong place and we now remove it from this wrong place. The escaping should not happen inside trans().
There was a problem hiding this comment.
The escaping should not happen inside trans()
It should, but the escaping is indeed in the wrong place, you cannot be sure that what you are getting from a translation is safe
So to achieve that you escape the output of the retrieved translation but do not escape the parameters
This is the only way to achieve XSS safety
I can provide a super simple XSS demo with this, and it should not be released like this, that's a 7 out of 10 CVE because it allows translators of a site to inject html anywhere they want and then gain unauthorized access by data exfiltration
There was a problem hiding this comment.
trans() is a function that is supposed to translate strings. It has nothing to do with security. Security should not be handled inside trans().
Escaping properly HTML before sending it to user is not the responsibility of the translator, it is the responsibility of the controller/view layer.
I can provide a super simple XSS demo with this, and it should not be released like this, that's a 7 out of 10 CVE because it allows translators of a site to inject html anywhere they want and then gain unauthorized access by data exfiltration
From the Back-office, it is possible to upload a SVG image which can carry a XSS too. Translators can only "inject" HTML if they provide translations that are being imported into PrestaShop from the Back-office. That means they have been trusted by the Back-office administrator to be granted access to the Back-office or he trusted them when they provided translation files. If they have been trusted but they use this trust to inject HTML, the problem is that the Back-office administrator trusted the wrong persons, it's not a problem with the Back-office.
You can see translation files as inputs just like modules. If a merchant installs a garbage module, it could crash his shop, it could expose vulnerable code that makes his shop vulnerable. The problem is not that PrestaShop did not "prevent" or "validate" the wrong module: installing only trusted modules is the responsibility of the shop owner.
There was a problem hiding this comment.
I'll have to disagree, in big companies access control is very important. It's even common to outsource translation to freelancers that should only be given permission to the translation in the back office and nothing else
But if now they start to also have the ability to inject scripts transparently wherever they please without anyone noticing, effectively installing backdoors then there is a big problem
Translation should never allow html in from the translator side, translators have no business touching html markup, so it should be escaped. The global output of the translation though shouldn't be escaped, just the original translation string with the placeholders still in them
If html needs to be used for links or other, that's when the placeholders and parameters come in handy because those which are provided by the code will not be escaped
My point being, this design issue has a much safer possible solution, so why not use that instead of introducing an XSS willingly? That makes no sense
The argument being used is basically, prestashop is already unsafe, so it doesn't matter if we make it more unsafe. When it should be prestashop has some safety issues, we should aim to make it safer
There was a problem hiding this comment.
This PR is the last step of a discussion and analyzis that has been going on since October 2022 so please trust us when I tell you it's the best compromise we could get 😉
Escaping HTML? It's a good thing.
Doing it inside trans() ? It's not a good idea. It brings you hundred of other problems.
The idea of this PR is not "let's remove HTML escaping" it's "let's move the HTML escaping to the right place".
If you do not understand the reasons behind it, I can show you the history:
- bug BO/FO- HTML tags are not well displayed in different pages #29940
- 1st attempt to fix it, this is where idea to remove escape from
trans()Fixed TranslatorTrait to correctly render html tags on BO #29943 - related problem Fix duplicate / wrongly pluralized error message on field length in BO #29584
- PR Fix applying special char on translations #30415 for 8.0.0
- Issue Delete _raw parameter in trans methods #30539
Sorry I don't want to continue writing hundred words of explanation ^^ this is a topic that span for 8 months now so I have not enough motivation to tell you every aspects of it that brought us to this conclusion.
If, after reading all of the history, you can provide an interesting solution that meets all criteria, including fixing bug #29940 without breaking something else, then I'll be happy to review it 😉
There was a problem hiding this comment.
Doing it inside trans() ? It's not a good idea. It brings you hundred of other problems.
It's not doing it inside of trans that gives problems, it's doing it on the whole content of trans
The idea of this PR is not "let's remove HTML escaping" it's "let's move the HTML escaping to the right place".
The problem with this decision is as before, you can only either escape everything or not escape anything, the right place to escape is as I explained, inside trans but not to it's entire content, so that translations and parameters can be escaped separately, so trans needs a parameter to escape the content of the string without escaping the content of parameters, which would move the logic more nested inside the trans function instead of just blindly escaping everything, which would entirely solve the XSS issue and all the problems previously listed
I already read all the history and provided my solution in written form, which now with this PR should be straightforward to implement and would not introduce a breaking change
I'd be happy to make a PR when I have time if you agree with the solution I'm giving, but please do consider the security implications from this and hold on a release until the PR to fix this
There was a problem hiding this comment.
Hi @Tofandel I just wanted to tell you I don't forget you, this message is inside my todolist and I have requested the help of someone else on this topic because I can't handle everything.
