JS CWE-732: Overpermissive file creation

[ghost]

Description

World writable resources such as config files can allow other users to control program behavior. In some cases there's code injection through the config file, which can lead to privilege elevation. World writable directories have the same weakness since they allow adding files within them. A world writable config directory /etc/froznator/conf.d offers a route to controlling program behavior through a new config file. MITRE calls this CWE-732, one of the top 25 most dangerous vulnerabilities.

This set of queries finds creations of world writable files and directories. It aims to detect all the major ways of specifying mode:

• Eliding mode, which uses the Node.js default that grants world write permission.
• As a literal number or string in the call.
• As a literal first transported through variables and expressions, eg by storing a program default mode.
• Constructed by setting bits. fs.constants.S_IRWXU | fs.constants.S_IRWXG
• Constructed by clearing bits. 0o777 & ~fs.constants.S_IRWXO

It also detects functions which offer no way of specifying mode. These functions always use the default world writable mode and are therefore unsecurable. Query help recommends replacing these functions with securable alternatives.

The queries check for these conditions:

• Unsecurable file creation
• Modeless file creation
• Overpermissive file creation with a literal mode
• Overpermissive file creation with a transported mode
• Overpermissive file creation with a mode constructed by inclusion
• Overpermissive file creation with a mode constructed by exclusion
• Unsecurable directory creation
• Modeless directory creation
• Overpermissive directory creation with a literal mode
• Overpermissive directory creation with a transported mode
• Overpermissive directory creation with a mode constructed by inclusion
• Overpermissive directory creation with a mode constructed by exclusion

The queries target the standard fs module plus replacement modules via NodeJSLib::FS::moduleMember, along with the fs/promises interface. Several popular npm modules are also detected. They are, with weekly downloads:

• fs-extra 40M
• fs-extra-p 20K
• mkdirp 37M
• jsonfile 26M
• cp-file 2M
• make-dir 35M
• cpy 1M
• move-file 200K
• @npmcli/move-file 5M
• trash 59K

Calls and constructions are both detected since they both result in a creation. Number and octal numeric string mode specifiers are both detected and correctly interpreted.

Libraries ModableFileCreation.qll ModableDirectoryCreation.qll define the functions that can take a mode. Tests can't import these due to the dash in the path experimental/Security/CWE-732/*. I did a strange thing of defining libraries that expose a query predicate to make them testable, ModableFileCreationTest.qll ModableDirectoryCreationTest.qll. My inclination is to test these but if that's just an awful pattern I can remove them.

I have equivalent queries for the other languages on the way.

Omitted

Related modules ncp cpr were left out because they always copy permissions. rimraf was left out because it never creates files. makedir was left out for low usage.

Hard links always have the same permissions as target. Symbolic links can sometimes have their own permissions but they can never bypass target permissions.

False Positives

The queries only find creation functions called with an insecure mode. But in some cases there's surrounding logic that makes creation impossible / virtually impossible. If there's an immediate preceding read from the same file, it will fail if the file doesn't exist and so the write should never create a new file. The queries don't detect this situation.

Results

Unsecurable file creation

https://lgtm.com/query/8754882199744511397/

• eclipse-theia/theia
• semantic-release/changelog
• serverless/serverless

Unsecurable directory creation

https://lgtm.com/query/2881405778980302287/

• ampproject/amphtml
• angular/angular

Modeless file creation

https://lgtm.com/query/308122751628928618/

• mozilla/pdf.js
• Semantic-Org/Semantic-UI
• ampproject/amphtml
• angular/angular
• emberjs/ember.js
• ether/etherpad-lite
• meteor/meteor

Modeless directory creation

https://lgtm.com/query/8429440007276116881/

• mozilla/pdf.js
• Semantic-Org/Semantic-UI
• ampproject/amphtml
• angular/angular
• emberjs/ember.js
• ether/etherpad-lite
• meteor/meteor

File creation with literal mode

https://lgtm.com/query/8337507973041063599/

• jspm/jspm-cli

Directory creation with literal mode

https://lgtm.com/query/7981273486056700546/

• getsentry/sentry-electron

File creation with transported mode

https://lgtm.com/query/2480237548234300509/

• base-cms/base-cms
• withspectrum/spectrum

Directory creation with transported mode

https://lgtm.com/query/1573873983796861180/

• withspectrum/spectrum

File creation with mode constructed by inclusion

Had to run this one locally.

• jscutlery/semver

Directory creation with mode constructed by inclusion

Had to run this one locally.

• xialy8/SCRepair

Mode constructed by exclusion

I do find examples of the exclusion pattern for constructing mode but none of them are vulnerable, so you guys might want those ones removed.

