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.

Sign up for GitHub

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

Jump to bottom

[PPP-5370][Update XSS Vulnerabilities] #5758

Open
wants to merge 4 commits into
base: XSS-PPP-5370
Choose a base branch
from
Open

[PPP-5370][Update XSS Vulnerabilities] #5758

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c14f53f
[PPP-5370][Update XSS Vulnerabilities]
mmujahiduddin Oct 30, 2024
0e882a0
Update End Of Line
mmujahiduddin Oct 30, 2024
b3b5988
Merge branch 'pentaho:PPP-5370' into PPP-5370
mmujahiduddin Nov 11, 2024
9c795c4
Update Review Comments#1
mmujahiduddin Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions assemblies/pentaho-war/src/main/webapp/js/ajaxslt/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@
// Spec. However, different browsers actually pass very different
// values at the API.
//
function xmlResolveEntities(s) {

define(['common-ui/util/xss'],
function(xssUtil) {

function xmlResolveEntities(s) {

var parts = stringSplit(s, '&');

Expand Down Expand Up @@ -73,7 +77,7 @@ function xmlResolveEntities(s) {
// through the W3C DOM. W3C DOM access is specified to resolve
// entities.
var span = window.document.createElement('span');
span.innerHTML = '&' + rp[0] + '; ';
xssUtil.setHtml(span, '&' + rp[0] + '; ');
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file, which is not prepared to be a RequireJS module, you should use the global version of the method: pho.util.xss.setHtml(., .) (and undo the proposed RequireJS modifications).

ch = span.childNodes[0].nodeValue.charAt(0);
}
ret += ch + rp[1];
Expand Down Expand Up @@ -437,4 +441,4 @@ XNode.prototype.getElementsByTagName = function(name, list) {
}

return list;
}
} });
9 changes: 6 additions & 3 deletions assemblies/pentaho-war/src/main/webapp/js/google-demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
* Change Date: 2028-08-13
******************************************************************************/

define(['common-ui/util/xss'],
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT, this file is not referenced anywhere.

It appears to demonstrate use of some Google Maps component (GeoCoder) via Pentaho XActions. XActions are essentially deprecated.

Unless you can find a reference to it, solving the XSS issue by removing the file.

function(xssUtil) {

var map;
var redicon;
var yellowicon;
Expand Down Expand Up @@ -86,7 +89,7 @@ greenicon = icon;

function updateProductMix( content ) {
document.getElementById( 'details-div' ).style.display='block';
document.getElementById( 'details-cell1' ).innerHTML=content;
xssUtil.setHtml(document.getElementById('details-cell1'), content);
pentahoAction( "steel-wheels", "google", "customer_details.xaction",
new Array( new Array( "customer", currentRecord[7] ) ),
'updateHistory'
Expand All @@ -95,7 +98,7 @@ greenicon = icon;

function updateHistory( content ) {
document.getElementById( 'details-div' ).style.display='block';
document.getElementById( 'details-cell2' ).innerHTML=content;
xssUtil.setHtml(document.getElementById('details-cell2'), content);
}

function showAddress(address, name, custNum, value, selected) {
Expand Down Expand Up @@ -193,4 +196,4 @@ function showAddress(address, name, custNum, value, selected) {
}


}
} });
9 changes: 6 additions & 3 deletions assemblies/pentaho-war/src/main/webapp/js/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
* Change Date: 2028-08-13
******************************************************************************/

define(['common-ui/util/xss'],
function(xssUtil) {

function runInBackground( url, target )
{
var response = confirm( "Info: Reports that prompt for parameters are not supported with this feature."
Expand All @@ -20,11 +23,11 @@ function runInBackground( url, target )
url = url + "&background=true";
if ( target.toLowerCase().indexOf( 'new' ) >= 0 )
{
var targetWin = window.open( url );
var targetWin = window.open(xssUtil.sanitizeHtml(url), 'noopener,noreferrer');
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not prepared to be a RequireJS module, and, as such, you should use the global version of the XSS utility library, available under pho.util.xss (and undo the proposed RequireJS modifications).

However, the library does not currently offer any mechanism to validate URLs. AFAIR (but please confirm!), URL protection against XSS consists of ensuring that the URL does not have a javascript protocol. A function such as sanitizeUrl(url) should be added to the XSS utility and then used here.

I am not sure what's the best source to base our implementation of, in this regard. Some pointers:

Copy link
Contributor

@dcleao dcleao Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding this, I think we might be able to use DOMPurify to this effect.

In the DOMPlurify playground page, https://cure53.de/purify, try out the following sample Dirty HTML:

<a href='
  javas cript:alert(1)'>I am a dolphin!</a>
<a href='http://www.google.com'>I am a dolphin!</a>

It cleans it as:

<a>I am a dolphin!</a>
<a href="http://www.google.com">I am a dolphin!</a>

As such, we should be able to implement sanitizeUrl(url) by creating a dummy a element string with the URL as the href attribute: '<a href="' + url + '">'. Then reading back the resulting href value, if any.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, we can just base our implementation on their own, from:

Which seems like somewhat concise regexp code that we can mimic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another aspect which must be considered is whether the suggested sanitizeUrl() method alone would automatically resolve the AppScan finding. I believe the finding is being triggered due to any use of window.open() with a URL string. And so we should need to also create a method such as xssUtil.openWindow(window, url, options) that achieves this in a safe way (by internally sanitizing the URL and hiding the call to window.open).

}
else
{
window.location = url;
window.location = xssUtil.sanitizeHtml(url);
}
}
return undefined; // forces current page to remain unchanged when target=new
Expand Down Expand Up @@ -113,4 +116,4 @@ function showShareDialog( event, solution, path, filename )
var position = UIUtil.getScrollCoords( { left: event.clientX, top: event.clientY } );
shareDialog.setPosition( { left: position.left+ "px", top: position.top + "px" } );
shareDialog.show();
}
} });
6 changes: 5 additions & 1 deletion assemblies/pentaho-war/src/main/webapp/js/parameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@

