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

Makefile auto generating output results in executing actions on the makefile which may have inadvertent behavior including forking processes and executing code. #505

Closed
tylerhjones opened this issue Sep 28, 2023 · 10 comments
Labels
bug Something isn't working fixed-pending-release Fix is merged and will be included in the next release.
Milestone

Comments

@tylerhjones
Copy link

tylerhjones commented Sep 28, 2023

Open a project with the makefile extension installed and this makefile present in the project.

otherthing:
	@echo "building otherthing with blocking call"
	sleep 999

.env:
	@if [ -z "$$doesnt_exist" ]; then \
		$(MAKE) otherthing; \
	fi

%: .env
	@echo "building $@"

Once the Configuring: Generating dry-run output... executes (which it does automatically)
Screenshot 2023-09-28 at 3 49 52 PM

This will result in a process fork like so

-+= 00001 root /sbin/launchd
 \-+- 25987 tjones /Library/Developer/CommandLineTools/usr/bin/make --dry-run --keep-going --print-directory
   \-+- 25988 tjones /bin/sh -c if [ -z "$doesnt_exist" ]; then \011/Library/Developer/CommandLineTools/usr/bin/make otherthing; fi
     \-+- 25989 tjones /Library/Developer/CommandLineTools/usr/bin/make otherthing
       \-+- 25990 tjones /bin/sh -c if [ -z "$doesnt_exist" ]; then \011/Library/Developer/CommandLineTools/usr/bin/make otherthing; fi
         \-+- 25991 tjones /Library/Developer/CommandLineTools/usr/bin/make otherthing
           \-+- 25992 tjones /bin/sh -c if [ -z "$doesnt_exist" ]; then \011/Library/Developer/CommandLineTools/usr/bin/make otherthing; fi
             \-+- 25994 tjones /Library/Developer/CommandLineTools/usr/bin/make otherthing
 ...etc

While this might not be a normal makefile, just opening a project withit should not result in a forkbomb / code execution on the hostmachine due to an automatic action from the makefile extension.

@tylerhjones tylerhjones changed the title Makefile auto generating output results in executing actions on the makefile which may have inadvertent behavior including forking processes. Makefile auto generating output results in executing actions on the makefile which may have inadvertent behavior including forking processes and executing code. Sep 28, 2023
@lochyj
Copy link

lochyj commented Oct 2, 2023

I reported this to msrc and I am waiting to hear back from them... it's been over a week now.
There is another way to reproduce this using one line that may be inconspicuous.

@gcampbell-msft
Copy link
Collaborator

@lochyj Thank you for following up. We have been investigating this and will continue to do so this week. We will update you with any status we have as soon as we have it.

@andreeis is the person currently investigating this. Thanks!

@gcampbell-msft gcampbell-msft added the bug Something isn't working label Oct 2, 2023
@lochyj
Copy link

lochyj commented Oct 2, 2023

Thanks for the context Garret!

@andreeis
Copy link
Contributor

andreeis commented Oct 3, 2023

@lochyj and @tylerhjones, we are going to work on two fixes for this:

  • honor the trust workspace contract in VSCode, which we currently don't (work item Support Workspace Trust #160)
  • popup a warning even when a workspace is trusted (until the user dismissed it with a "don't show again") that loading a makefile repository in VSCode (while having Makefile Tools installed) is going to invoke "make --dry-run" which for a limited set of corner cases will result in code being run versus listed only. One time warning about possible commands being run via --dry-run #506

In your case, what is causing this is the $MAKE recursive invocation which (as I only recently looked at closer via another bug that was opened) is losing the --dry-run from the main process. We are looking at fixing most common scenarios of that with a PR from the community, #500

I am thinking that we can close this ticket since workspace trust will be covered by 160 and a warning (until user dismisses it permanently) will be implemented with 506. On top of that. We don't know yet how we would fix the recursive calls of sub-make but we're still brainstorming.

Anything else about this issue that you'd like to see us fix besides what I listed above? Ok to close the ticket?

@andreeis andreeis added this to the 0.8.0 milestone Oct 3, 2023
@lochyj
Copy link

lochyj commented Oct 3, 2023

That sounds like a good fix to me. Although i would like to point out that this is only an issue when the C/C++ extension is also installed - at least for me. Other than that I'd say this is resolved.

@tylerhjones
Copy link
Author

Thanks for your input and time addressing this issue. Your plan sounds good to me.

@andreeis andreeis added the fixed-pending-release Fix is merged and will be included in the next release. label Oct 23, 2023
@drok
Copy link
Contributor

drok commented Oct 23, 2023

I suggest creating a [sandbox] label to track this, and other issues that have not been reported yet, grouped loosely in a "trust" class. A makefile is an arbitrary script, and should be handled with respect, meaning enforced, reasonable limits. I suggested a sandbox mode at drok#2

While @lochyj and @tylerhjones may be satisfied if the existing "workspace trust" mode worked correctly, personally I don't. Insted I find trust mode to be merely a waiver of responsibility, and would expect a more deliberate handling of trust issues.

I am tracking sandboxing issues independently at https://github.com/drok/vscode-makefile-tools/labels/sandbox on order to manage related issues with a more fine-grained resolution, and to further define the requirements of such a sandbox mode; I might implement one myself. I would be interested to know if there is interest and support from others on this topic; comment at drok#2 or on this issue please.

@lochyj
Copy link

lochyj commented Oct 24, 2023

@drok This is a sound suggestion and I can think of a number of cases where this is optimal over just a "trust" based system. This may however, impact the startup time of vscode to a point where it isn't worth it. Although, I don't develop vscode plugins and I may be wrong about the performance, I would be happily proved wrong.

@drok
Copy link
Contributor

drok commented Oct 27, 2023

@lochyj I have added some reading references at drok#2; they are all Linux specific; I don't yet know how resource limits might be implemented on Windows or Darwin. I'm not sure on what basis you infer performance issues (do you assume 'sandbox' is a synonym for 'java VM'?) The term merely refers to means of controlling resource usage, but does not imply any specific tech.

Would an unconstrained fork-bomb be acceptable, if the startup time is fast? 😉

Copy link

🎉 This issue has now been fixed and is now available in the latest release! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed-pending-release Fix is merged and will be included in the next release.
Projects
Status: Done
Development

No branches or pull requests

5 participants