eslint-community/eslint-plugin-security

ESLint rules for Node Security

eslint-plugin-security's Issues

False positive for detect-non-literal-fs-filename

detect-non-literal-fs-filename seems to also be triggered when passing fs.writeFile to other functions, especially explicitly safe ones like util.promisify.

The message is also quite wrong ("Found fs.writeFile with non literal argument at index 0") while the actual warning is more that it's being passed in to a function and who knows what that function does with it?

Is this still maintained ?

Since the last commit is from 4.April 2019 ?

detect-child-process false-positive with spawn

const { spawn } = require('child_process');

Gives the Found require("child_process") error.

According to Avoiding Command Injection in Node.js, child_process.spawn is safer than child_process.exec.

So is it a false-positive, or does the detect-child-process rule tell me to completely avoid using child_process?

detect-child-process false-positive with exec

This plugin treats RegExp.prototype.exec() as child_process's method exec.

const string = 'hello'
const result = /hello/.exec(string) // Yields here

Is this a timing attack?

This doesn't seem right.

The keyword 'const' is reserved

Trying to use this plugin got a strange mistake in every file:
Parsing error: The keyword 'const' is reserved
Is there any ability to use it with ES6 code?

remove this issue

A more relevant "detect-object-injection"

Is there any way that we can work towards a more helpful/relevant report of Object injection sinks?

I can't think of a relevant security use case where Object injection would be relevant outside of the scope of a function directly linked to a web service.

I can understand based on tree traversal that determining the difference in between functions that are used in response to direct network calls would be [near] impossible to determine, but if I use bracket notation at the top level of my module, likely this rule should not notify.

Heuristics for detect-object-injection

security/detect-object-injection is a really important rule, but needs some love to become practical. What about adding some light heuristics?

A value is safe if either:

• It is a literal primitive or undefined
• It is an expression which contains only safe values with operators (a:safe + b:safe * c:safe is safe, while foo() + x:safe is unsafe)
• It is a template string whose interpolated values are safe

Otherwise it is unsafe (function expressions, etc).

A variable is safe if either:

• It is only assigned to safe values
• It is preceded by a comment containing @injectionsafe

Otherwise it is unsafe, and will trigger the warning.

 /* @injectionSafe */
const foo = bar()

or

let /* @injectionSafe */ a = safe(), b = 1337, c = unsafe()

I would like to take over as maintainer

Hi there,

This is just a public note to follow up on a Twitter conversation. In short, I think this project is important to the ESLint community and I'd like to offer to take ownership of it and continue its maintenance and development. I'm happy to work through whatever process GitHub/npm would like to do that.

cc @evilpacket @MylesBorins

Plugin security/recommended giving link error

When I use security/recommended plugin the url in the report points to a link which does not exist(404). Eg:- https://eslint.org/docs/rules/security/detect-object-injection

The solution is to have an extra else condition in template-generator.js
else if (_.startsWith(ruleId, 'security')) {
ruleId = ruleId.replace('security/', '');
ruleLink = https://github.com/nodesecurity/eslint-plugin-security#${ruleId};
}

Develop & Maintain

I developped a similar plugin which focuses in server side vulnerabilities in node.js. Looking for co-workers to develop and maintain it!
Plugin name in npm: eslint-plugin-security-node

False alarm for “Found fs.readFile with non literal argument at index 0”?

Originally asked at https://stackoverflow.com/questions/63262683/how-to-fix-found-fs-readfile-with-non-literal-argument-at-index-0

Copy to here:

---

I am trying to add eslint-plugin-security in a TypeScript project. However, for these codes

import { promises as fsp } from 'fs';
import fs from 'fs';
import path from 'path';

const index = await fsp.readFile(path.resolve(__dirname, './index.html'), 'utf-8');
const key = fs.readFileSync(path.join(__dirname, './ssl.key'));
await fsp.writeFile(path.resolve(__dirname, './sitemap.xml'), sitemap);

I got many these ESLint warnings:

warning Found fs.readFile with non literal argument at index 0   security/detect-non-literal-fs-filename
warning Found fs.readFileSync with non literal argument at index 0  security/detect-non-literal-fs-filename
warning Found fs.writeFile with non literal argument at index 0  security/detect-non-literal-fs-filename

I found the document about this ESLint error at https://github.com/nodesecurity/eslint-plugin-security#detect-non-literal-fs-filename

