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

Discover plugins installed using Net tools #5990

Open
wants to merge 21 commits into
base: dev-feature-dot-net-tool-plugin-support
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(TargetFrameworksLibraryForSigning)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworksLibraryForSigning);$(NETCoreTargetFramework)</TargetFrameworks>
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
<TargetFramework />
<NoWarn>$(NoWarn);CS1591;CS1573;CS0012;RS0041</NoWarn>
<PackageTags>nuget protocol</PackageTags>
Expand Down
109 changes: 108 additions & 1 deletion src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Common;
Expand Down Expand Up @@ -140,6 +143,105 @@ private List<PluginFile> GetPluginFiles(CancellationToken cancellationToken)
return files;
}

/// <summary>
/// Gets auth plugins installed using dotnet tools. This is done by iterating through each file in directories found int eh
/// `PATH` environment variable.
/// The files are also expected to have a name that starts with `nuget-plugin-`
/// </summary>
/// <returns></returns>
private static List<string> GetNetToolsPluginFiles()
{
var pluginFiles = new List<string>();
var paths = Environment.GetEnvironmentVariable("PATH")?.Split(Path.PathSeparator) ?? Array.Empty<string>();

foreach (var path in paths)
{
if (Directory.Exists(path))
{
var directoryInfo = new DirectoryInfo(path);
var files = directoryInfo.GetFiles("nuget-plugin-*");

foreach (var file in files)
{
if (IsValidPluginFile(file))
{
pluginFiles.Add(file.FullName);
}
}
}
}

return pluginFiles;
}

/// <summary>
/// Checks whether a file is a valid plugin file for windows/Unix.
/// Windows: It should be either .bat or .exe
/// Unix: It should be executable
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
private static bool IsValidPluginFile(FileInfo fileInfo)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return fileInfo.Extension.Equals(".exe", StringComparison.OrdinalIgnoreCase) ||
fileInfo.Extension.Equals(".bat", StringComparison.OrdinalIgnoreCase);
}
else
{
#if NET8_0
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
var fileMode = File.GetUnixFileMode(fileInfo.FullName);

return fileInfo.Exists &&
((fileMode & UnixFileMode.UserExecute) != 0 ||
(fileMode & UnixFileMode.GroupExecute) != 0 ||
(fileMode & UnixFileMode.OtherExecute) != 0);
#else
return fileInfo.Exists && (fileInfo.Attributes & FileAttributes.ReparsePoint) == 0 && IsExecutable(fileInfo);
Copy link
Contributor

@kartheekp-ms kartheekp-ms Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please help me understand the reasoning behind the (fileInfo.Attributes & FileAttributes.ReparsePoint == 0) check. My internet search suggested it is related to symbolic links, etc., but I happy to learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I wanted to ensure the file both exists and is a real file (not a symbolic link) to minimize the risk of running malicious code. For example, imagine a symbolic link located in one of the directories listed in the user's PATH environment variable. The link has a name like nuget-plugin-*, making it appear like a normal plugin, but it actually points to a malicious file in a different directory. If we run that symlink, we unintentionally execute the malicious file from the other location. We could say it is up to the user to ensure there is no malicious files in the PATH folders. However, I don't see a reason why we should leave that vulnerability open.

#endif
}
}

/// <summary>
/// Checks whether a file is executable or not in Unix.
/// This is done by running bash code: `if [ -x {fileInfo.FullName} ]; then echo yes; else echo no; fi`
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
private static bool IsExecutable(FileInfo fileInfo)
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
{
#pragma warning disable CA1031 // Do not catch general exception types
try
{
string output;
using (var process = new Process())
{
// Use a shell command to check if the file is executable
process.StartInfo.FileName = "/bin/bash";
process.StartInfo.Arguments = $"if [ -x {fileInfo.FullName} ]; then echo yes; else echo no; fi";
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardOutput = true;

process.Start();
if (!process.WaitForExit(1000) || process.ExitCode != 0)
{
return false;
}

output = process.StandardOutput.ReadToEnd().Trim();
}

// Check if the output is "yes"
return output.Equals("yes", StringComparison.OrdinalIgnoreCase);
}
catch
{
return false;
}
#pragma warning restore CA1031 // Do not catch general exception types
}

private IEnumerable<string> GetPluginFilePaths()
{
if (string.IsNullOrEmpty(_rawPluginPaths))
Expand All @@ -149,7 +251,12 @@ private IEnumerable<string> GetPluginFilePaths()
// Internal plugins are only supported for .NET Framework scenarios, namely msbuild.exe
directories.Add(PluginDiscoveryUtility.GetInternalPlugins());
#endif
return PluginDiscoveryUtility.GetConventionBasedPlugins(directories);
var plugins = PluginDiscoveryUtility.GetConventionBasedPlugins(directories).ToList();

// Get plugins added using .Net Tools
plugins.AddRange(GetNetToolsPluginFiles());

return plugins;
}

return _rawPluginPaths.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries);
Expand Down
Loading