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.

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

Replace CSP 'nonce-<base64>' directive with 'self' directive #43553

Merged
merged 4 commits into from
Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 2 additions & 4 deletions docs/setup/settings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,8 @@ in a manner that is inconsistent with `/proc/self/cgroup`

`csp.rules:`:: A template
https://w3c.github.io/webappsec-csp/[content-security-policy] that disables
certain unnecessary and potentially insecure capabilities in the browser. All
instances of `{nonce}` will be replaced with an automatically generated nonce
at load time. We strongly recommend that you keep the default CSP rules that
ship with Kibana.
certain unnecessary and potentially insecure capabilities in the browser. We
strongly recommend that you keep the default CSP rules that ship with Kibana.

`csp.strict:`:: *Default: `true`* Blocks access to Kibana to any browser that
does not enforce even rudimentary CSP rules. In practice, this will disable
Expand Down
3 changes: 0 additions & 3 deletions src/core/public/plugins/plugin_loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,12 @@ beforeEach(() => {
appendChildSpy = jest.spyOn(document.body, 'appendChild').mockReturnValue({} as any);

// Mock global fields needed for loading modules.
coreWindow.__kbnNonce__ = 'asdf';
coreWindow.__kbnBundles__ = {};
});

afterEach(() => {
appendChildSpy.mockRestore();
createElementSpy.mockRestore();
delete coreWindow.__kbnNonce__;
delete coreWindow.__kbnBundles__;
});