But I still have no idea how to fix it. Any guide will be helpful! Thanks

---

UPDATE:

Found out as long as using passing the path returned by path.join or path.resolve will show this ESLint issue.

If I change to absolute path, the ESLint issue is gone. However, this loose the benefit of the relative path by path.join or path.resolve.

fs.readFileSync('/Users/me/project/ssl.key');

Looking for an alternative / better way if exists.

Rule documentation in metadata

ESLint defines a metadata property (meta.docs.url) for rules to specify a URL where the full documentation for the rule can be found.

One of your users recently submitted jfmengels/eslint-rule-documentation#35 to get documentation links showing in some places, but this should be being specified using the metadata property so anywhere that supports ESLint rules will know the proper link to the documentation.

Ideally while solving this you probably should move the documentation into a docs/rule-name.md structure to allow for greater detail for each rule (and cleaner links), but that isn't a requirement 😉.

Note: This was originally filed as #1, but I believe that may have been before ESLint added the metadata property.

Conflict with ESLint quotes "backtick"

In our project, we decided to always use template literals with backticks instead of normal quotes as Template Literals are Strictly Better Strings. So we want to use const str = `text`; instead of const str = 'text';. See our issue on that topic.

Unfortunately, this plugin conflicts with that rule:

const fs = require('fs'); // error by ESLint's quotes rule
const fs = require(`fs`); // warning by this plugin's detect-non-literal-require rule
// -> No possibility to use require without errors/warnings

All rules enforcing a literal usage pass with single ticks and fail with back ticks, without respecting whether or not the template string does actually contain variables.

The rules actually check the argument's type to be 'Literal' (rules/detect-non-literal-require.js#L18), maybe it's possible to also check to case that the argument is non-literal, but is a template string doesn't containing any variables?

Add missing 1.5.0 changelog

The changelog for release 1.5.0 can't be found on: https://github.com/nodesecurity/eslint-plugin-security/blob/main/CHANGELOG.md

Add eslint-plugin-security

https://github.com/nodesecurity/eslint-plugin-security

Documentation for each rule?

Hi there. Thanks for this plugin - I've got it installed and just about every rule enabled.

But I'd love to know more about some of these. In particular, the detect-object-injection rule isn't explained very well. I took a look at all the blog entries listed here: https://nodesecurity.io/resources, but I didn't find anything.

Maybe just deep links into some of the talks on that page would be enough?

Thanks!

Deprecate detect-buffer-noassert

The detect-buffer-noassert rule is no longer needed by the noAssert option was removed from the Buffer API:
nodejs/node#18395

We can probably just mention this in the docs and update the meta of the rule.

I don't believe in removing rules just in case people are still using them.

detect-child-process TypeError: Cannot read property 'type' of undefined

Unless I use the below:

"rules": {
    "security/detect-child-process": "off"
 }

The linter fails with:

