Skip to content

Commit

Permalink
Fixed two XSS vulnerabilities in SSR. Skip invalid tag names and vali…
Browse files Browse the repository at this point in the history 
…date attribute names.
  • Loading branch information
@Havunen
Havunen committed Aug 6, 2018
1 parent ac2f02b commit e88c23c
Show file tree
Hide file tree
Showing 6 changed files with 162 additions and 32 deletions.
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ Just run `npm run test:browser:debug` Open localhost:9876 and click debug!
Debugging NodeJS
----------------
Its possible to debug inferno tests by running following command `npm run debug` and open chrome web address: chrome://inspect/#devices
However: The issue is that ts-jest does post processing to compiled files this needs to be avoided.
Edit following files: /node_modules/ts-jest/dist/postprocess.js and remove following settings line numer 15 => retainLines: true, sourceMaps: 'inline'.
However: There is issue with ts-jest; It does additional post processing to compiled files this needs to be disabled to see source files.
Edit following file: `/node_modules/typescript-babel-jest/node_modules/babel-jest/build/index.js` and change hardcoded babel option `retainLines` property to false.

Pro tip: You can filter down number of tests by editing `debug` -task:
`node --inspect-brk ./node_modules/.bin/jest {*edit this*} --runInBand --no-cache --no-watchman`
Expand Down
78 changes: 78 additions & 0 deletions packages/inferno-server/__tests__/security.spec.server.jsx
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { createElement } from 'inferno-create-element';
import { renderToString, streamAsString, streamQueueAsString } from 'inferno-server';
import concatStream from 'concat-stream-es6';

describe('Security - SSR', () => {
describe('renderToString', () => {
it('Should not render invalid tagNames', () => {
expect(() => renderToString(createElement('div'))).not.toThrow();
expect(() => renderToString(createElement('x-💩'))).not.toThrow();
expect(() => renderToString(createElement('a b'))).toThrow(/<a b>/);
expect(() => renderToString(createElement('a\0b'))).toThrow(/<a\0b>/);
expect(() => renderToString(createElement('a>'))).toThrow(/<a>>/);
expect(() => renderToString(createElement('<'))).toThrow(/<<>/);
expect(() => renderToString(createElement('"'))).toThrow(/<">/);
});

it('Should not render invalid attribute names', () => {
const props = {};
const userProvidedData = '></div><script>alert("hi")</script>';

props[userProvidedData] = 'hello';

let html = renderToString(<div {...props} />);

expect(html).toBe('<div></div>');
});

it('should reject attribute key injection attack on markup', () => {
const element1 = createElement('div', { 'blah" onclick="beevil" noise="hi': 'selected' }, null);
const element2 = createElement('div', { '></div><script>alert("hi")</script>': 'selected' }, null);
let result1 = renderToString(element1);
let result2 = renderToString(element2);
expect(result1.toLowerCase()).not.toContain('onclick');
expect(result2.toLowerCase()).not.toContain('script');
});
});

describe('streams', () => {
[streamAsString, streamQueueAsString].forEach(method => {
it('Should not render invalid attribute names', () => {
const props = {};
const userProvidedData = '></div><script>alert("hi")</script>';

props[userProvidedData] = 'hello';

streamPromise(<div {...props} />, method).then(html => expect(html).toBe('<div></div>'));
});

it('should reject attribute key injection attack on markup', done => {
const element1 = createElement('div', { 'blah" onclick="beevil" noise="hi': 'selected' }, null);
streamPromise(element1, method).then(result1 => {
expect(result1.toLowerCase()).not.toContain('onclick');
done();
});
});

it('should reject attribute key injection attack on markup #2', done => {
const element2 = createElement('div', { '></div><script>alert("hi")</script>': 'selected' }, null);
streamPromise(element2, method).then(result2 => {
expect(result2.toLowerCase()).not.toContain('script');
done();
});
});
});
});
});

function streamPromise(dom, method) {
return new Promise(function(res, rej) {
method(dom)
.on('error', rej)
.pipe(
concatStream(function(buffer) {
res(buffer.toString('utf-8'));
})
);
});
}
25 changes: 16 additions & 9 deletions packages/inferno-server/src/renderToString.queuestream.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { combineFrom, isFunction, isInvalid, isNull, isNullOrUndef, isNumber, is
import { ChildFlags, VNodeFlags } from 'inferno-vnode-flags';
import { Readable } from 'stream';
import { renderStylesToString } from './prop-renderers';
import { escapeText, voidElements } from './utils';
import { escapeText, isAttributeNameSafe, voidElements } from './utils';

export class RenderQueueStream extends Readable {
public collector: any[] = [Infinity]; // Infinity marks the end of the stream
Expand Down Expand Up @@ -212,23 +212,30 @@ export class RenderQueueStream extends Readable {
}
break;
default:
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
if (isAttributeNameSafe(prop)) {
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
}
}
break;
}
}
}
renderedString += `>`;

