[MAGNOLIA-3805] Page titles are not HTML encoded if a user cancels the inline editing in the tree Created: 22/Aug/11 Updated: 16/Sep/11 Resolved: 07/Sep/11 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | admininterface |
| Affects Version/s: | 4.4.4 |
| Fix Version/s: | 4.4.5 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Rory Gibson | Assignee: | Ondrej Chytil |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Template: |
|
| Acceptance criteria: |
Empty
|
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
| Date of First Response: |
| Description |
|
Steps to reproduce:
This could be potentially harmful to the system. |
| Comments |
| Comment by Magnolia International [ 23/Aug/11 ] |
|
Are we talking about STK templates ? Is this reproducible with all templates ? |
| Comment by Rory Gibson [ 23/Aug/11 ] |
|
Hi Gregory
I've also tried entering '<strong onclick='alert()'>some text</strong>' and this results in an alert box appear upon click on the title. It looks like validation is different on blur than submit. Also - if you enter two HTML'ed titles, pressing enter for each one (so that they are rendered normally, escaped), then, when you break one of them by blurring focus, and go back into the other one, the behaviour upon pressing Enter is now incorrect. Looks like there may be something being stored statically that's being affected. |
| Comment by Rory Gibson [ 23/Aug/11 ] |
|
Note: i've just replicated this in the demoauthor instance - it should be there for the next few minutes. |
| Comment by Magnolia International [ 23/Aug/11 ] |
|
Oh wow ok, I get it now. |
| Comment by Magnolia International [ 23/Aug/11 ] |
|
Tentatively scheduled for 4.4.5 but might be bumped to next version. |
| Comment by Jan Haderka [ 07/Sep/11 ] |
|
don't use escape ... there must be a better way. |
| Comment by Richard Unger [ 16/Sep/11 ] |
|
Hi Gregory, hi Jan, We're having some serious discussions regarding security that are related to this bug: In a recent internal security check (we're currently using 4.4.2 EE) we also noticed that it is possible to insert javascripts into the title fields in the admin trees. The inserted scripts are executed within the backend, already a problem in itself, but are also executed in the front-ends, when the title fields are rendered in the web-pages (this, of course depends on how the templates are written, admittedly). One question is, of course, whether it makes sense to secure the back-end against the editors, who are after all authorized users, and should be trusted. But setting aside this question, which is more philosophical than technical, I have the following technical question: Examining the subversion commits related to this bug it seems the only modification associated with this bug-fix is to the file "/magnolia-module-admininterface/src/main/resources/mgnl-resources/admin-js/tree.js". If this is the case, then I contend that this is not a fix, but merely a "cosmetic change". If we're assuming "evil editors", attacking the system with javascript, they can simply use firebug (or similar tools) to modify the behaviour of tree.js, and disable your angle-bracket removal code. Or they can skip past the all checks by intercepting the communication resulting from a tree-edit, and supplying modified values after all checks have been perfomed. To really fix this bug there needs to be a change on the server side, and inputs need to be escaped by the server, before storing to JCR. I'd appreciate feedback on whether the subversion commits attached to this bug are "complete", and I am correct in my assumtion that the only change that was made is in the java-scripts, i.e. client-side... Thanks! Regards from Vienna, Richard |
| Comment by Jan Haderka [ 16/Sep/11 ] |
|
Hi Richard, even if you subvert the javascript in the browser you can do so only for your browser, since script is executed even when reading values from server to display them in the tree. This way you are able to store the html in the node value when necessary, but it would still not execute for other users in admin central. That being said, we are still preparing solution on the server side, but since sometimes (e.g. html paragraph) it is perfectly valid to store the pure html, we can's just blindly switch it off for all. Regards, |
| Comment by Richard Unger [ 16/Sep/11 ] |
|
Hi Jan, ok, thank you very much - I understand. Thanks also for the information on this and related issues today, that was very helpful, Regards from Vienna, Richard |