$ eslint --no-eslintrc -c ~/Downloads/eslint-4.json ~/workspace/lms-spa/app/
Cannot read property 'type' of undefined
TypeError: Cannot read property 'type' of undefined
at MemberExpression (/usr/local/lib/node_modules/eslint-plugin-security/rules/detect-child-process.js:34:87)
at listeners.(anonymous function).forEach.listener (/usr/local/lib/node_modules/eslint/lib/util/safe-emitter.js:47:58)
at Array.forEach ()
at Object.emit (/usr/local/lib/node_modules/eslint/lib/util/safe-emitter.js:47:38)
at NodeEventGenerator.applySelector (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:251:26)
at NodeEventGenerator.applySelectors (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:280:22)
at NodeEventGenerator.enterNode (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:294:14)
at CodePathAnalyzer.enterNode (/usr/local/lib/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
at Traverser.enter [as _enter] (/usr/local/lib/node_modules/eslint/lib/linter.js:865:28)
at Traverser._traverse (/usr/local/lib/node_modules/eslint/lib/util/traverser.js:132:14)

Is this a known issue with detect-child-process ?

Should __dirname be ignored in detect-non-literal-require?

const utils = require(__dirname + '/utils');
results in security/detect-non-literal-require warning although using the node constant __dirname is a common pattern.

Shouldn't this be handeled by a global whitelist?

Plugin security/recommended giving link error

When I use security/recommended plugin the url in the report points to a link which does not exist(404). Eg:- https://eslint.org/docs/rules/security/detect-object-injection

The solution is to have an extra else condition in template-generator.js
else if (_.startsWith(ruleId, 'security')) {
ruleId = ruleId.replace('security/', '');
ruleLink = https://github.com/nodesecurity/eslint-plugin-security#${ruleId};
}

Better unsafe regex detector

Hi all,

I'm a systems/security researcher at Virginia Tech and have been studying the incidence of vulnerable regexes in the wild.

This plugin's unsafe regex detector relies on safe-regex, which uses star height (nested quantifiers) to identify unsafe regexes.

Pros:

1. safe-regex is fast.
2. safe-regex is an npm module which makes it easy to work with.
3. safe-regex has no non-JS dependencies.

As a result, safe-regex is great for CI use cases.

Cons:

1. safe-regex is incorrectly implemented and substack is not maintaining it.
2. safe-regex has lots of false positives (e.g. (ab+)+).
3. safe-regex will only identify one type of exponential-time vulnerability, and ignores all polynomial-time vulnerabilities. In my research I found that, in the wild, polynomial-time vulnerabilities are far more common than exp-time vulnerabilities.

There are some alternatives to safe-regex that report exploit strings so you can tell if they're correct or not.

1. Rathnayake's rxxr2. Like safe-regex, this only checks for star height-style vulnerabilities. But it doesn't have false positives as far as I can tell.
2. Wustholz's REXPLOITER. This tests star height and other exp-time vulnerabilities, plus poly-time vulnerabilities.
3. Weideman's RegexStaticAnalysis. Like Wustholz's REXPLOITER, but open-source and it works better.

Unfortunately:

1. These alternatives all have non-JS dependencies (e.g. OCaml or Java) and have inconsistent interfaces.
2. Some (especially Weideman) can take minutes to test a single regex.

My project vuln-regex-detector provides a convenient wrapper for these alternatives, and enforces time and memory limits to get results or fail relatively quickly.

However, I'd be surprised if developers were willing to wait even 30 seconds for linting. To address that, I'm nearly done implementing a server side so queries can be answered by hitting the server for a pre-computed answer instead of doing the expensive computation locally. The server processes not-seen-before queries in the background so subsequent queries will get a real answer.

Once that's done, would you folks be interested in hitting my server first and falling back to safe-regex if my server hasn't seen the query before? I've got a sample client that can be used with a one-line tweak for this use case.

Are you still working on this?

Very interested in this plugin but is it possible you're not maintaining it anymore since the acquisition? ESLINT in my opinion is probably the best approach to write implement best practices around writing secured code.

If you're not maintaining it anymore would you be open to transfer this project to me? I'd like to keep it alive and update it with other rules.

Help wanted: Issue and PR Review

There are currently 30 open issues and 11 open pull requests. It would help me out tremendously if folks could go through them all and leave a comment containing:

1. Write a brief summary of the issue
2. Indicate if you think the issue is still relevant or not
3. Recommend what you think the next steps should be

Once we get this for all issues and PRs, I can figure out where to start.

False positive detect-non-literal-fs-filename on _.exists

Using lodash 4.17.4 and lodash-exists 1.0.3.

const _ = require('lodash');
require('lodash-exists');

...

if (_.exists(memberId)) {
  this.memberId = memberId;
}

Found fs.exists with non literal argument at index 0 security/detect-non-literal-fs-filename

add TravisCI / CircleCI and AppVeyor

Currently we have unit tests but these are not tested for every commit and PR.

Add recommended rules to plugin config?

It may be super-neat if there was a recommended config bundled in the plugin (Ref: eslint-plugin-react).

This would let us do something like:

{
  "extends": [
    "eslint:recommended",
    "plugin:security/recommended"
  ],
  "plugins": [
    "security"
  ]
}

I think it's as easy as adding a configs.recommended.rules object to the index.js:

/**
 * eslint-plugin-security - ESLint plugin for Node Security
 */

'use strict';

module.exports = {
  rules: {
    // ...
  },
  rulesConfig: {
    // ...
  },
  configs: {
    recommended: {
      rules: {
        'security/detect-buffer-noassert': 'warn',
        'security/detect-child-process': 'warn',
        'security/detect-disable-mustache-escape': 'warn',
        'security/detect-eval-with-expression': 'warn',
        'security/detect-new-buffer': 'warn',
        'security/detect-no-csrf-before-method-override': 'warn',
        'security/detect-non-literal-fs-filename': 'warn',
        'security/detect-non-literal-regexp': 'warn',
        'security/detect-non-literal-require': 'warn',
        'security/detect-object-injection': 'warn',
        'security/detect-possible-timing-attacks': 'warn',
        'security/detect-pseudoRandomBytes': 'warn',
        'security/detect-unsafe-regex': 'warn'
      }
    }
  }
};

Individual rules can overridden in the user's .eslintrc file:

{
  "extends": [
    "eslint:recommended",
    "plugin:security/recommended"
  ],
  "plugins": [
    "security"
  ],
  "rules": {
    "security/detect-object-injection": "off"
  }
}

Is it alive?

Seems like developers left the library development. So many issues not answered.

Package Modernization

The first priority for getting this package into a state where we can make changes is to go through and modernize the repo. Here are the steps:

• Switch the primary branch to main
• Set up automated CI testing using GitHub actions (separate lint and test steps)
• Enforce conventional commits formats on pull requests
• Update all dependencies to latest
• Set up a release process

Capture group names causing linter to report unsafe regex

The linter reports this as unsafe:

const regex = /\/portal\/(?<governmentCode>[\w]+)\/projects\/[\d]+\?section=(?<sectionId>[\d]+)/gi;

The linter report this as safe:

const regex = /\/portal\/([\w]+)\/projects\/[\d]+\?section=([\d]+)/gi;

Is there something unsafe about named groups, or the regex as I have it, or is this a false positive?

This plugin overrides current eslint rules

I am using this .eslintrc.json configuration:

{
    "extends": "airbnb-base",
    "env": {
      "es6": true,
      "node": true,
      "jest" : true
    },
    "parserOptions": {
      "ecmaVersion": 2018,
      "sourceType": "script",
      "ecmaFeatures": {
        "modules": false
      }
    },
    "plugins": [
      "security"
    ],
    "extends": [
      "plugin:security/recommended"
    ],
    "rules": {
      "space-before-function-paren": ["error", "never"],
      "comma-dangle": ["error", {
        "arrays": "always-multiline",
        "exports": "always-multiline",
        "functions": "ignore",
        "imports": "always-multiline",
        "objects": "always-multiline"
      }]
    }
}

By using the eslint-plugin-security for example the rules:

error| no-unused-vars: 'variable' is assigned a value but never used.
warning| no-console: Unexpected console statement.

are being overriden and eslint don't raise the notice.

Is it a problem of mine for a bad config file or a plugin problem?

Configuration:
OS: OSx 10.13.6
Text editor: VIM v.8.1.300 using ALE plugin v.2.1.0

Including "unicode bidi attacks" defense

A solution is https://github.com/lirantal/eslint-plugin-anti-trojan-source

Consider integrating with it?

detect-object-injection only reporting rule name (with empty data)

When detect-object-injection is run on Windows, it results in the following output:

Seems the plugin in only reporting the rule name, but nothing else that we would expect to see in the output, like the line number, or the filename, etc...

TypeError: Cannot read property 'type' of undefined in detect-child-process rule

Description

I get TypeError while linter is detecting MemberExpression of detect-child-process rule. The problem occurs while it's checking the exec() method. I'm using Mongoose as ODM to work with MongoDB. As it references here, there is an exec() method on its Query prototype to execute what query or callback we expect. But it is namesake with child_process.exec() and linter throws TypeError on every line that Mongoose Query.exec() appears.

https://github.com/nodesecurity/eslint-plugin-security/blob/edd1ae27245b0b220cbfdab59c3aaa0d279fea3d/rules/detect-child-process.js#L33-L37

Possible Solution

As I checked node.parent.arguments, there is no arguments while linting Mongoose exec(). I tested multiple scenarios of child_process.exec() and Mongoose Query.exec(). So that I believe if we check the emptiness of the arguments, the problem will be solved.

if (
  node.parent &&
  node.parent.arguments &&
  node.parent.arguments.length > 0 &&
  node.parent.arguments[0].type !== 'Literal'
) {
    return context.report(node, 'Found child_process.exec() with non Literal first argument');
}

Links Referencing linksecurity.io Broken

Links referencing linksecurity.io in the README file are broken. Based on the results of browsing to one of these links, it appears the domain registration may have expired/the site is no longer available.

Expected Result: Browsing to a linksecurity.io link directs the user to a post further explaining a rule.

Actual Result: The user is directed to a generic landing domain registrar landing page (see attached image).

Several issues with using this plugin

1. The security/detect-non-literal-fs-filename rule reports fs.${whatever} regardless of whether that's what i'm using or not. For example, i use jsonFile a lot, so an error with jsonFile.readFile gets reported as fs.readFile, when there is no reference to fs on that page. Sure, the error probably still applies, but it's confusing when the error output doesn't line up with the actual code

2. It's unclear, even with the linked resources, what exactly needs to be done to resolve an issue, or even what the issue is. With the above error, I was not at all sure what the problem really was or how to resolve it. For reference, that file (and just about all instances of that error on my code) are just using paths like this: ${process.cwd()}/path/to/json. I have a hard time seeing how this is exploitable?

3. I'm getting several unsafe regex errors. Most of them were common regexes (verify zip codes, stuff like that). So as recommended, I used the OWASP link to replace them with their safe variants. Even after replacing them, the error still persists

4. detect-object-injection. As this test is currently designed, it's completely overwhelming

I really want to use this plugin to easily catch minor security issues, but as is, i can't imagine it doing anything more than frustrate people

Current project status

Hi, what is the current status of this project? Is it still actively maintained?

tslint

How can I use this plugin with TSLint?

When I tried adding it to tsconfig.json, I got Invalid "extends" configuration value - could not require "plugin:security/recommended". Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) for the approximate method TSLint uses to find the referenced configuration file.

