Skip to content

Fix: Open With no longer expands filenames into shell code (R1) #176

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

Merged
MemerGamer merged 7 commits into from
Oct 12, 2026
Merged

Fix: Open With no longer expands filenames into shell code (R1) #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
76508cd
Prevent Open With filenames from expanding into shell code
Oct 12, 2026
328fb93
Merge remote-tracking branch 'origin/main' into linux/a5-S1
MemerGamer Oct 12, 2026
205d09f
Address review findings
Oct 12, 2026
499282c
Expand desktop Exec field codes only as separate argv entries
Oct 12, 2026
0c15de4
Merge remote-tracking branch 'origin/main' into linux/a5-S1
MemerGamer Oct 12, 2026
dff6fcd
Redirect launched applications' stdio to /dev/null
Oct 12, 2026
2d4827d
Merge remote-tracking branch 'origin/main' into linux/a5-S1
MemerGamer Oct 12, 2026
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 docs/linux-port/investigations/security-review-2026-10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Priorities: P0 critical immediate exposure; P1 command execution or significant

**Evidence:** Reproduced with `/bin/sh` and harmless `printf INJECTED`. The expanded script contained the literal `$(printf INJECTED)` filename; shell output instead contained `INJECTED`. Existing `DesktopExecInjectionTests` cover unquoted placeholders inside scripts, not this nested case. Service-menu expansion uses a separate restrictive path; this finding concerns `Expand`.

**Suggested fix:** Tokenize the desktop Exec before substituting filenames and keep targets as argv data. Reject field-code substitution into shell script arguments; support a safe explicitly defined form passing targets as positional arguments to a fixed script (`sh -c 'viewer "$1"' sh <target>`). Do not attempt to fix arbitrary nested script syntax with another escaping pass. Add nested double/single quotes, substitutions and backticks to launch tests.
**Resolution:** Parse Exec using Desktop Entry quoting rules and reject field codes embedded in or quoted within arguments. Expand codes only into separate argv entries; `%%` represents a literal percent. Remove shell/wrapper detection entirely. Exec lines from installed desktop entries and service menus are trusted configuration (the same trust model as GIO); filenames are untrusted data and are only ever passed as separate argv. A configured interpreter can intentionally interpret an argument as code; identifying or policing such trusted commands is outside this boundary. LinuxFiles must not evaluate Exec through a shell and uses `ProcessStartInfo.ArgumentList`.

### R2 — P1: Archive merge can delete/overwrite outside the extraction destination

