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

ESLint Rule to discourage hashes being created with unsafe algorithms #190973

Merged
merged 34 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
0b1705a
Adding initial functions
kc13greiner Aug 21, 2024
68b92f1
change rule to be an allow list of safe hash functions
SiddharthMantri Aug 29, 2024
264709e
modify rule
SiddharthMantri Aug 30, 2024
820d47c
use sourceCode.getScope instead of scopeManager
SiddharthMantri Aug 30, 2024
ba435ba
Merge branch 'main' into feature/FIPS_ESLint_Rule
SiddharthMantri Aug 30, 2024
03c0b5c
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Aug 30, 2024
fd26972
update allow list
SiddharthMantri Aug 30, 2024
6337115
fix expected error messages
SiddharthMantri Aug 30, 2024
6b3b111
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Aug 30, 2024
79678e8
update allow list
SiddharthMantri Aug 30, 2024
8fbf1c2
update allow list, add rule ignore to certain files
SiddharthMantri Sep 2, 2024
9ebee51
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 2, 2024
38dfaa1
check for node: imports
SiddharthMantri Sep 6, 2024
8e4d07d
include error for usage of upcoming `hash` function
SiddharthMantri Sep 11, 2024
440037d
address renamed default imports
SiddharthMantri Sep 16, 2024
cf26ac6
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 19, 2024
91b3729
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Sep 19, 2024
6193218
update dll manifests to ignore rule for renamed default import
SiddharthMantri Sep 19, 2024
d13dcc0
Merge branch 'main' into feature/FIPS_ESLint_Rule
SiddharthMantri Sep 19, 2024
4ce03b7
Merge branch 'main' into feature/FIPS_ESLint_Rule
SiddharthMantri Sep 20, 2024
82f16e7
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Sep 20, 2024
c7a8ecc
Merge branch 'main' into feature/FIPS_ESLint_Rule
kc13greiner Sep 20, 2024
b51c78f
Merge branch 'main' into feature/FIPS_ESLint_Rule
kc13greiner Sep 20, 2024
3cc88e5
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 20, 2024
427e451
Merge branch 'main' into feature/FIPS_ESLint_Rule
kc13greiner Sep 20, 2024
ff6e90f
Changing autoremoved imports
kc13greiner Sep 20, 2024
836b57b
Last autoremoved undos
kc13greiner Sep 20, 2024
efb31cc
2 more oops
kc13greiner Sep 20, 2024
ed67304
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Sep 20, 2024
4f143a1
Merge branch 'main' into feature/FIPS_ESLint_Rule
SiddharthMantri Sep 24, 2024
91e5029
revert ESlint ignore change from bad merge
SiddharthMantri Sep 25, 2024
2f55ed9
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 25, 2024
6e214de
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 25, 2024
3bcf283
Merge branch 'main' into feature/FIPS_ESLint_Rule
elasticmachine Sep 30, 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
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import * as Rx from 'rxjs';
import { map, takeUntil } from 'rxjs';

