Lack of escaping on some pages can lead to XSS exposure (CVE-2020-7106)

[0xfatty]

Describe the bug
Data source input validation error leads to Stored XSS within Description after creating a device with Malicious code embedded in Description field.

To Reproduce
Steps to reproduce the behavior:

1. Navigate to Console -> Create -> New Device
2. In Description field, input script payload: <svg/onload=alert(1)>
3. Fill out the rest of the form as normal
4. Click Save. After the first click, there will be an error dialog saying Whitelisting is ON (yes, I turned it on this time). However, I was still able to save the "New Device" form by clicking "Save" once again.
5. After the message of "Successfully operated", click on "Data Sources" on the top right menu. XSS dialog will pop up.

You might ask me questions about CSP and the whitelisting thing in config.php:
If I turned the whitelisting feature ON? YES
If I fixed html.php to append CSP policy? YES

Screenshots

• Input fields: https://imgur.com/GcAyUkC
• Pop XSS dialog: https://imgur.com/VkMIgs2
• CSP Enabled: https://imgur.com/4A0v3jq
• XSS string was not filtered in data_sources.php: https://imgur.com/xRFHXAP

[0xfatty]

Remediation

• Tracing back to the code, I saw that the Description gets stored into DB. Then in data_sources.php line 1331-1332, $description takes that string (XSS) from database and then gets displayed in raw by the $header which then leads to Cross-site scripting.
• Hence, to fix this, I would suggest to run $description through htmlspecialchars() to avoid XSS payloads.

Original:

$description = db_fetch_cell_prepared('SELECT description FROM host WHERE id = ?', array(get_request_var('host_id')));
$header = __('Data Sources [ %s ]', $description);

Suggestion:

$description = db_fetch_cell_prepared('SELECT description FROM host WHERE id = ?', array(get_request_var('host_id')));
$description = htmlspecialchars($description, ENT_QUOTES);
$header = __('Data Sources [ %s ]', $description);

[bmfmancini]

Wow great catch sir!!

[netniV]

Thanks for your ongoing efforts to improve the code Chi, appreciated. If you already have a patch, you should submit it as a pull request so we can merge it. Since this is an XSS bug, if you could do that against the 1.2.x branch and include the CHANGELOG entry too, that would be perfect.

[0xfatty]

Hi @netniV

Thank you for your response. I am happy to do so.

[cigamit]

Yea, I found a few more. Comitting shortly.

[cigamit]

total files are:

• data_sources.php
• color_templates_item.php
• graphs.php
• graph_items.php
• lib/api_automation.php
• user_admin.php
• user_group_admin.php

Bummer.

[cigamit]

Okay, just committed the fixes. That's from a pretty comprehensive audit.

[cigamit]

@smutranchi, if you can get a CVE number for this, it would be appreciated.

[0xfatty]

Hi @cigamit ,

Thank you for your response. I have raised a CVE request. I will put it in here once I got the number.

Sincerely,
Chi Tran

[0xfatty]

Hi @netniV @cigamit,

A CVE was assigned to this bug as:
CVE-2020-7106.

Please let me know if you need any further information.

Best regards,
Chi Tran

[netniV]

If you can review the code change, that would add some validation to the fix. We will use the CVE in our notifications on the next release.

[cigamit]

A double review of the commit would be appreciated.

[0xfatty]

I have just pulled new commits from 1.2.x branch and started reviewing. I will let you guys know if I found anything else.

[hlef]

@cigamit is can see that several lines in this diff are still missing the __esc:

e.g.

4cbb045#diff-317c61020ed4b560afa7e1760f261534R1155

or

4cbb045#diff-3398f8d2633c0b07fbd66bd7b8f75ecdR2018

Is it intentional ?

[cigamit]

Yes, if you follow the code, those two strings are included in a subsequent __esc() further in.

[0xfatty]

Hi @cigamit ,

I noticed that when a report is created. In file lib/html_reports.php, function reports_generate_html() gets called with param $reports passed in.

Tracing back to lib/reports.php where reports_generate_html() function is declared, I observed that the report tables prints $reports[‘name’] in raw. (Line 707 reports.php)

Hence, if we send an XSS payload into field “Report Name”, it will still be executed when users navigate to tab Preview.

[cigamit]

Good catch.

[cigamit]

Okay, got that one caught now.

[0xfatty]

Hi @cigamit ,

I tried to trace from the Device description and got one more file that is affected.
After changing Device description to XSS payload, all graph names (title) will become |host description| - graph feature

Then if admin turns on WEBLOG, cli/clog_webapi.php would be still fetching $ds_title in raw which contains XSS payload.

And pop XSS diaglog when admin navigate to Logs tab from Menu bar

[0xfatty]

I have just also made a change to 1.2.x branch on clog_webapi.php file.

[netniV]

Thanks again Chi, I'll take a look and see about submitting it.