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

Update LanguageServerHost to use NamedPipe #69816

Closed

beccamc wants to merge 7 commits into dotnet:main from beccamc:u/beccam/pipe

Contributor

beccamc commented Sep 5, 2023

Current communication between client and the Rolsyn language server is writing to STDOUT. This makes the connection vulnerable to any code in the process writing to stdout stream and corrupting the data.

We have seen at least one bug with this problem: microsoft/vscode-dotnettools#333

The fix it to use a named pipe to communicate. Two things to note about this...

The named pipe connection is created by the client. It's a little confusing, but the language client creates the named pipe server, and the language server actually creates a named pipe client.
The connection string format for .NET code is different than the connection string format for nodejs. We pass the named pipe name only, and then construct the appropriate connection string. If you try to use the same connection string it will fail silently.

beccamc added 3 commits

August 31, 2023 15:01


          add namedpipe

add2080


          change pipe to client

deaa53c


          update pipeName description

5786c09

beccamc requested a review from a team as a code owner

September 5, 2023 18:12

dotnet-issue-labeler bot added Area-IDE untriaged labels

Contributor Author

beccamc commented Sep 5, 2023

Do we have any tests for this component? I don't see any...

beccamc mentioned this pull request

Use NamedPipe for server communication dotnet/vscode-csharp#6282

Closed

Member

dibarbet commented Sep 5, 2023

Do we have any tests for this component? I don't see any...

We don't for the program component itself, only for the component that we pass the streams to. We likely should but I haven't gotten to it.

dibarbet approved these changes

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Outdated Show resolved Hide resolved

dibarbet reviewed

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Show resolved Hide resolved

jasonmalinowski reviewed

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Outdated Show resolved Hide resolved

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Outdated

Comment on lines 226 to 229

+              static string GetDotNetPipeConnectionString(string pipeName)
+              {
+                  return "\\\\.\\" + pipeName;
+              }

Member

jasonmalinowski Sep 5, 2023

Does this syntax also work on Mac/Linux pipes? Or should we expect that the host is providing the Win32 name?

Contributor Author

beccamc Sep 5, 2023

I am not sure! Attempting to test on mac now

jasonmalinowski reviewed

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs

+                  var pipeNameOption = new CliOption<string>("--pipe")
+                  {
+                      Description = "The name of the pipe to use for RPC with the parent process.",
+                      Required = true

Member

jasonmalinowski Sep 5, 2023

Should we still allow for stdin/stdout for a bit longer, if only so that way we don't have to merge/flow this simultaneously with the VS Code side of things?

Contributor Author

beccamc Sep 5, 2023

David and I discussed this, and determined it would be fine to merge both PRs close together. The only risk is that someone else updates roslyn version before I get PR in, correct?


          pr comments

b918214

build-analysis bot mentioned this pull request

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

beccamc added 2 commits

September 6, 2023 17:20


          update for linux

39841d0


          current user

9b0d54d

dibarbet reviewed

View reviewed changes

Member

dibarbet left a comment

One thing we should change, otherwise looks good.

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs Outdated Show resolved Hide resolved


          cleanup pipe name

555bd3c

beccamc commented

View reviewed changes

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs

                   // TODO: Remove, the path should match exactly. Workaround for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1830914.
                   Microsoft.CodeAnalysis.EditAndContinue.EditAndContinueMethodDebugInfoReader.IgnoreCaseWhenComparingDocumentNames = Path.DirectorySeparatorChar == '\\';
-                  var server = new LanguageServerHost(Console.OpenStandardInput(), Console.OpenStandardOutput(), exportProvider, loggerFactory.CreateLogger(nameof(LanguageServerHost)));
+                  // Named pipe server is actually created from the client, so here we create a client stream.
+                  var pipeServer = new NamedPipeClientStream(serverName: ".", serverConfiguration.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous);

Contributor Author

beccamc Sep 7, 2023

@dibarbet PipeOptions.CurrentUserOnly isn't working. It connects but doesn't send any messages. I could investigate further (I don't know if the issue is between .net and nodjs?) but I don't know if it's worth the time. I don't think this is less secure than the stdin/out was? Thoughts?

Member

dibarbet Sep 7, 2023

I wonder if the client is not creating a pipe that applies to the current user.

Honestly I'm not super familiar with named pipes - but it seems like a lower privileged user could hijack the pipe name and then get access to files from the server that they normally couldn't get. I'm not sure it would be as easy to hijack stdin/out. But maybe it doesn't matter if the pipe is created in the user's tmp dir?

But I don't really know much about named pipes. Not sure @jaredpar might have additional insight?

Member

jasonmalinowski Sep 7, 2023

I don't think this is less secure than the stdin/out was?

You can't get a handle to another processes stdin/out, but depending on how the pipe is created, this could be. This is a pretty common source of security bugs in the Linux world, especially since /tmp is world-accessible. The potential attack here is imagine you have two users on a machine, one launches VS Code. The other malicious user sees the pipe get created, and connects to it prior to our LSP connecting to it. In that case, you're now able to send LSP messages to the other user's client, which could be potentially bad/broken. (Imagine getting didOpen for files you can't otherwise read, or sending LSP messages back which executes stuff on the VS Code side...)

It's entirely possible we're OK here if the pipe creation API is already doing the right thing, but we'd need to validate that.

Member

dibarbet Sep 7, 2023

I also don't see a way to configure access on the nodejs side - https://nodejs.org/api/net.html#netcreateserveroptions-connectionlistener

Member

dibarbet Sep 7, 2023

Found it here - https://nodejs.org/api/net.html#serverlistenoptions-callback - looks like the default should be current user, so that looks ok (unless vscode is launching as a different user)

Member

jaredpar Sep 7, 2023

Where do you see the default as current user?

Member

dibarbet Sep 7, 2023 •

edited

Loading

Under server.listen options (from https://nodejs.org/api/net.html#serverlistenoptions-callback ), e.g.

beccamc closed this

Contributor Author

beccamc commented Sep 12, 2023

closed in favor of #69918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged