-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 9 commits
0b1705a
68b92f1
264709e
820d47c
ba435ba
03c0b5c
fd26972
6337115
6b3b111
79678e8
8fbf1c2
9ebee51
38dfaa1
8e4d07d
440037d
cf26ac6
91b3729
6193218
d13dcc0
4ce03b7
82f16e7
c7a8ecc
b51c78f
3cc88e5
427e451
ff6e90f
836b57b
efb31cc
ed67304
4f143a1
91e5029
2f55ed9
6e214de
3bcf283
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
*/ | ||
|
||
module.exports = { | ||
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) { | ||
const allowedAlgorithms = ['sha1', 'sha256', 'sha3-256']; | ||
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: I assume, this doesn't catch There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: any reason we don't need\want to cover There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(', '), | ||
}, | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
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 rule = require('./no_unsafe_hash'); | ||||||||||||||||||
const dedent = require('dedent'); | ||||||||||||||||||
|
||||||||||||||||||
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'); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: [sha1, sha256, sha3-256]. 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: [sha1, sha256, sha3-256]. 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: [sha1, sha256, sha3-256]. If you need to use a different algorithm, please contact the Kibana security team.', | ||||||||||||||||||
}, | ||||||||||||||||||
], | ||||||||||||||||||
}, | ||||||||||||||||||
], | ||||||||||||||||||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we may want to avoid sha1 if we can?
https://www.nist.gov/news-events/news/2022/12/nist-retires-sha-1-cryptographic-algorithm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Addressed in 79678e8