Prevent possible XSS attack vector in the input fields of the group section

[PromoFaux]

By submitting this pull request, I confirm the following:

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)

---

What does this PR aim to accomplish?:

Calls the already existing utils.escapeHtml() to make sure nothing is executed. Works independent of CSP changes

[XhmikosR]

You need to include utils.js in dns_records.php before ip-address-sorting.js.

I still need to test this myself before approving.

[XhmikosR]

I only tested group-domains.php and it seems to work. Someone should try the other pages just to be sure.

[XhmikosR]

We should prefer the built in solution assuming it works the same IMHO because it's tested and works for everything, not just quotes.

…

On Mon, Jun 8, 2020, 18:11 Adam Warner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scripts/pi-hole/php/groups.php <#1443 (comment)>: > @@ -58,7 +58,10 @@ function JSON_error($message = null) } elseif ($_POST['action'] == 'add_group') { // Add new group try { - $names = str_getcsv(trim($_POST['name']), ' '); + // Payload from the web interface will have been sanitised with " converted to &quot; + // so before we process the input, we will do the opposite and replace &quot; with "; + $input = str_replace("&quot;","\"",trim($_POST['name'])); Fair shout, I didn't know of it's existence before now. Either or works in this case. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNIIUSZU22FUWR3X423RVT5TNANCNFSM4NVCBUOQ> .

[XhmikosR]

I'd like @DL6ER to confirm the database-related code. For example I see this from the PHP page:

Although this argument is technically optional, you are highly encouraged to specify the correct value for your code if you are using PHP 5.5 or earlier, or if your default_charset configuration option may be set incorrectly for the given input.

I will try the branch again tomorrow, and try to review all JS code in those files.

[PromoFaux]

1c5bbc1 prevents an (unlikely, but possible) attack vector:

Given the following entry in the static lease file:

dhcp-host=AA:BB:CC:DD:EF:FF,192.168.1.1,<script>alert("hello")</script>

Before patch:

After patch script is no longer run:

[DL6ER]

@XhmikosR I cannot test anything until beginning/mid of next week. I fully trust @PromoFaux to be aware of any database-related issues that might be there. Note that it is planed to drop any SQL stuff from the web interface for Pi-hole v6.0 and to shift any list manipulations into the API (provided by FTL). This to reduce the number of places where stuff can get changed and where write-access is needed.

[PromoFaux]

There are a couple more places I want to check before merging (see my screenshots on mattermost for more info)

[DL6ER]

Tested and found no oddities.

[PromoFaux]

Thanks to Dino at @telspacesystems and also @spartacvs for reporting the issues that lead to this PR

[iasdeoupxe]

CVE-2020-14971 seems got assigned for this issue:

https://blog.telspace.co.za/2020/06/pi-hole-code-injection-cve-2020-14971.html

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1