Skip to content

fix(sanitizer): improve reliability of sanitizer #26820

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

Merged
liamdebeasi merged 3 commits into from
Feb 17, 2023
Merged

fix(sanitizer): improve reliability of sanitizer #26820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57074aa
fix(sanitizer): account for javascript: cases
liamdebeasi Feb 17, 2023
e403c5e
fix(sanitizer): avoid attribute clobbering
liamdebeasi Feb 17, 2023
7805862
fix(sanitizer): abort early if onload is present
liamdebeasi Feb 17, 2023
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
36 changes: 34 additions & 2 deletions core/src/utils/sanitization/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ export const sanitizeDOMString = (untrustedString: IonicSafeString | string | un
return untrustedString;
}

/**
* onload is fired when appending to a document
* fragment in Chrome. If a string
* contains onload then we should not
* attempt to add this to the fragment.
*/
if (untrustedString.includes('onload=')) {
return '';
}

/**
* Create a document fragment
* separate from the main DOM,
Expand Down Expand Up @@ -89,6 +99,17 @@ const sanitizeElement = (element: any) => {
return;
}

/**
* If attributes is not a NamedNodeMap
* then we should remove the element entirely.
* This helps avoid DOM Clobbering attacks where
* attributes is overridden.
*/
if (typeof NamedNodeMap !== 'undefined' && !(element.attributes instanceof NamedNodeMap)) {
element.remove();
return;
}

for (let i = element.attributes.length - 1; i >= 0; i--) {
const attribute = element.attributes.item(i);
const attributeName = attribute.name;
Expand All @@ -103,10 +124,21 @@ const sanitizeElement = (element: any) => {
// that attempt to do any JS funny-business
const attributeValue = attribute.value;

/* eslint-disable-next-line */
if (attributeValue != null && attributeValue.toLowerCase().includes('javascript:')) {
/**
* We also need to check the property value
* as javascript: can allow special characters
* such as 	 and still be valid (i.e. java	script)
*/
const propertyValue = element[attributeName];

/* eslint-disable */
if (
(attributeValue != null && attributeValue.toLowerCase().includes('javascript:')) ||
(propertyValue != null && propertyValue.toLowerCase().includes('javascript:'))
) {
element.removeAttribute(attributeName);
}
/* eslint-enable */
}

/**
Expand Down
3 changes: 3 additions & 0 deletions core/src/utils/sanitization/test/sanitization.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ describe('sanitizeDOMString', () => {
expect(sanitizeDOMString('<a href="javascript:alert(document.cookie)">harmless link</a>')).toEqual(
'<a>harmless link</a>'
);
expect(sanitizeDOMString('<a href="javascr&Tab;ipt:alert(document.cookie)">harmless link</a>')).toEqual(
'<a>harmless link</a>'
);
});

it('filter <a> href JS + class attribute', () => {
Expand Down