Expand Down
265 changes: 50 additions & 215 deletions src/Files.Platform.Linux/Launching/DesktopExecExpander.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,173 +14,89 @@ namespace Files.Platform.Linux.Launching
/// </summary>
public static class DesktopExecExpander
{
private static readonly HashSet<string> Shells = ["sh", "bash", "dash", "zsh", "ksh", "fish", "ash", "csh", "tcsh"];

/// <summary>
/// Splits an Exec value into arguments using shell quoting rules (double quotes, single quotes, backslash).
/// Returns null when quotes are unbalanced.
/// Parses an Exec value after desktop-entry string unescaping. Only whole double-quoted arguments are supported.
/// </summary>
public static List<string>? Tokenize(string exec)
{
var tokens = new List<string>();
var current = new StringBuilder();
var hasToken = false;
var quote = '\0';

for (var i = 0; i < exec.Length; i++)
for (var i = 0; i < exec.Length;)
{
var c = exec[i];
if (quote == '\'')
if (exec[i] is ' ' or '\t') { i++; continue; }
var quoted = exec[i] == '"';
if (quoted) i++;
var value = new StringBuilder();
var closed = !quoted;
while (i < exec.Length)
{
if (c == '\'')
quote = '\0';
else
current.Append(c);
}
else if (quote == '"')
{
if (c == '\\' && i + 1 < exec.Length && exec[i + 1] is '"' or '`' or '$' or '\\')
current.Append(exec[++i]);
else if (c == '"')
quote = '\0';
else
current.Append(c);
}
else if (c is '"' or '\'')
{
quote = c;
hasToken = true;
}
else if (c == '\\' && i + 1 < exec.Length)
{
current.Append(exec[++i]);
hasToken = true;
}
else if (c is ' ' or '\t')
{
if (hasToken)
tokens.Add(current.ToString());
current.Clear();
hasToken = false;
}
else
{
current.Append(c);
hasToken = true;
var c = exec[i++];
if (quoted && c == '"') { closed = true; break; }
if (!quoted && c is ' ' or '\t') break;
if (quoted && c == '\\')
{
if (i == exec.Length || exec[i] is not ('"' or '`' or '$' or '\\')) return null;
c = exec[i++];
}
else if ((!quoted && "\"'\\><~|&;$*?#()`\n\r".Contains(c)) ||
(quoted && c is '$' or '`')) return null;
if (c == '%')
{
if (i == exec.Length) return null;
var code = exec[i++];
if (code != '%' && (quoted || !"fFuUickdDnNvm".Contains(code))) return null;
value.Append('%').Append(code);
}
else value.Append(c);
}
if (!closed || (quoted && i < exec.Length && exec[i] is not (' ' or '\t'))) return null;
var token = value.ToString();
for (var j = 0; j < token.Length; j++)
if (token[j] == '%' && token[++j] != '%' && token.Length != 2) return null;
tokens.Add(token);
}

if (quote != '\0')
return null;

if (hasToken)
tokens.Add(current.ToString());

return tokens;
}

/// <summary>
/// Expands the application's Exec into one or more argument vectors for the given targets (paths or URIs).
/// Field codes are substituted into the raw Exec string as shell-quoted words before tokenizing, so a value
/// stays one literal word, including inside <c>sh -c "..."</c> scripts. The first element of each vector is the program.
/// Returns an empty list when the Exec is empty or has unbalanced quotes.
/// </summary>
/// <summary>Expands field codes into separate argv entries, never command text.</summary>
public static IReadOnlyList<IReadOnlyList<string>> Expand(DesktopApplication application, IReadOnlyList<string> targets)
{
var exec = application.Exec;
if (string.IsNullOrWhiteSpace(exec))
return [];

var hasShell = (Tokenize(exec) ?? []).Any(t => Shells.Contains(System.IO.Path.GetFileName(t)));
var perTarget = HasCode(exec, 'f') || HasCode(exec, 'u');
IReadOnlyList<string?> singles = !perTarget ? new string?[] { null } : targets.Count == 0 ? new string?[] { string.Empty } : [.. targets];

// Installed desktop entries/service menus are trusted configuration, as with GIO.
// Filenames are untrusted data: substitute only separate argv entries, never shell text.
var tokens = Tokenize(application.Exec);
if (tokens is null || tokens.Count == 0 || tokens[0].Length == 0 ||
tokens[0].Contains('=') || tokens[0].Replace("%%", "", StringComparison.Ordinal).Contains('%')) return [];
if (tokens.Count(t => t is "%f" or "%F" or "%u" or "%U") > 1) return [];
var perTarget = tokens.Any(t => t is "%f" or "%u");
IReadOnlyList<string?> singles = !perTarget || targets.Count == 0 ? new string?[] { null } : [.. targets];
var result = new List<IReadOnlyList<string>>();
foreach (var single in singles)
{
var argv = Tokenize(Substitute(application, exec, targets, single, hasShell));
if (argv is null || argv.Count == 0)
return [];

result.Add(argv);
}

return result;
}

/// <summary>Service menus never invoke a shell. Only a single target may fill a long option's entire value.</summary>
public static IReadOnlyList<IReadOnlyList<string>> ExpandServiceMenu(DesktopApplication app, IReadOnlyList<string> targets)
{
// Refuse shell syntax conservatively, even if quoted. Substituted paths are not inspected as command text.
if (app.Exec.Any(c => c is '|' or '&' or ';' or '<' or '>' or '`' or '$' or '*' or '?' or '~' or '#' || char.IsControl(c))) return [];
var tokens = Tokenize(app.Exec);
if (tokens is null || tokens.Count == 0 || tokens[0].Contains('%') || tokens[0].Contains('=') ||
tokens.Any(t => Shells.Contains(System.IO.Path.GetFileName(t))) ||
(System.IO.Path.GetFileName(tokens[0]) == "env" && tokens.Any(t => t.StartsWith("-S", StringComparison.Ordinal) || t.StartsWith("--split-string", StringComparison.Ordinal)))) return [];
var targetCodes = tokens.Select(t => t is "%f" or "%F" or "%u" or "%U" ? t[1] : LongOptionTargetCode(t)).ToArray();
if (targetCodes.Count(c => c != '\0') > 1 ||
(targetCodes.Any(c => c is 'f' or 'F' or 'd' or 'D') && targets.Any(t => !ToPathOrUri(t).StartsWith('/'))) ||
(tokens.Any(t => LongOptionTargetCode(t) != '\0') && targets.Count != 1)) return [];
var single = tokens.Any(t => t is "%f" or "%u");
var result = new List<IReadOnlyList<string>>();
foreach (var target in single ? targets : new string[] { string.Empty })
{
var argv = new List<string>();
foreach (var token in tokens)
{
switch (token)
{
case "%f": argv.Add(PathArgument(target)); break;
case "%u": argv.Add(ToUri(target)); break;
case "%f": if (single is not null) argv.Add(PathArgument(single)); break;
case "%u": if (single is not null) argv.Add(ToUri(single)); break;
case "%F": argv.AddRange(targets.Select(PathArgument)); break;
case "%U": argv.AddRange(targets.Select(ToUri)); break;
case "%c": argv.Add(app.Name); break;
case "%k": argv.Add(app.DesktopFilePath); break;
case "%c": argv.Add(application.Name); break;
case "%k": argv.Add(application.DesktopFilePath); break;
case "%i":
if (!string.IsNullOrEmpty(app.IconName)) { argv.Add("--icon"); argv.Add(app.IconName); }
break;
default:
var code = LongOptionTargetCode(token);
if (code != '\0')
{
var value = code is 'u' or 'U' ? ToUri(targets[0]) : PathArgument(targets[0]);
if (code is 'd' or 'D') value = System.IO.Path.GetDirectoryName(value) ?? "/";
argv.Add(token[..^2] + value);
break;
}
var literal = new StringBuilder();
for (var i = 0; i < token.Length; i++)
{
if (token[i] != '%') literal.Append(token[i]);
else if (i + 1 < token.Length && token[++i] == '%') literal.Append('%');
else return [];
}
argv.Add(literal.ToString());
if (!string.IsNullOrEmpty(application.IconName)) { argv.Add("--icon"); argv.Add(application.IconName); }
break;
case "%d": case "%D": case "%n": case "%N": case "%v": case "%m": break;
default: argv.Add(token.Replace("%%", "%", StringComparison.Ordinal)); break;
}
}
if (argv.Count == 0 || argv.Any(a => a.Contains('\0'))) return [];
if (argv.Any(a => a.Contains('\0'))) return [];
result.Add(argv.ToArray());
}
return result;
}

// Accept only --name=%x with an ASCII option name and no surrounding value text.
private static char LongOptionTargetCode(string token)
{
var equals = token.IndexOf('=');
if (!token.StartsWith("--", StringComparison.Ordinal) || equals < 3 || equals != token.Length - 3 ||
token[equals + 1] != '%' || token[^1] is not ('f' or 'F' or 'u' or 'U' or 'd' or 'D') ||
!char.IsAsciiLetterOrDigit(token[2])) return '\0';
for (var i = 3; i < equals; i++)
if (!char.IsAsciiLetterOrDigit(token[i]) && token[i] != '-') return '\0';
return token[^1];
}

/// <summary>
/// Quotes a value as a single shell word: wrapped in single quotes with embedded quotes escaped.
/// </summary>
public static string ShellQuote(string value) => "'" + value.Replace("'", "'\\''") + "'";
/// <summary>Service menus use the same Exec grammar and argv expansion as desktop entries.</summary>
public static IReadOnlyList<IReadOnlyList<string>> ExpandServiceMenu(DesktopApplication app, IReadOnlyList<string> targets) => Expand(app, targets);

/// <summary>
/// Converts a path or file URI to a local path; other URIs are returned unchanged.
Expand All @@ -204,87 +120,6 @@ public static string ToUri(string target)
return new Uri(target).AbsoluteUri;
}