if (String(type).match(/[\s\n\/='"\0<>]/)) {
throw renderedString;
}

// Voided element, push directly to queue
if (isVoidElement) {
this.addToQueue(renderedString + `>`, position);
this.addToQueue(renderedString, position);
// Regular element with content
} else {
renderedString += `>`;
// Element has children, build them in
const childFlags = vNode.childFlags;

Expand Down
30 changes: 18 additions & 12 deletions packages/inferno-server/src/renderToString.stream.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { combineFrom, isFunction, isInvalid, isNull, isNullOrUndef, isNumber, is
import { ChildFlags, VNodeFlags } from 'inferno-vnode-flags';
import { Readable } from 'stream';
import { renderStylesToString } from './prop-renderers';
import { escapeText, voidElements } from './utils';
import { escapeText, isAttributeNameSafe, voidElements } from './utils';
import { VNode } from 'inferno';

const resolvedPromise = Promise.resolve();
Expand Down Expand Up @@ -181,24 +181,30 @@ export class RenderStream extends Readable {
}
break;
default:
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
if (isAttributeNameSafe(prop)) {
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
}
break;
}
break;
}
}
}

renderedString += `>`;
this.push(renderedString);

if (String(type).match(/[\s\n\/='"\0<>]/)) {
throw renderedString;
}

if (isVoidElement) {
renderedString += `>`;
this.push(renderedString);
return;
} else {
renderedString += `>`;
this.push(renderedString);
if (html) {
this.push(html);
this.push(`</${type}>`);
Expand Down
28 changes: 19 additions & 9 deletions packages/inferno-server/src/renderToString.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { EMPTY_OBJ } from 'inferno';
import { combineFrom, isFunction, isInvalid, isNull, isNullOrUndef, isNumber, isString, isTrue, throwError } from 'inferno-shared';
import { ChildFlags, VNodeFlags } from 'inferno-vnode-flags';
import { renderStylesToString } from './prop-renderers';
import { escapeText, voidElements } from './utils';
import { escapeText, isAttributeNameSafe, voidElements } from './utils';

function renderVNodeToString(vNode, parent, context, firstChild): string | undefined {
function renderVNodeToString(vNode, parent, context, firstChild): string {
const flags = vNode.flags;
const type = vNode.type;
const props = vNode.props || EMPTY_OBJ;
Expand Down Expand Up @@ -79,6 +79,7 @@ function renderVNodeToString(vNode, parent, context, firstChild): string | undef
} else if ((flags & VNodeFlags.Element) > 0) {
let renderedString = `<${type}`;
let html;

const isVoidElement = voidElements.has(type);
const className = vNode.className;

Expand Down Expand Up @@ -118,13 +119,16 @@ function renderVNodeToString(vNode, parent, context, firstChild): string | undef
}
break;
default:
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
if (isAttributeNameSafe(prop)) {
if (isString(value)) {
renderedString += ` ${prop}="${escapeText(value)}"`;
} else if (isNumber(value)) {
renderedString += ` ${prop}="${value}"`;
} else if (isTrue(value)) {
renderedString += ` ${prop}`;
}
}

break;
}
}
Expand Down Expand Up @@ -152,6 +156,11 @@ function renderVNodeToString(vNode, parent, context, firstChild): string | undef
renderedString += `</${type}>`;
}
}

if (String(type).match(/[\s\n\/='"\0<>]/)) {
throw renderedString;
}

return renderedString;
} else if ((flags & VNodeFlags.Text) > 0) {
return (firstChild ? '' : '<!---->') + (children === '' ? ' ' : escapeText(children));
Expand All @@ -165,7 +174,8 @@ function renderVNodeToString(vNode, parent, context, firstChild): string | undef
}
throwError();
}
return undefined;

return '';
}

export function renderToString(input: any): string {
Expand Down
29 changes: 29 additions & 0 deletions packages/inferno-server/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,35 @@ export function getCssPropertyName(str): string {
return (CssPropCache[str] = str.replace(uppercasePattern, '-$&').toLowerCase() + ':');
}

const ATTRIBUTE_NAME_START_CHAR =
':A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6\\u00F8-\\u02FF\\u0370-\\u037D\\u037F-\\u1FFF\\u200C-\\u200D\\u2070-\\u218F\\u2C00-\\u2FEF\\u3001-\\uD7FF\\uF900-\\uFDCF\\uFDF0-\\uFFFD';

const ATTRIBUTE_NAME_CHAR = ATTRIBUTE_NAME_START_CHAR + '\\-.0-9\\u00B7\\u0300-\\u036F\\u203F-\\u2040';

export const VALID_ATTRIBUTE_NAME_REGEX = new RegExp('^[' + ATTRIBUTE_NAME_START_CHAR + '][' + ATTRIBUTE_NAME_CHAR + ']*$');

const illegalAttributeNameCache = {};
const validatedAttributeNameCache = {};

export function isAttributeNameSafe(attributeName: string): boolean {
if (validatedAttributeNameCache[attributeName] !== void 0) {
return true;
}
if (illegalAttributeNameCache[attributeName] !== void 0) {
return false;
}
if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
validatedAttributeNameCache[attributeName] = true;
return true;
}
illegalAttributeNameCache[attributeName] = true;
if (process.env.NODE_ENV !== 'production') {
// tslint:disable-next-line:no-console
console.log('Invalid attribute name: ' + attributeName);
}
return false;
}

export const voidElements = new Set([
'area',
'base',
Expand Down

0 comments on commit e88c23c

Please sign in to comment.