Fix #6013

[Byron]

Make sure paths are quoted when passing them to the shell that runs the signing command.

We run it in a shell, specifically the Git shell, on Windows so more programs are available.
On Unix it sholdn't hurt even though it doesn't have a specific purpose there.

The quoting is needed on Windows to prevent backslashes to mean escaping.

Fixes #6013 .

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
gitbutler-components	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 21, 2025 4:37pm

[vercel]

@Byron is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

[EliahKagan]

If it is ever possible for the strings being single-quoted to contain single quotes, then their single quotes should also be neutralized by replacing each original occurrence of ' with '\''.

(This works in sh, other shells behaving POSIX-compatibly, and in practice in Bourne-style shells in general. It looks like it would always work here. It is not guaranteed to work in non-Bourne-style shells. But in most non-Bourne-style shells, single-quoting is not as strong, so pasting them on the outside would be insufficient for other reasons.)

---

For quoting command when it is, in its entirety, the path to a program, it seems like this is something gitoxide might reasonably be able to do, as an opt-in feature when using gix_command::Prepare. I've opened GitoxideLabs/gitoxide#1799 about this and other aspects of ambiguity in how the command and argument strings are interpreted.

Please note that, at least as proposed, it looks like that would not cover any of the quoting done in this PR, which seems to affect only arguments. But I think existing gix_command functionality could, if desired, neutralize that by using args for everything but the command name.

(If done here, that change would reach all the way up to the let mut cmd_string line. But it should work just as well to neutralize existing quotes when passing quotes the outside of arguments or, if it is certain that no ' will ever appear, to do nothing further.)

[Byron]

Thanks for researching this. I'd also think that it would be better not to build command-strings here and instead pass everything as arguments officially. That way everything regarding escaping should be taken care of properly.
Even though it's not relevant here as the input is well-known (except for the path to the command), that would definitely make for simpler and less error-prone code.

[EliahKagan]

Even though it's not relevant here as the input is well-known (except for the path to the command)

The path to the command is the main part I would be worried about, since it might potentially be in an unusually named directory. (I don't know if Windows will ever automatically name a user profile directory with a ' in it. So I don't know how plausible the specific example I gave would be.)

[Byron]

While I am working on this, do you think that it's appropriate to forcefully change an ssh key to have usable permissions?

gitbutler/crates/gitbutler-repo/src/repository_ext.rs

Lines 444 to 450 in bc482aa

	#[cfg(unix)]
	{
	// make sure the tempfile permissions are acceptable for a private ssh key
	let mut permissions = key_storage.as_file().metadata()?.permissions();
	permissions.set_mode(0o600);
	key_storage.as_file().set_permissions(permissions)?;
	}

[EliahKagan]

I'm not sure and I think I would need to understand more about the context. 0o600 are the usual permissions for it. I can think of two possible concerns:

• Maybe someone set the permissions to something ssh won't use (probably by making them too restrictive) to prevent a key they don't want used from being used.
• Maybe someone set the permissions too broadly, so that ssh refuses to use them, and depending on their threat model--for example, if there are other users on the system--the appropriate solution may be to refrain from using (or stop using) that key and generate a new one that is known not to have leaked.

But I emphasize I haven't looked into the details of how this is being used, nor as of yet thought through the full implications of either of those scenarios.

[Byron]

Thank you! Then I will leave it as it clearly is a convenience measure. I'd think that if an attacker can create SSH keys there, it would probably also be able to set their permissions accordingly. Probably SSH also checks ownership, which isn't changed here, and which might prevent a 'key-swap' attack of sorts.

Edit: here is the adjustment: #6060

[EliahKagan]

I'd think that if an attacker can create SSH keys there, it would probably also be able to set their permissions accordingly. Probably SSH also checks ownership, which isn't changed here, and which might prevent a 'key-swap' attack of sorts.

At least when the key is in the usual location of the user's .ssh directory, I would say that if an attacker can or could previously modify it or create other files there, then the attacker can already configure ssh for the user in such a way as to cause the user big problems, separately from being able to read or change the private key file directly.