ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.65k stars 3.71k forks source link

Link Feature: Possible Attack Vector When Editing Relative Links in Secured Environments #15258

Open mmichaelis opened 1 year ago

mmichaelis commented 1 year ago

πŸ“ Provide detailed reproduction steps (if any)

  1. Use CKEditor 5 within a secured environment to access possibly tainted data (user input from a website, moderated within a RIA).
  2. User input contains either perfectly valid relative links in context of original site (like /see/also/page), but may also contain attacking links like /admin/actions/delete-user?name=admin executed in secure environment (don't question "secure" in this context, if such requests are allowed).
  3. The default behavior of the Link Feature, will expose the relative link as "clickable" in a secure environment, possibly triggering the user to take malicious actions on a click.

βœ”οΈ Expected result

Some countermeasures exist, to prevent this.

❌ Actual result

No countermeasures exist. There is no "context-aware" resolving of relative links.

❓ Possible solution

Preferred Solution

Forbid clicking on relative links within the "Link Balloon": We do not want to mangle with the user generated content. The relative link is just fine, we just need to prevent an editorial staff to accidentally click on these.

This would also ease migration from CKEditor 4, as for CKEditor 4, it was never possible to just click and follow a given link from within the CKEditor 4 UI. Links always were just "plaintext". Thus, when migrating a "relative link support" from CKEditor 4 to CKEditor 5 you will suddenly face this new issue with possible attack vectors.

Filter Relative URLs

This is similar to:

But while ckeditor/ckeditor5#6016 only refers to editorial actions, this issue is also about incoming data, which not necessarily have been created by CKEditor 5 before.

As an effect, if you keep the "empty href", you will see some anchor-tag without URL.

This is the option, we will currently take as "least effort solution".

Configuration Option for "baseURL"

Add an option to the link feature, adding a "baseURL" for links. This may have several effects:

While it seems desirable at first glance, it would be no option for us to take, as we have no easy way to "fixate" baseURL in our system.

πŸ“ƒ Other details


If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

Witoso commented 1 year ago

Thanks for the detailed request!

Forbid clicking on relative links within the "Link Balloon": We do not want to mangle with the user generated content. The relative link is just fine, we just need to prevent an editorial staff to accidentally click on these.

I feel this would have a considerable impact on the user experience. Taking away a possibility to click links seems like a very β€œnuclear” option of solving this problem. It will solve it but everyone will hate how it does it.

(...) (user input from a website, moderated within a RIA).

Although I get all the points, I am not sure if the editor is the best place for data sanitization. Ideally, this should happen at the earlier stages.

Configuration Option for "baseURL"

I do feel that this is the best solution, similar to the defaultProtocol. Still, I think it would work only on the added links. Therefore, it wouldn't solve your specific problem.


Last thoughts, it does seem that some config option, disableLinkBallon, would need to be added. We need to gather interest, and discuss it internally.

mmichaelis commented 1 year ago

Perhaps another possible option to take: There is already some code in place to ensure "safe URLs" within the link feature, used within the editing layer:

https://github.com/ckeditor/ckeditor5/blob/899f2fd9d0964de0449a91182300b47d616fe97a/packages/ckeditor5-link/src/utils.ts#L73-L77

Adding some configurable behavior would allow generating "unclickable" links within the link editing UI. I think, this is where a configuration option like allowRelative or similar may be introduced. This could also affect hash-/search-parameters, such as URLs like ?action=delete-user&name=admin that may cause malicious behavior.

Having this, an option like "auto-complete" relative links would still be a nice option... but could also be solved in the data-processing layer (like toView resolves relative links to absolute ones, while toData could again scan for links to shorten to relative links).

Mgsy commented 1 year ago

I wonder whether making links unclickable will solve the problem, as in this case, the vulnerability seems to exist in the endpoint that allows executing ?action=delete-user&name=admin with no additional checks, e.g. CSRF token validation (https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html).

In theory, an attacker can prepare a malicious website and trick the user using social engineering to visit it, which will result in sending the request to the affected endpoint automatically.Β 

Even if the editor will block clicking on such links, there are other ways for triggering the request, so I believe it should be patched on the application side.

mmichaelis commented 1 year ago

I wonder whether making links unclickable will solve the problem

I agree regarding my examples, which may be bad ones: Of course, any environment embedding the CKEditor 5 should not (completely) rely on CKEditor 5 providing some suitable "firewall" here. Nevertheless, for some protocols (data: URLs, which pose a risk of embedding HTML within the data), CKEditor 5 already provides such a "firewall" with SAFE_URL/ensureSafeUrl (thanks for that, btw.).

Perhaps it really boils down to some awareness of relative links, as already sketched in #6016 (here, it was about disallowing editors from generating relative links; too narrow scope, I think, as it ignores systems, where CKEditor 5 is not the only source of data).

Given this, the issue may just have "too much drama" (although it matches my current challenge, of having to display highly uncontrolled data from unknown sources right within CKEditor 5).

It may be worth instead writing extra issues with clearer scope, then, which would better match the challenges for possibly tainted data:

A broader feature, that would incorporate both of the above, would be some configurable ViewUrlMapper that controls the URL as it is rendered within the editing layers. This could as well sanitize links, it could resolve relative links as well as it could open the door to some hardening towards link interaction (block known malicious URLs, show a pop-up You are going to an external site., ...).

If you agree, I will happily create corresponding issues, so that we may close this one as inappropriate (well, you could do so anyway πŸ˜„).

Witoso commented 1 year ago

If you agree, I will happily create corresponding issues, so that we may close this one as inappropriate (well, you could do so anyway πŸ˜„).

I think that would be the best next step.

CKEditorBot commented 4 weeks ago

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.