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 unescaped strings XSS issues #13554

Merged
merged 10 commits into from Nov 24, 2021
Merged

Conversation

enferas
Copy link
Contributor

@enferas enferas commented Nov 22, 2021

Fix XSS vulnerabilities in /include/html directory.

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2021

CLA assistant check
All committers have signed the CLA.

@@ -18,5 +18,5 @@
$vars['fromdevice'] = false;
require_once 'includes/html/modal/alert_details.php';
require_once 'includes/html/common/alert-log.inc.php';
echo implode('', $common_output);
echo htmlspecialchars(implode('', $common_output));
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure $common_output is html. You will need to make any fixes in includes/html/common/alert-log.inc.php instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your response.

I can see that $common_output has the variable $selected_min_severity in here.
Which has a source here
What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Fix it at the source ;)

Copy link
Member

Choose a reason for hiding this comment

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

Funny below it is cast to int (preventing xss)

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget the other $common_output ones.

Copy link
Member

Choose a reason for hiding this comment

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

Funny below it is cast to int (preventing xss)

This seems like the (more) correct fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand well the cast for int is for the function array_search. but the variable $selected_min_severity will keep the string value. Because there is a concat string .= in the second assign.

$selected_min_severity = '<option value="' . $_POST['min_severity'] . '" selected="selected">';
$selected_min_severity .= array_search((int) $_POST['min_severity'], $alert_severities) . '</option>';

@murrant
Copy link
Member

murrant commented Nov 23, 2021

Thanks for the fixes! Some however escape strings that contain html or json. You'll need a different strategy for those.

@enferas
Copy link
Contributor Author

enferas commented Nov 23, 2021

Thanks for the fixes! Some however escape strings that contain html or json. You'll need a different strategy for those.

The changes are done to sanitize at the level of the sources instead of the sinks.

Copy link
Member

@murrant murrant left a comment

Choose a reason for hiding this comment

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

Looks good now @enferas

Thanks :)

@murrant murrant merged commit 35a6905 into librenms:master Nov 24, 2021
10 checks passed
@murrant murrant changed the title Fix security vuls Fix unescaped strings XSS issues Nov 24, 2021
@Jellyfrog
Copy link
Member

Thanks for not only reporting these, but also stepping up to fixing them!

@enferas
Copy link
Contributor Author

enferas commented Nov 24, 2021

Thanks for not only reporting these, but also stepping up to fixing them!

It is my pleasure. Thank you for your fast responses and merging my pull request.

@enferas
Copy link
Contributor Author

enferas commented Dec 2, 2021

3 CVEs are assigned.

CVE-2021-44277
Librenms 21.11.0 is affected by is affected by a Cross Site Scripting (XSS) vulnerability in includes/html/common/alert-log.inc.php.

CVE-2021-44278
Librenms 21.11.0 is affected by a path manipulation vulnerability in includes/html/pages/device/showconfig.inc.php.

CVE-2021-44279
Librenms 21.11.0 is affected by is affected by a Cross Site Scripting (XSS) vulnerability in includes/html/forms/poller-groups.inc.php.

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/21-12-0-changelog/17740/1

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/21-12-0-release/17742/1

NightowlKr pushed a commit to NightowlKr/librenms that referenced this pull request Dec 30, 2021
* Fix XSS vulnerabilities

* fix XSS vulnerabilities in alerts.inc.php

* fix XSS vulnerability in poller-groups.inc.php

* small fix for the integration

* another fix for the inegration

* another fix for the inegration

* change the sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of common_output and current_config sinks

* fix path manipulation vulnerability
NightowlKr pushed a commit to NightowlKr/librenms that referenced this pull request Dec 31, 2021
* Fix XSS vulnerabilities

* fix XSS vulnerabilities in alerts.inc.php

* fix XSS vulnerability in poller-groups.inc.php

* small fix for the integration

* another fix for the inegration

* another fix for the inegration

* change the sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of common_output and current_config sinks

* fix path manipulation vulnerability
NightowlKr pushed a commit to NightowlKr/librenms that referenced this pull request Dec 31, 2021
* Fix XSS vulnerabilities

* fix XSS vulnerabilities in alerts.inc.php

* fix XSS vulnerability in poller-groups.inc.php

* small fix for the integration

* another fix for the inegration

* another fix for the inegration

* change the sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of json_encode sinks

* another change sanitizer at sources instead of common_output and current_config sinks

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

Successfully merging this pull request may close these issues.

None yet

5 participants