// used in getParameters and doClearIgnoreFields to ignore specific hidden fields

define(['common-ui/util/xss'],
function(xssUtil) {

var pentaho_ignoreFields = new Array();
var pentaho_ignoreIndexOfFields = new Array();
var pentaho_optionalParams = new Array();
Expand Down Expand Up @@ -360,7 +363,7 @@ function executeAction (target, submitUrl) {
// convert characters from entities like &#305; to display characters (HTML)
function convertHtmlEntitiesToCharacters(theStr) {
var newDiv = document.createElement(newDiv);
newDiv.innerHTML = theStr;
xssUtil.setHtml(newDiv, theStr);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file, which is not prepared to be a RequireJS module, you should use the global version of the method: pho.util.xss.setHtml(., .) (and undo the proposed RequireJS modifications).

return newDiv.innerHTML;
}

Expand Down Expand Up @@ -488,3 +491,4 @@ function closeMantleTab(){
alert("error closing tab: "+e);
}
}
});
7 changes: 5 additions & 2 deletions assemblies/pentaho-war/src/main/webapp/js/src/html/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ modified BSD license. For more information on Dojo licensing, see:
http://dojotoolkit.org/community/licensing.shtml
*/

define(['common-ui/util/xss'],
function(xssUtil) {

dojo.provide("dojo.html.util");
dojo.require("dojo.html.layout");

Expand Down Expand Up @@ -210,7 +213,7 @@ dojo.html.createNodesFromText = function(/* string */txt, /* boolean? */trim){
txt = "<table>" + txt + "</table>";
tableType = "section";
}
tn.innerHTML = txt;
xssUtil.setHtml(tn, txt);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file, which is not prepared to be a RequireJS module, you should use the global version of the method: pho.util.xss.setHtml(., .) (and undo the proposed RequireJS modifications).

if(tn["normalize"]){
tn.normalize();
}
Expand Down Expand Up @@ -481,4 +484,4 @@ dojo.html.scrollIntoView = function(/* HTMLElement */node){
parent.scrollTop -= (parent.scrollTop - node.offsetTop);
}
}
}
} });
11 changes: 8 additions & 3 deletions assemblies/pentaho-war/src/main/webapp/js/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ over a cell (to make it easier to determine what cell you're over). Added commen
in the style sheet, to make it more clear what the different style elements are for.
*/

define(['common-ui/util/xss'],
function(xssUtil) {

var datePickerDivID = "datepicker";
var iFrameDivID = "datepickeriframe";

Expand Down Expand Up @@ -294,8 +297,8 @@ function refreshDatePicker(dateFieldName, year, month, day)

// and finally, close the table
html += xTABLE;
document.getElementById(datePickerDivID).innerHTML = html;

xssUtil.setHtml(document.getElementById(datePickerDivID), html);
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not prepared to be a RequireJS module, and, as such, you should use the global version of the XSS utility library, available under pho.util.xss (and undo the proposed RequireJS modifications).

As you can see in the code above, there are multiple instances of event handler attributes (e.g. onMouseOut, onClick), which using setHtml would eliminate altogether, breaking the functionality.
Instead, must sanitize/encode the parts used to build the HTML string which may pose a XSS risk.

The arguments year, month and day are already properly handled in a safe manner.

The argument dateFieldName however is being used directly as a JavaScript string (with quotes, double or single, provided outside). It should be escaped before use. We need a a function such as encodeForJavaScript(text) (following, for example, OWASP's functionality of OWASP Encoder) and add it to the XSS utility and then use here.

Then, in this same line, mark the setting of unsafe HTML using xssUtil.setHtmlUnsafe(document.getElementById(datePickerDivID), html); instead.

Note that method getButtonCode also needs to be changed.

// add an "iFrame shim" to allow the datepicker to display above selection lists
adjustiFrame();
}
Expand Down Expand Up @@ -530,4 +533,6 @@ function isValidName(name){
function reservedCharListForDisplay( separatorString ) {
//ToDo: Fix this
return "/ \ :";
}
}

});
8 changes: 7 additions & 1 deletion user-console/src/main/resources/org/pentaho/mantle/public/home/content/welcome/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@

/* CCP Functions */

define(['common-ui/util/xss'],
function(xssUtil) {

var CCP = CCP || {};

/* Utility function to get a method from a different window / frame */
Expand Down Expand Up @@ -50,7 +53,9 @@ CCP.liveChat = function(){
pucOpenTab( name, title, url );
}
else {
window.open( url );
/* noopener and noreferrer: These attributes mitigate the risk of tabnabbing and
prevent the new page from accessing the original window’s properties. */
window.open(xssUtil.sanitizeHtml(url),'noopener,noreferrer');
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not prepared to be a RequireJS module, and, as such, you should use the global version of the XSS utility library, available under pho.util.xss (and undo the proposed RequireJS modifications).

Please, use the new method suggested above, pho.util.xss.sanitizeUrl(..).

}
}

Expand All @@ -68,3 +73,4 @@ CCP.getForumFeed = function(divId){
error: function() { throw new Error("jQuery RSS: url don't link to RSS-Feed") }
});
}
});
9 changes: 6 additions & 3 deletions user-console/src/main/resources/org/pentaho/mantle/public/home/js/gettingStarted.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
define([
"common-ui/util/ContextProvider",
"common-ui/util/BootstrappedTabLoader",
"common-ui/util/HandlebarsCompiler"
], function (ContextProvider, BootstrappedTabLoader, HandlebarsCompiler) {
"common-ui/util/HandlebarsCompiler",
"common-ui/util/xss"
], function (ContextProvider, BootstrappedTabLoader, HandlebarsCompiler, xssUtil) {

var brightCoveVideoTemplate =
'<iframe src="https://players.brightcove.net/4680021553001/default_default/index.html?videoId={{videoId}}&autoplay=true"' +
Expand Down Expand Up @@ -206,7 +207,9 @@ define([

launchLink.unbind("click");
launchLink.bind("click", function () {
window.open(href, "_blank");
/* noopener and noreferrer: These attributes mitigate the risk of tabnabbing and
prevent the new page from accessing the original window’s properties. */
window.open(xssUtil.sanitizeHtml(href), "_blank", 'noopener,noreferrer');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use the new method suggested above, xssUtil.sanitizeUrl(..).

});
}

Expand Down