private static bool HasCode(string exec, char code)
{
for (var i = 0; i + 1 < exec.Length; i++)
{
if (exec[i] != '%')
continue;

if (exec[i + 1] == code)
return true;

i++;
}

return false;
}

private static string Substitute(DesktopApplication app, string exec, IReadOnlyList<string> targets, string? single, bool hasShell)
{
var sb = new StringBuilder();
var quote = '\0';

for (var i = 0; i < exec.Length; i++)
{
var c = exec[i];
if (c != '%' || i + 1 >= exec.Length)
{
if (quote == '\0' && c is '"' or '\'')
quote = c;
else if (c == quote)
quote = '\0';
else if (c == '\\' && quote != '\'' && i + 1 < exec.Length)
{
sb.Append(c).Append(exec[++i]);
continue;
}

sb.Append(c);
continue;
}

var code = exec[++i];
var value = code switch
{
'f' => string.IsNullOrEmpty(single) ? null : Quote(PathArgument(single), quote, hasShell),
'u' => string.IsNullOrEmpty(single) ? null : Quote(ToUri(single), quote, hasShell),
'F' => targets.Count == 0 ? null : string.Join(' ', targets.Select(t => Quote(PathArgument(t), quote, hasShell))),
'U' => targets.Count == 0 ? null : string.Join(' ', targets.Select(t => Quote(ToUri(t), quote, hasShell))),
'c' => Quote(app.Name, quote, hasShell),
'k' => Quote(app.DesktopFilePath, quote, hasShell),
'i' => string.IsNullOrEmpty(app.IconName) ? null : "--icon " + Quote(app.IconName, quote, hasShell),
_ => null,
};

if (code == '%')
sb.Append('%');
else if (value is not null)
sb.Append(value);
}

return sb.ToString();
}

/// <summary>
/// Quotes a value for insertion at the current quote context of the Exec string.
/// </summary>
private static string Quote(string value, char context, bool hasShell)
{
var quoted = ShellQuote(value);
if (context == '"' && !hasShell)
quoted = value;
return context switch
{
// Our tokenizer consumes one level of double-quote escapes, so escape for it
'"' => quoted.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("`", "\\`").Replace("$", "\\$"),

// Shells: escape the quoted word for the raw single-quoted script. Others: close the region, insert the word, reopen
'\'' => hasShell ? quoted.Replace("'", "'\\''") : "'" + quoted + "'",
_ => quoted,
};
}

/// <summary>
/// Converts a target to a local path argument; relative paths starting with '-' are prefixed so they cannot be read as options.
/// </summary>
Expand Down
Loading
Loading