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

Do not include `.git/index` when the `main` field in package.json is set to `index` #35

Closed
Raynos opened this issue Jun 27, 2019 · 14 comments

Comments

Projects
None yet
6 participants
@Raynos
Copy link

commented Jun 27, 2019

When the "main" field in package.json is set to index instead of index.js it will attempt to include all files called index in the tarball when running npm pack

This includes .git/index which includes the .git directory which makes npm fail with EISGIT at installation time ( not publish time ).

We do not want to publish .git because that poisons the tarball.

Steps to reproduce :

  • edit package.json "main" to index instead of index.js ( can be in npm-packlist )
  • run npm pack --dry-run
  • verify that .git/index is printed on STDOUT
@JLLeitschuh

This comment has been minimized.

Copy link

commented Jun 28, 2019

This should be considered a security vulnerability.

Exposing the .git/config file has security ramifications as often this file holds sensitive credentials!

CC: @evilpacket

@feross

This comment has been minimized.

Copy link

commented Jun 28, 2019

Other files like .DS_Store are also getting published. Might be related to this issue.

@DanielRuf

This comment has been minimized.

Copy link

commented Jun 30, 2019

Exposing the .git/config file has security ramifications as often this file holds sensitive credentials!

Not sure but I see no relevance to the reported issue which just includes .git/index or any file called index.

@feross see 7fcd045 and 68b7c96

@isaacs

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

As of 6.10.0, the root .git folder is no longer even being walked. I think this is probably not an issue any more. Care to check in the latest release?

@isaacs isaacs closed this Jul 3, 2019

@JLLeitschuh

This comment has been minimized.

Copy link

commented Jul 5, 2019

@isaacs Does this need a CVE number?

@DanielRuf

This comment has been minimized.

Copy link

commented Jul 5, 2019

@JJLeitschuh I don't think so or can you provide a PoC which adds the mentioned file by default?

#35 (comment)

As of 6.10.0, the root .git folder is no longer even being walked.

walk !== include, it just included the .git directory for searching the files to be packed but does not pack the full .git directory afaik.

@isaacs

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

walk !== include

In the case of this module, it does, because it only can include files from folders it walks, and it only includes folders containing included files.

68b7c96 fixes this issue.

PoC setup:

$ mkdir gh-35

$ cd gh-35

$ git init
Initialized empty Git repository in /Users/isaacs/dev/npm/npm-packlist/gh-35/.git/

$ echo '{"main":"index", "name":"testing-gh-35","version":"1.2.3"}' > package.json

$ git add package.json

$ git commit -m 'package.json'
[master (root-commit) 5a63f8c] package.json
 1 file changed, 1 insertion(+)
 create mode 100644 package.json

Prior to 68b7c96:

$ node -p 'require("../").sync()'
[ 'package.json', '.git/index' ]

On 68b7c96 and later:

$ node -p 'require("../").sync()'
[ 'package.json' ]

This change shipped in 8d85a56 on version 1.4.2.

Version 1.4.4 was added to the npm cli on npm/cli@ec62362, which was shipped in the npm cli version 6.10.0 on 2019-07-03: npm/cli@c1522be https://github.com/npm/cli/releases/tag/v6.10.0

If you wish to open a CVE, that's fine, but it should note the facts: this issue is resolved and a fix has already been shipped.

@JLLeitschuh

This comment has been minimized.

Copy link

commented Jul 6, 2019

I will file a CVE that the .git/index file is included.
This is simply an information disclosure vulnerability correct?

Sent with GitHawk

@DanielRuf

This comment has been minimized.

Copy link

commented Jul 6, 2019

Would be new for me that there is anything confidential in .git/index.

Just paths and hashes but no contents / credentials.

See https://mincong-h.github.io/2018/04/28/git-index/

@DanielRuf

This comment has been minimized.

Copy link

commented Jul 6, 2019

this issue is resolved and a fix has already been shipped.

Exactly.

@DanielRuf

This comment has been minimized.

Copy link

commented Jul 6, 2019

In the case of this module, it does

But not all files like .git/config. This is what I mean. It does not include the complete .git directory including objects, hooks, config, ...

@JLLeitschuh

This comment has been minimized.

Copy link

commented Jul 6, 2019

Upon further reflection, I don't think this is a security issue. Cheers everyone, thanks for your feedback!

@michaelsbradleyjr

This comment has been minimized.

Copy link

commented Jul 13, 2019

I experienced this today w/ npm 6.10.1 on macOS.

See: https://npm.community/t/installing-with-git-url-results-in-spurious-git-directory-within-node-modules/8815

There is discussion in that post of using a docker image that involves npm 6.9.0 so in just a moment I'll update with output from a docker run with 6.10.1.

@michaelsbradleyjr

This comment has been minimized.

Copy link

commented Jul 13, 2019

Okay, I'm actually coming back with a good report. 😄

In a fresh docker run of node:10.16.0 I upgraded to npm@6.10.1 and then when installing from the git:// URL as described in the bug report linked in my previous comment, I didn't experience the bugged behavior.

So then I went back to my macOS environment with 6.10.1, ran npm cache clear --force and retried the install with the git:// URL — no spurious .git/ folder! 🎉

Maybe npm 6.10.2 could include some bug-fix logic whereby on first-run it nukes from the cache any packages that were installed from git:// URLs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.