Reports

I reported to all the active projects. Only jspm was inactive. Receptiveness varied. Some projects updated right away, others were dismissive. PDF.js had mixed feelings, with some devs thinking it's valuable and others aggressively opposed.

• jscutlery/semver
• base-cms/base-cms
• eclipse-theia/theia
• getsentry/sentry-electron
• Semantic-Org/Semantic-UI
• semantic-release/changelog
• serverless/serverless
• withspectrum/spectrum
• xiaoly8/SCRepair
• mozilla/pdf.js

For the projects with a disclosure policy I sent private reports.

• ampproject/amphtml
• angular/angular
• emberjs/ember.js
• ether/etherpad-lite
• meteor/meteor

For the modules with unsecurable functions I suggested exposing a mode parameter so they can be secured.

• fs-extra
• fs-extra-p
• move-file
• @npmcli/move-file

Patches

• semver
• move-file
• cp-file
• trash

Modules

I went ahead and published secure-fs, a drop in replacement fs with secure modes by default. Hopefully that can lower the cost of choosing security for devs.

import fs from 'secure-fs'

fs.writeFile('file', 'SensitiveData', () => {
  // File created with secure mode
})

fs-extra was unreceptive to changing the interface so I published secure-fs-extra. It uses secure modes by default and offers controllable modes everywhere.

import fs from 'secure-fs-extra'

// Full path created securely
await fs.ensureDir('/var/froznator/queue')

// File and full path created securely
await fs.outputFile('/etc/froznator/conf.d/main.conf', 'AdminPassword=123')

// File and full path created with carefully loosened permissions
await fs.outputJSON('/srv/froznator/feed.json', feed, {
  mode: 0o640, // owner RW, group R
  dirMode: 0o750 // owner RWX, group RX
})

// Read sensitive data with confidence
const config = await readConfig('/etc/froznator/conf.d/main.conf')
if (password === config.AdminPassword) showAdminInterface()

Discussion

This tentative blog post describes the queries, to be finalized after code review.

https://dittyroma.wordpress.com/2021/06/17/javascript-overpermissive-file-creation/

[esbena]

Hi @dittyroma

With some adjustments, we would like to merge this significant PR from you, can I convince you to reopen it? I think the query can be valuable for audits, but it I expect that it is too prone to false positives to be run by default, but will not prevent it from being being merged. Perhaps CWE-276 should be used to reflect that the query does not make an attempt to identify the kind of file/directory that is being created?

The primary adjustment I would like to see is a consolidation of the 12 queries into one or perhaps two queries. It looks like your qll files already are general enough to support this (well done!). What do you think of that suggestion?

I suspect that such an adjustment would cut significantly down on the need for qhelp and query info text, making the PR easier to review and maintain.

As an example. Consider js/insecure-fs/modeless-directory-creation and js/insecure-fs/modeless-file-creation. I suspect they could be turned into a single more general query in the style of:

/**
 * @name Modeless file or directory creation
 * @description Creating files or directories as world writable may allow an attacker to control
 *              program behavior.
 * @kind problem
 * @problem.severity warning
 * @id js/insecure-fs/modeless-entry-creation
 * @tags security
 *       external/cwe/cwe-732
 */

import ModableEntryCreation

from ModableEntryCreation creation
where creation.isInsecure() and creation.isModeless()
select creation, "This modeless call creates world a writable " + creation.getEntryKind() + "."

Similar simple unifications should be possible throughout the ql files.

The tricky part may be the merging of the @kind problem and the @kind path-problem queries. I think that consolidation can be skipped.

I am happy to help out with suggestions for how to merge additional queries if you get stuck.

[ghost]

Hi @esbena. Thank you for suggesting this.

I was thinking of the code as my main submission here. I imagined doing a series of builds like this. Chose something simple as a way to learn the language, hoping it would turn into some ongoing coding. Does that sound like something the team might be interested in?

I've done some coding bounties on eg Bountysource in the past. Perhaps we could communicate beforehand and the team could set some requirements, similar to what you would post in a bounty. If there are areas valuable to the project that could be filled out, I'd be glad to do more builds with quality similar to this work.

[esbena]

hoping it would turn into some ongoing coding. Does that sound like something the team might be interested in?

Sure. We are happy to iterate on pull requests.

Perhaps we could communicate beforehand and the team could set some requirements, similar to what you would post in a bounty.

https://github.com/github/securitylab/discussions is the suggested place to discuss potential queries.
The formal requirements for submissions can be seen here: https://securitylab.github.com/bounties/.

[ghost]

Alright. It won't lead to what I want so I'm leaving this one alone.