Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(custom-templates): XSS issue in Custom Template Note <EE-1054> #5766

Merged
merged 5 commits into from Sep 29, 2021

Conversation

cheloRydel
Copy link
Contributor

Closes EE-1054

return nil
}

func isValidNote(note string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will block the update of the already saved templates that previously contains . It also doesn't stop already injected imgs to be displayed. I recon we need to update the front-end to stop displaying these imgs instead. Using the $sanitize then removing the elements in the output should be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do both. Sanitizing the data as they are stored (including from API), is more secured. It will avoid the need for different UI/clients to sanitize them and potentially send non sanitized data to third party interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with doing both steps, I could leave this logic in the API, and also remove tags from already saved templates before rendering themm.

Regarding "This function will block the update of the already saved templates that previously contains ", wouldn't that be OK, given the scenario where a user would whant to update an "unsanitized" template?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scenario of the external party using the data, sanitizing at the backend is safer. Although I feel that we need to do more than just regex checking the img tag including:

  • removing dangerous tags using a library that is designed for the job (see OWASP suggestion)
  • sanitizing other dangerous tags such as <script> if the input is supposed to be displayed as HTML
  • encoding the received input if they are only supposed to be text
  • sanitizing the other API endpoints (e.g. create/update team name)
  • potential data migration

I suggest the whole sanitization in the backend can be done separately because it's a different scenario and a bigger job of its own. Keeping the <img> checking is OK, but the UX and implementation are likely to be changed in the above steps.

Regarding "This function will block the update of the already saved templates that previously contains ", wouldn't that be OK, given the scenario where a user would whant to update an "unsanitized" template?

For a full backend sanitization, I was thinking about a non-blocking UX (we still sanitize the data) because we might not be able to tell the user all the exact issues when we sanitizing from the backend depending on the sanitization library we choose. However, if you choose to keep the img tag backend checking as is then this blocking is still required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The displaying part is ok, removing the img tag validation is optional so I will pass the technical review.

@cheloRydel cheloRydel merged commit fce8859 into develop Sep 29, 2021
9 checks passed
@cheloRydel cheloRydel deleted the fix/EE-810/EE-1054 branch September 29, 2021 19:47
@huib-portainer
Copy link
Contributor

Closes #5805

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants