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

Trying to get in touch regarding a security issue #1058

Closed
JamieSlome opened this issue Dec 27, 2021 · 5 comments · Fixed by #1060
Closed

Trying to get in touch regarding a security issue #1058

JamieSlome opened this issue Dec 27, 2021 · 5 comments · Fixed by #1060

Comments

@JamieSlome
Copy link

@JamieSlome JamieSlome commented Dec 27, 2021

Hey there!

I belong to an open source security research community, and a member (@Haxatron) has found an issue, but doesn’t know the best way to disclose it.

If not a hassle, might you kindly add a SECURITY.md file with an email, or another contact method? GitHub recommends this best practice to ensure security issues are responsibly disclosed, and it would serve as a simple instruction for security researchers in the future.

Thank you for your consideration, and I look forward to hearing from you!

(cc @huntr-helper)

@Haxatron
Copy link

@Haxatron Haxatron commented Dec 27, 2021

Report is located here https://huntr.dev/bounties/50996581-c08e-4eed-a90e-c0bac082679c (only maintainers with write permissions can view)

This issue is different from the known issues listed here https://github.com/shelljs/shelljs/wiki/Security-guidelines

@nfischer
Copy link
Member

@nfischer nfischer commented Dec 31, 2021

Thanks for reaching out. I sent an email to @Haxatron earlier today. I'll review this report over the next few days and decide on next steps (if any).

nfischer added a commit that referenced this issue Jan 7, 2022
This locks down file permissions used by the internal implementation of
`shell.exec()`.

Issue #1058
Tested manually using the documented scenarios
nfischer added a commit that referenced this issue Jan 7, 2022
This locks down file permissions used by the internal implementation of
`shell.exec()`.

Issue #1058
Tested manually using the documented scenarios
nfischer added a commit that referenced this issue Jan 7, 2022
This locks down file permissions used by the internal implementation of
`shell.exec()`.

Issue #1058
Tested manually using the documented scenarios
@nfischer nfischer linked a pull request that will close this issue Jan 7, 2022
@nfischer nfischer added this to the v0.8.5 milestone Jan 7, 2022
nfischer added a commit that referenced this issue Jan 7, 2022
No change to code. This adds a security policy.

Issue #1058
nfischer added a commit that referenced this issue Jan 7, 2022
No change to code. This adds a security policy.

Issue #1058
@nfischer
Copy link
Member

@nfischer nfischer commented Jan 7, 2022

Thanks for the report. I believe this is a valid issue in ShellJS, so I've created a fix and pushed a release.

  • The patch for this is #1060. That PR landed on tip-of-tree (master branch), however I also cherrypicked this onto the 0.8-release branch.
  • This fix was released in 0.8.5. The patch for this bug is the only change between 0.8.4 and 0.8.5.
  • I'm trying to figure out a nice way to edit the CHANGELOG.md to explain what went into these two versions, but this doesn't work well with the tool we use to automate changelog generation. I'll need to think a bit more about how to mix manual edits with autogenerated changelog.
  • I added a SECURITY.md in #1061. For future reference, the best way to get in contact with me for security issues is with the email address in that page.

If you believe this patch is insufficient, please let me know privately via email and I'll gladly investigate further.

@nfischer nfischer closed this Jan 7, 2022
@JamieSlome
Copy link
Author

@JamieSlome JamieSlome commented Jan 7, 2022

@nfischer - thank you for such an in-depth follow-up of the report.

Would it be possible to confirm the fix commit SHA against the report?

You can also bag a bounty for your work as well! ❤️

nfischer added a commit to shelljs/release that referenced this issue Jan 8, 2022
Update to shelljs@0.8.5. See
shelljs/shelljs#1058.
nfischer added a commit to nfischer/shelljs-plugin-sleep that referenced this issue Jan 8, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.
nfischer added a commit to nfischer/shelljs-plugin-sleep that referenced this issue Jan 8, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.
nfischer added a commit to shelljs/changelog that referenced this issue Jan 8, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.
nfischer added a commit to nfischer/travis-check-changes that referenced this issue Jan 8, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.

I'm updating some metadata in the package.json. This also bumps engines
to node v8 because I think ShellJS is the only consumer of this package.
nfischer added a commit to shelljs/plugin-open that referenced this issue Jan 8, 2022
This downgrades mocha and eslint for node v4. This upgrades shelljs and
related dependencies, which includes shelljs/shelljs#1058.
nfischer added a commit to nfischer/shelljs-exec-proxy that referenced this issue Jan 8, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.
nfischer added a commit to shelljs/shx that referenced this issue Jan 9, 2022
No change to logic. This updates dependencies, which includes
shelljs/shelljs#1058.
@nfischer
Copy link
Member

@nfischer nfischer commented Jan 10, 2022

The report should be public now. I recommend folks upgrade to ShellJS 0.8.5 to ensure you have the fix.

This bug only impacts the synchronous version of shell.exec(). All other ShellJS methods (including the async usage of shell.exec()) should not be impacted, however it's of course perfectly safe to update ShellJS anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants