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.

Sign up for GitHub

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

Jump to bottom

Installing malicious extension via cli should not dump a stack #42115

Closed
roblourens opened this issue Jan 24, 2018 · 4 comments
Closed

Installing malicious extension via cli should not dump a stack #42115

roblourens opened this issue Jan 24, 2018 · 4 comments
Assignees
@joaomoreno
Labels
bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions verified Verification succeeded
Milestone
January 2018

Comments

@roblourens
Copy link
Member

  • Install malicious extension through cli
  • Get a stack trace. Looks broke. Should only print error.
~ ❯❯❯ ~/Downloads/Visual\ Studio\ Code.app/Contents/Resources/app/bin/code --install-extension joaomoreno.banana
Found 'joaomoreno.banana' in the marketplace.
Installing...
Error: Can't install extension since it was reported to be malicious.
    at /Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cliProcessMain.js:125:555
    at Object.v [as _notify] (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:41:729)
    at Object.enter (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:45:61)
    at n.Class.derive._creator._run (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:46:866)
    at n.Class.derive._creator._completed (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:46:307)
    at n.Class.define.cancel.then (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:48:301)
    at Object.enter (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:44:109)
    at n.Class.derive._creator._run (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:46:866)
    at n.Class.derive._creator._setCompleteValue (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:46:560)
    at Object.v [as _notify] (/Users/roblou/Downloads/Visual Studio Code.app/Contents/Resources/app/out/vs/code/node/cli.js:41:709)
@joaomoreno joaomoreno assigned sandy081 and unassigned joaomoreno Jan 25, 2018
@joaomoreno
Copy link
Member

@sandy081 I actually get a very different output when running from sources:

vscode → ./scripts/code-cli.sh --install-extension joaomoreno.banana
Found 'joaomoreno.banana' in the marketplace.
Installing...
WARNING: Promise with no error callback:13
{ exception: null,
  error:
   Error: Can't install extension since it was reported to be malicious.
       at /Users/joao/Work/vscode/out/vs/platform/extensionManagement/node/extensionManagementService.js:265:31
       at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:59)
       at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:852:30)
       at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
       at _Base.Class.derive._creator._completed (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1036:18)
       at CompletePromise_ctor.CompletePromise_then [as then] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1566:49)
       at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:747:28)
       at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
       at _Base.Class.derive._creator._setCompleteValue (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1052:18)
       at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:28),
  promise:
   ErrorPromise_ctor {
     _value:
      Error: Can't install extension since it was reported to be malicious.
          at /Users/joao/Work/vscode/out/vs/platform/extensionManagement/node/extensionManagementService.js:265:31
          at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:59)
          at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:852:30)
          at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
          at _Base.Class.derive._creator._completed (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1036:18)
          at CompletePromise_ctor.CompletePromise_then [as then] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1566:49)
          at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:747:28)
          at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
          at _Base.Class.derive._creator._setCompleteValue (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1052:18)
          at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:28),
     _isException: false,
     _errorId: 13 },
  handler: undefined,
  id: 13,
  parent: undefined }
/Users/joao/Work/vscode/out/vs/base/common/errors.js:59
                        throw new Error(e.message + '\n\n' + e.stack);
                        ^

Error: Can't install extension since it was reported to be malicious.

Error: Can't install extension since it was reported to be malicious.
    at /Users/joao/Work/vscode/out/vs/platform/extensionManagement/node/extensionManagementService.js:265:31
    at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:59)
    at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:852:30)
    at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
    at _Base.Class.derive._creator._completed (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1036:18)
    at CompletePromise_ctor.CompletePromise_then [as then] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1566:49)
    at Object.enter (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:747:28)
    at _Base.Class.derive._creator._run (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1068:29)
    at _Base.Class.derive._creator._setCompleteValue (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1052:18)
    at Object.notifySuccess [as _notify] (/Users/joao/Work/vscode/out/vs/base/common/winjs.base.raw.js:1170:28)
    at Timeout._onTimeout (/Users/joao/Work/vscode/out/vs/base/common/errors.js:59:31)
    at ontimeout (timers.js:386:14)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)

This indicates that errors are not being propagated back. Here's the culprit:

https://github.com/Microsoft/vscode/blob/0e25fa2ad13b57cfc23832e660554cdce93470bf/src/vs/platform/extensionManagement/node/extensionManagementService.ts#L246

That line just swallows up errors and doesn't let them reach the outside.

sandy081 added a commit that referenced this issue Jan 25, 2018
@sandy081
#42115 handle error
f93fb56
@sandy081
Copy link
Member

@joaomoreno Above line returns the wrapped errors. Issue is at different place (see the above fix). Some how this only happens when installed from cli otherwise, error is properly propagated (from workbench for eg).

Please take a look if you want to show the complete stack trace or not in CLI. IMO it is fine to show it.

@sandy081 sandy081 assigned joaomoreno and unassigned sandy081 Jan 25, 2018
@sandy081 sandy081 added this to the January 2018 milestone Jan 25, 2018
@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions labels Jan 25, 2018
@joaomoreno
Copy link
Member

@sandy081 You're absolutely right! Good fix.

I think we shouldn't show the stack, but just the exception message.

@chrmarti
Copy link
Contributor

chrmarti commented Feb 2, 2018

@joaomoreno will add the test extension back to the blacklist for testing.

@chrmarti chrmarti added the verified Verification succeeded label Feb 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@joaomoreno joaomoreno

Labels
bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions verified Verification succeeded
Projects
None yet
Milestone
January 2018
Development

No branches or pull requests
4 participants
@joaomoreno @roblourens @chrmarti @sandy081