export const generateFileHash = (fd: number): Promise<string> => {
const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
const read = createReadStream(null as any, {
fd,
start: 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export const bootstrapRendererFactory: BootstrapRendererFactory = ({
publicPathMap,
});

const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
hash.update(body);
const etag = hash.digest('hex');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type SavedObjectTypeMigrationHash = string;
export const getMigrationHash = (soType: SavedObjectsType): SavedObjectTypeMigrationHash => {
const migInfo = extractMigrationInfo(soType);

const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash

const hashParts = [
migInfo.name,
Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-es/src/install/install_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,15 @@ async function sourceInfo(cwd: string, license: string, log: ToolingLog = defaul
log.info('on %s at %s', chalk.bold(branch), chalk.bold(sha));
log.info('%s locally modified file(s)', chalk.bold(status.modified.length));

const etag = crypto.createHash('md5').update(branch);
const etag = crypto.createHash('md5').update(branch); // eslint-disable-line @kbn/eslint/no_unsafe_hash
etag.update(sha);

// for changed files, use last modified times in hash calculation
status.files.forEach((file) => {
etag.update(fs.statSync(path.join(cwd, file.path)).mtime.toString());
});

const cwdHash = crypto.createHash('md5').update(cwd).digest('hex').substr(0, 8);
const cwdHash = crypto.createHash('md5').update(cwd).digest('hex').substr(0, 8); // eslint-disable-line @kbn/eslint/no_unsafe_hash

const basename = `${branch}-${task}-${cwdHash}`;
const filename = `${basename}.${ext}`;
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-eslint-config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ module.exports = {
'@kbn/eslint/no_this_in_property_initializers': 'error',
'@kbn/eslint/no_unsafe_console': 'error',
'@kbn/eslint/no_unsafe_js_yaml': 'error',
'@kbn/eslint/no_unsafe_hash': 'error',
'@kbn/imports/no_unresolvable_imports': 'error',
'@kbn/imports/uniform_imports': 'error',
'@kbn/imports/no_unused_imports': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-eslint-plugin-eslint/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ module.exports = {
no_this_in_property_initializers: require('./rules/no_this_in_property_initializers'),
no_unsafe_console: require('./rules/no_unsafe_console'),
no_unsafe_js_yaml: require('./rules/no_unsafe_js_yaml'),
no_unsafe_hash: require('./rules/no_unsafe_hash'),
},
};
116 changes: 116 additions & 0 deletions packages/kbn-eslint-plugin-eslint/rules/no_unsafe_hash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
const allowedAlgorithms = ['sha256', 'sha3-256', 'sha512'];

module.exports = {
allowedAlgorithms,
meta: {
type: 'problem',
docs: {
description: 'Allow usage of createHash only with allowed algorithms.',
category: 'FIPS',
recommended: false,
},
messages: {
noDisallowedHash:
'Usage of createHash with "{{algorithm}}" is not allowed. Only the following algorithms are allowed: [{{allowedAlgorithms}}]. If you need to use a different algorithm, please contact the Kibana security team.',
},
schema: [],
},
create(context) {
let isCreateHashImported = false;
let createHashName = 'createHash';
const sourceCode = context.getSourceCode();

const disallowedAlgorithmNodes = new Set();

function isAllowedAlgorithm(algorithm) {
return allowedAlgorithms.includes(algorithm);
}

function getIdentifierValue(node) {
const scope = sourceCode.getScope(node);
if (scope) {
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: might be slighter easier to read (early return + less indentation for the main body):

      if (!scope) {
        return;
      }

      const variable = scope.variables.find((variable) => variable.name === node.name);

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, added in 38dfaa1

const variable = scope.variables.find((variable) => variable.name === node.name);
if (variable && variable.defs.length > 0) {
const def = variable.defs[0];
if (
def.node.init &&
def.node.init.type === 'Literal' &&
!isAllowedAlgorithm(def.node.init.value)
) {
disallowedAlgorithmNodes.add(node.name);
return def.node.init.value;
}
}
}
return undefined;
}

return {
ImportDeclaration(node) {
if (node.source.value === 'crypto') {
Copy link
Member

Choose a reason for hiding this comment

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

issue: I assume, this doesn't catch node: protocol imports (import { createHash } from 'node:crypto';), or?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, it wouldn't have caught those. Addressed in 38dfaa1.

node.specifiers.forEach((specifier) => {
if (specifier.type === 'ImportSpecifier' && specifier.imported.name === 'createHash') {
Copy link
Member

Choose a reason for hiding this comment

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

question: any reason we don't need\want to cover createSign, createHmac, and upcoming hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @kc13greiner - wdyt? I think this is quite valid but I lack context on the FIPS recommendations

isCreateHashImported = true;
createHashName = specifier.local.name;
}
});
}
},
VariableDeclarator(node) {
if (node.init && node.init.type === 'Literal' && !isAllowedAlgorithm(node.init.value)) {
disallowedAlgorithmNodes.add(node.id.name);
}
},
AssignmentExpression(node) {
if (
node.right.type === 'Literal' &&
node.right.value === 'md5' &&
node.left.type === 'Identifier'
) {
disallowedAlgorithmNodes.add(node.left.name);
}
},
CallExpression(node) {
if (
(node.callee.type === 'MemberExpression' &&
node.callee.object.name === 'crypto' &&
node.callee.property.name === 'createHash') ||
(isCreateHashImported && node.callee.name === createHashName)
) {
if (node.arguments.length > 0) {
const arg = node.arguments[0];
if (arg.type === 'Literal' && !isAllowedAlgorithm(arg.value)) {
context.report({
node,
messageId: 'noDisallowedHash',
data: {
algorithm: arg.value,
allowedAlgorithms: allowedAlgorithms.join(', '),
},
});
} else if (arg.type === 'Identifier') {
const identifierValue = getIdentifierValue(arg);
if (disallowedAlgorithmNodes.has(arg.name) && identifierValue) {
context.report({
node,
messageId: 'noDisallowedHash',
data: {
algorithm: identifierValue,
allowedAlgorithms: allowedAlgorithms.join(', '),
},
});
}
}
}
}
},
};
},
};
99 changes: 99 additions & 0 deletions packages/kbn-eslint-plugin-eslint/rules/no_unsafe_hash.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

const { RuleTester } = require('eslint');
const { allowedAlgorithms, ...rule } = require('./no_unsafe_hash');

const dedent = require('dedent');

const joinedAllowedAlgorithms = `[${allowedAlgorithms.join(', ')}]`;

const ruleTester = new RuleTester({
parser: require.resolve('@typescript-eslint/parser'),
parserOptions: {
sourceType: 'module',
ecmaVersion: 2018,
ecmaFeatures: {
jsx: true,
},
},
});

ruleTester.run('@kbn/eslint/no_unsafe_hash', rule, {
valid: [
// valid import of crypto and call of createHash
{
code: dedent`
import crypto from 'crypto';

crypto.createHash('sha256');
Copy link
Member

Choose a reason for hiding this comment

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

question: Sorry, I missed that during the initial review, but it looks like this can be easily bypassed if someone uses a custom name for the default export/import (like in the example below). Is this something that can be caught at all? The same goes for async imports (await import('crypto').then((_crypto) => _crypto.)).

Suggested change
code: dedent`
import crypto from 'crypto';
crypto.createHash('sha256');
code: dedent`
import _crypto from 'crypto';
_crypto.createHash('sha1');

Copy link
Contributor

Choose a reason for hiding this comment

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

@azasypkin Made some changes here: 440037d I think it works.

`,
},
// valid import and call of createHash
{
code: dedent`
import { createHash } from 'crypto';

createHash('sha256');
`,
},
// valid import and call of createHash with a variable containing a compliant aglorithm
{
code: dedent`
import { createHash } from 'crypto';
const myHash = 'sha256';
createHash(myHash);
`,
},
],

invalid: [
// invalid call of createHash when calling from crypto
{
code: dedent`
import crypto from 'crypto';

crypto.createHash('md5');
`,
errors: [
{
line: 3,
message: `Usage of createHash with "md5" is not allowed. Only the following algorithms are allowed: ${joinedAllowedAlgorithms}. If you need to use a different algorithm, please contact the Kibana security team.`,
},
],
},
// invalid call of createHash when importing directly
{
code: dedent`
import { createHash } from 'crypto';

createHash('md5');
`,
errors: [
{
line: 3,
message: `Usage of createHash with "md5" is not allowed. Only the following algorithms are allowed: ${joinedAllowedAlgorithms}. If you need to use a different algorithm, please contact the Kibana security team.`,
},
],
},
// invalid call of createHash when calling with a variable containing md5
{
code: dedent`
import { createHash } from 'crypto';
const myHash = 'md5';
createHash(myHash);
`,
errors: [
{
line: 3,
message: `Usage of createHash with "md5" is not allowed. Only the following algorithms are allowed: ${joinedAllowedAlgorithms}. If you need to use a different algorithm, please contact the Kibana security team.`,
},
],
},
],
});
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export async function reportFailuresToFile(
// Jest could, in theory, fail 1000s of tests and write 1000s of failures
// So let's just write files for the first 20
for (const failure of failures.slice(0, 20)) {
const hash = createHash('md5').update(failure.name).digest('hex');
const hash = createHash('md5').update(failure.name).digest('hex'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
const filenameBase = `${
process.env.BUILDKITE_JOB_ID ? process.env.BUILDKITE_JOB_ID + '_' : ''
}${hash}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { parseFields, IBody, IQuery, querySchema, validate } from './fields_for'
import { DEFAULT_FIELD_CACHE_FRESHNESS } from '../../constants';

export function calculateHash(srcBuffer: Buffer) {
const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
hash.update(srcBuffer);
return hash.digest('hex');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const renderFullStoryLibraryFactory = (dist = true) =>
headers: HttpResponseOptions['headers'];
}> => {
const srcBuffer = await fs.readFile(FULLSTORY_LIBRARY_PATH);
const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
hash.update(srcBuffer);
const hashDigest = hash.digest('hex');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { ActionExecutionSourceType } from '@kbn/actions-plugin/server/types';
import type { FixtureStartDeps } from './plugin';

const hashParts = (parts: string[]): string => {
const hash = createHash('sha1');
const hash = createHash('sha1'); // eslint-disable-line @kbn/eslint/no_unsafe_hash
const hashFeed = parts.join('-');
return hash.update(hashFeed).digest('hex');
};
Expand Down