reference to external sources

I think each rule should point to some resource which is maintained by project or some reliable source not just blog post :)
e.g. https://blog.liftsecurity.io/2015/01/14/the-dangers-of-square-bracket-notation/ is dead already

for example this is good option and sits with rule itself.
https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-dynamic-require.md

Should not mention filename in error output

Currently the plugin is not just mentioning the error message itself but also adds the filename.

64:19  warning  Generic Object Injection Sink: /Users/sebastian/Workspace/sebastian-software/edgestack/src/webpack/ConfigFactory.js: 64
              copy[key] = obj[key]

                             security/detect-object-injection

which actually makes a lot of noise compared to other eslint checks.

ESLint already lists exactly the same issue location as the one mentioned in the message. So this is duplicate information.

Add eslint-plugin-xss

https://github.com/Rantanen/eslint-plugin-xss

Console text bold after running this extension

I have just installed this extension and I noticed is that my console text is stuck on bold right after the extension runs.

Maybe there is some text formatting that is not reset after it is done?

I'm using VS Code IDE.

security/detect-non-literal-fs-filename bug

In a react Component, I'm getting this warning even if I'm not using fs.open

 error  Found fs.open with non literal argument at index 0
111:                      onClick={() => this.open(item)}  security/detect-non-literal-fs-filename