Expand All @@ -64,7 +62,6 @@ test('`loadPluginBundles` creates a script tag and loads initializer', async ()
'/bundles/plugin/plugin-a.bundle.js'
);
expect(fakeScriptTag.setAttribute).toHaveBeenCalledWith('id', 'kbn-plugin-plugin-a');
expect(fakeScriptTag.setAttribute).toHaveBeenCalledWith('nonce', 'asdf');
expect(fakeScriptTag.onload).toBeInstanceOf(Function);
expect(fakeScriptTag.onerror).toBeInstanceOf(Function);
expect(appendChildSpy).toHaveBeenCalledWith(fakeScriptTag);
Expand Down
4 changes: 0 additions & 4 deletions src/core/public/plugins/plugin_loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export type UnknownPluginInitializer = PluginInitializer<unknown, Record<string,
* @internal
*/
export interface CoreWindow {
__kbnNonce__: string;
__kbnBundles__: {
[pluginBundleName: string]: UnknownPluginInitializer | undefined;
};
Expand Down Expand Up @@ -80,9 +79,6 @@ export const loadPluginBundle: LoadPluginBundle = <
script.setAttribute('id', `kbn-plugin-${pluginName}`);
script.setAttribute('async', '');

// Add kbnNonce for CSP
script.setAttribute('nonce', coreWindow.__kbnNonce__);

const cleanupTag = () => {
clearTimeout(timeout);
// Set to null for IE memory leak issue. Webpack does the same thing.
Expand Down
39 changes: 38 additions & 1 deletion src/legacy/server/config/transform_deprecations.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,42 @@ const dataPath = (settings, log) => {
}
};

const NONCE_STRING = `{nonce}`;
// Policies that should include the 'self' source
const SELF_POLICIES = Object.freeze(['script-src', 'style-src']);
const SELF_STRING = `'self'`;

const cspRules = (settings, log) => {
const rules = _.get(settings, 'csp.rules');
if (!rules) {
return;
}

const parsed = new Map(rules.map(ruleStr => {
const parts = ruleStr.split(/\s+/);
return [parts[0], parts.slice(1)];
}));

settings.csp.rules = [...parsed].map(([policy, sourceList]) => {
if (sourceList.find(source => source.includes(NONCE_STRING))) {
log(`csp.rules no longer supports the {nonce} syntax. Replacing with 'self' in ${policy}`);
sourceList = sourceList.filter(source => !source.includes(NONCE_STRING));

// Add 'self' if not present
if (!sourceList.find(source => source.includes(SELF_STRING))) {
sourceList.push(SELF_STRING);
}
}

if (SELF_POLICIES.includes(policy) && !sourceList.find(source => source.includes(SELF_STRING))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone happened to set the following, we'll likely be breaking this install csp.rules: ["default-src 'unsafe-eval' 'nonce-{nonce}'"]. What if instead whenever we removed a nonce source, we added the 'self' source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should work. I think we should still also add it if script-src or style-src don't have self or nonce.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems reasonable to me.

log(`csp.rules must contain the 'self' source. Automatically adding to ${policy}.`);
sourceList.push(SELF_STRING);
}

return `${policy} ${sourceList.join(' ')}`.trim();
});
};

const deprecations = [
//server
unused('server.xsrf.token'),
Expand All @@ -69,7 +105,8 @@ const deprecations = [
savedObjectsIndexCheckTimeout,
rewriteBasePath,
configPath,
dataPath
dataPath,
cspRules
];

export const transformDeprecations = createTransform(deprecations);
121 changes: 112 additions & 9 deletions src/legacy/server/config/transform_deprecations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@ import { transformDeprecations } from './transform_deprecations';

describe('server/config', function () {
describe('transformDeprecations', function () {

describe('savedObjects.indexCheckTimeout', () => {
it('removes the indexCheckTimeout and savedObjects properties', () => {
const settings = {
savedObjects: {
indexCheckTimeout: 123
}
indexCheckTimeout: 123,
},
};

expect(transformDeprecations(settings)).toEqual({});
Expand All @@ -38,22 +37,22 @@ describe('server/config', function () {
const settings = {
savedObjects: {
indexCheckTimeout: 123,
foo: 'bar'
}
foo: 'bar',
},
};

expect(transformDeprecations(settings)).toEqual({
savedObjects: {
foo: 'bar'
}
foo: 'bar',
},
});
});

it('logs that the setting is no longer necessary', () => {
const settings = {
savedObjects: {
indexCheckTimeout: 123
}
indexCheckTimeout: 123,
},
};

const log = sinon.spy();
Expand All @@ -62,5 +61,109 @@ describe('server/config', function () {
sinon.assert.calledWithExactly(log, sinon.match('savedObjects.indexCheckTimeout'));
});
});

describe('csp.rules', () => {
describe('with nonce source', () => {
it('logs a warning', () => {
const settings = {
csp: {
rules: [`script-src 'self' 'nonce-{nonce}'`],
},
};

const log = jest.fn();
transformDeprecations(settings, log);
expect(log.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"csp.rules no longer supports the {nonce} syntax. Replacing with 'self' in script-src",
],
]
`);
});

it('removes a quoted nonce', () => {
expect(
transformDeprecations(
{ csp: { rules: [`script-src 'self' 'nonce-{nonce}'`] } },
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`]);
expect(
transformDeprecations(
{ csp: { rules: [`script-src 'nonce-{nonce}' 'self'`] } },
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`]);
});

it('removes a non-quoted nonce', () => {
expect(
transformDeprecations(
{ csp: { rules: [`script-src 'self' nonce-{nonce}`] } },
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`]);
expect(
transformDeprecations(
{ csp: { rules: [`script-src nonce-{nonce} 'self'`] } },
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`]);
});

it('removes a strange nonce', () => {
expect(
transformDeprecations(
{ csp: { rules: [`script-src 'self' blah-{nonce}-wow`] } },
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`]);
});

it('removes multiple nonces', () => {
expect(
transformDeprecations(
{
csp: {
rules: [
`script-src 'nonce-{nonce}' 'self' blah-{nonce}-wow`,
`style-src 'nonce-{nonce}' 'self'`,
],
},
},
jest.fn()
).csp.rules
).toEqual([`script-src 'self'`, `style-src 'self'`]);
});
});

describe('without self source', () => {
it('logs a warning', () => {
const log = jest.fn();
transformDeprecations({ csp: { rules: [`script-src 'unsafe-eval'`] } }, log);
expect(log.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"csp.rules must contain the 'self' source. Automatically adding to script-src.",
],
]
`);
});

it('adds self', () => {
expect(
transformDeprecations({ csp: { rules: [`script-src 'unsafe-eval'`] } }, jest.fn()).csp
.rules
).toEqual([`script-src 'unsafe-eval' 'self'`]);
});
});

it('does not add self to other policies', () => {
expect(
transformDeprecations({ csp: { rules: [`worker-src blob:`] } }, jest.fn()).csp.rules
).toEqual([`worker-src blob:`]);
});
});
});
});
27 changes: 1 addition & 26 deletions src/legacy/server/csp/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import {
createCSPRuleString,
generateCSPNonce,
DEFAULT_CSP_RULES,
DEFAULT_CSP_STRICT,
DEFAULT_CSP_WARN_LEGACY_BROWSERS,
Expand All @@ -40,7 +39,7 @@ import {
test('default CSP rules', () => {
expect(DEFAULT_CSP_RULES).toMatchInlineSnapshot(`
Array [
"script-src 'unsafe-eval' 'nonce-{nonce}'",
"script-src 'unsafe-eval' 'self'",
"worker-src blob:",
"child-src blob:",
"style-src 'unsafe-inline' 'self'",
Expand All @@ -56,32 +55,8 @@ test('CSP legacy browser warning defaults to enabled', () => {
expect(DEFAULT_CSP_WARN_LEGACY_BROWSERS).toBe(true);
});

test('generateCSPNonce() creates a 16 character string', async () => {
const nonce = await generateCSPNonce();

expect(nonce).toHaveLength(16);
});

test('generateCSPNonce() creates a new string on each call', async () => {
const nonce1 = await generateCSPNonce();
const nonce2 = await generateCSPNonce();

expect(nonce1).not.toEqual(nonce2);
});

test('createCSPRuleString() converts an array of rules into a CSP header string', () => {
const csp = createCSPRuleString([`string-src 'self'`, 'worker-src blob:', 'img-src data: blob:']);

expect(csp).toMatchInlineSnapshot(`"string-src 'self'; worker-src blob:; img-src data: blob:"`);
});

test('createCSPRuleString() replaces all occurrences of {nonce} if provided', () => {
const csp = createCSPRuleString(
[`string-src 'self' 'nonce-{nonce}'`, 'img-src data: blob:', `default-src 'nonce-{nonce}'`],
'foo'
);

expect(csp).toMatchInlineSnapshot(
`"string-src 'self' 'nonce-foo'; img-src data: blob:; default-src 'nonce-foo'"`
);
});
19 changes: 3 additions & 16 deletions src/legacy/server/csp/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,8 @@
* under the License.
*/

import { randomBytes } from 'crypto';
import { promisify } from 'util';

const randomBytesAsync = promisify(randomBytes);

export const DEFAULT_CSP_RULES = Object.freeze([
`script-src 'unsafe-eval' 'nonce-{nonce}'`,
`script-src 'unsafe-eval' 'self'`,
'worker-src blob:',
'child-src blob:',
`style-src 'unsafe-inline' 'self'`,
Expand All @@ -33,14 +28,6 @@ export const DEFAULT_CSP_STRICT = true;

export const DEFAULT_CSP_WARN_LEGACY_BROWSERS = true;

export async function generateCSPNonce() {
return (await randomBytesAsync(12)).toString('base64');
}

export function createCSPRuleString(rules: string[], nonce?: string) {
let ruleString = rules.join('; ');
if (nonce) {
ruleString = ruleString.replace(/\{nonce\}/g, nonce);
}
return ruleString;
export function createCSPRuleString(rules: string[]) {
return rules.join('; ');
}
6 changes: 0 additions & 6 deletions src/legacy/ui/ui_bundles/app_entry_template.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,6 @@ export const appEntryTemplate = (bundle) => `
* context: ${bundle.getContext()}
*/
// ensure the csp nonce is set in the dll
import 'dll/set_csp_nonce';
// set the csp nonce in the primary webpack bundle too
__webpack_nonce__ = window.__kbnNonce__;
// import global polyfills
import Symbol_observable from 'symbol-observable';
import '@babel/polyfill';
Expand Down
2 changes: 0 additions & 2 deletions src/legacy/ui/ui_render/bootstrap/template.js.hbs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
var kbnCsp = JSON.parse(document.querySelector('kbn-csp').getAttribute('data'));
window.__kbnStrictCsp__ = kbnCsp.strictCsp;
window.__kbnNonce__ = kbnCsp.nonce;

if (window.__kbnStrictCsp__ && window.__kbnCspNotEnforced__) {
var legacyBrowserError = document.getElementById('kbn_legacy_browser_error');
Expand Down Expand Up @@ -65,7 +64,6 @@ if (window.__kbnStrictCsp__ && window.__kbnCspNotEnforced__) {
var dom = document.createElement('script');

dom.setAttribute('async', '');
dom.setAttribute('nonce', window.__kbnNonce__);
dom.addEventListener('error', failure);
dom.setAttribute('src', file);
dom.addEventListener('load', next);
Expand Down
Loading