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

make install script have stable src code #2964

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link

@bmeck bmeck commented Mar 2, 2023

This is making security tooling have a high noise ratio for esbuild since every time a patch is published, even if it doesn't do anything new it looks like it has a new install script due to inlined version numbers; instead this PR makes it read out of package.json which has the value.

@evanw
Copy link
Owner

evanw commented Mar 2, 2023

Can you be specific? What security tooling are you talking about? Please describe the problem you're having.

@bmeck
Copy link
Author

bmeck commented Mar 2, 2023

@evanw I work at https://socket.dev and we detect a postinstall script in esbuild. Doing so users wish to ignore the script, and they can do so; however, currently every version of esbuild has a unique checksum for the install.js file since the publish process generates inline version strings. This means across versions, it appears that esbuild has changed the install.js file since checksums change for that file.

@evanw
Copy link
Owner

evanw commented Mar 3, 2023

This change seems somewhat meaningless to me. The obvious way of compromising the esbuild package would be to put something in the giant opaque 8mb binary executable, not in the easy-to-read install script. So I can make this change, but it doesn't feel like it meaningfully increase security here.

If your definition of non-suspicious is "we scanned the whole package and everything checked out" then you probably want to always flag esbuild as suspicious by that definition, since I assume you are not actually scanning esbuild itself. For example, I'm looking at https://socket.dev/npm/package/@esbuild/darwin-arm64 and it says the package is empty:

That's very incorrect. It seems to me like it'd be trivial to bypass the scans that https://socket.dev does by putting a malicious executable in a package. A human reviewing it using https://socket.dev would find it to be empty, which seems benign, when in fact it would actually be malicious. I'm guessing that from your point of view, you probably want to be pointing out to users when a package contains an opaque binary executable.

I'm not saying you should be suspicious of the esbuild binary on npm of course. I've done the work to ensure that esbuild's build system produces deterministic builds, and all binary executables published to npm are automatically verified against the source code on GitHub to flag a compromised build toolchain: https://github.com/evanw/esbuild/actions/workflows/validate.yml. But I'm assuming that's not the kind of test that https://socket.dev is able or willing to do, so then in your case you don't actually have the information necessary to successfully scan the esbuild package.

@evanw evanw closed this in 3f51381 Mar 3, 2023
@bmeck
Copy link
Author

bmeck commented Mar 3, 2023

Thanks much. Just explaining some things for more context

For example, I'm looking at https://socket.dev/npm/package/@esbuild/darwin-arm64 and it says the package is empty:

From the perspective of node loading that package; that is correct. Our issues are based upon entrypoint reachability. Completely scanning all possible files on the registry in depth is not something we currently provide but if properly motivated and not causing large amounts of noise it is certainly possible.

But I'm assuming that's not the kind of test that https://socket.dev/ is able or willing to do, so then in your case you don't actually have the information necessary to successfully scan the esbuild package.

That is good to know, for esbuild we might be able to do a one-off in the future but this kind of workflow is not standardized currently. We have thought about doing such deterministic build comparisons but have not created such a feature yet. If this seems appealing please get in touch.

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

Successfully merging this pull request may close these issues.

2 participants