continuous integration

Could you add a continuous integration so I could check whether the build is passing?
Eg. Travis CI - it's proven to be good and it's free for Open Source.

Documentation for `detect-new-buffer` is missing

This was added in #7 but somehow got removed later. In addition, that original description was a bit terse - can you include an explanation and/or a link as to why this is bad?

E.g. amqplib requires using a Buffer ctor with an arbitrary String argument to send messages. I could add a suppression, but want to understand the intent of this check.

Add ChangeLog

Reasons: http://keepachangelog.com/en/0.3.0/

Right now I need to update from 1.2 to 1.3 and can’t understand what are new features — commits list long and not clear.

detect-non-literal-require doesn't recognise backticks as literal

code sample:

const express = require(`express`);

will mark this line with a problem

`detect-non-literal-fs-filename` matches inconsistently

detect-non-literal-fs-filename matches all instances of id.open(nonLiteral), but this function name is used for more than the fs module.

On the other hand, when open is imported directly from fs as so:

import { open } from "fs"

the rule fails to fire.

Linter and formatter

What about using an style guide? I'm starting to use the AirBnb one in all my projects. This are the reasons:

• One of the most completes.
• ES6.
• ESLint setup file available with checks for everything covered in the Style guide.

More options:

• http://noeticforce.com/best-javascript-style-guide-for-maintainable-code
• https://github.com/feross/standard

I really don't care about which one, but IMHO we need to use one if we keep working in this awesome project. :)