Skip to content

XSS sanitize pwrstatd application script inputs#14545

Merged
murrant merged 2 commits intolibrenms:masterfrom
bnerickson:pwrstat
Nov 2, 2022
Merged

XSS sanitize pwrstatd application script inputs#14545
murrant merged 2 commits intolibrenms:masterfrom
bnerickson:pwrstat

Conversation

@bnerickson
Copy link
Copy Markdown
Contributor

Sanitizing and validating input for the pwrstatd application's script's data. We expect integers for all data except the serial number which is a string.

A bad actor could, in theory, construct a pwrstatd application that returned a serial number with "<script>..." in its name to arbitrarily execute on the pwrstatd application page. filter_var has been added to negate this possibility.

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.

continue;
}

$mruntime = is_int($data['mruntime']) ? $data['mruntime'] : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think any of these is_int() calls are necessary (and it might not even work). All these are stored in rrd (numeric only) or database fields that are numeric.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

$data is the json data pulled from the script output before its ingested into an rrd file, and, after monitoring overnight, these changes do work. I agree, however, that they aren't necessary. My rationale is to filter the data received at the edge rather than let rrd ingestion figure it out. For instance, I'm not sure what rrd does if it gets passed a string number such as "123", but it should be an invalid input IMO; so I set it to null.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"123" and 123 are the same as far as rrd is concerned.

@bnerickson bnerickson marked this pull request as ready for review October 31, 2022 18:59
@murrant murrant merged commit a63b12a into librenms:master Nov 2, 2022
@bnerickson bnerickson deleted the pwrstat branch February 20, 2024 23:46
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.

2 participants