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

2 reflected XSS #844

Closed
floyd-fuh opened this issue Jun 24, 2024 · 4 comments · Fixed by #846
Closed

2 reflected XSS #844

floyd-fuh opened this issue Jun 24, 2024 · 4 comments · Fixed by #846
Labels
QA Issue or Pull request that is in review security Pull requests that address a security vulnerability
Milestone

Comments

@floyd-fuh
Copy link

You have two trivial reflected Cross-Site Scripting (XSS) issues that might have an impact or not, as I didn't test an attack vector/exploitability, e.g. I don't know if there is a trivial link-click exploit path (I simply didn't try because I don't even have this plugin installed anywhere). Nevertheless, I suggest you fix them:

echo '<input type="hidden" name="record_id" value="' . ( $_GET['id'] ?? 0 ) . '" />';

onoffice-for-wp-websites/plugin/Gui/AdminPageFormSettingsBase.php:762:          echo '<input type="hidden" name="record_id" value="' . ( $_GET['id'] ?? 0 ) . '" />';

and

echo '<input type="hidden" name="record_id" value="' . ( $_GET['id'] ?? 0 ) . '" />';

onoffice-for-wp-websites/plugin/Gui/AdminPageSettingsBase.php:170:              echo '<input type="hidden" name="record_id" value="' . ( $_GET['id'] ?? 0 ) . '" />';

Obviously if the URL GET parameter id is something like ?id="><script>alert(1)</script> this will trigger the alert.

Again, I didn't test it, there might be a hundred code paths that verify the id parameter to be numeric before this code is trigger - or not, I didn't check.

As far as I saw you know what HTML output encoding is and means, so this should be trivial to fix.

@fredericalpers fredericalpers added this to the v4.25 milestone Jun 25, 2024
@fredericalpers fredericalpers added the security Pull requests that address a security vulnerability label Jun 25, 2024
@yeneastgate
Copy link
Contributor

@fredericalpers I will check and fix it asap

@yeneastgate
Copy link
Contributor

@fredericalpers @floyd-fuh I have listed and checked the files that may have XSS and SQL Injection vulnerabilities in the table below, and I have updated the locations where XSS security errors may occur.
image

Please check and let me know your opinion. Thanks!

@fredericalpers
Copy link
Member

Thank you for fixing this, we will review this asap.

@fredericalpers fredericalpers added the QA Issue or Pull request that is in review label Jun 28, 2024
@fredericalpers fredericalpers modified the milestones: v4.25, v4.21 Jun 28, 2024
@fredericalpers fredericalpers linked a pull request Jun 28, 2024 that will close this issue
@floyd-fuh
Copy link
Author

looks ok to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Issue or Pull request that is in review security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants