fix: Update init to use full path to starship executable

[bbigras]

Description

• get the current exe path with env::current_exe() (which might cause security problems)
• put the path in doublequotes to support spaces in the path
• escape doublequotes in the path

I didn't want to touch the big BASH_INIT, ZSH_INIT.. variable so I renamed the starship to STARSHIP_EXEC and used a replace. I wonder if it's less optimal than using the format macro or something else.

Motivation and Context

Closes #220

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

• I have tested using MacOS
• I have tested using Linux
• I have tested using Windows

Checklist:

• I have updated the documentation accordingly.
• I have updated the tests accordingly.

[bbigras]

shellcheck whines about this one but not .replace("\"", "\"'\"'\"").

[chipbuster]

Unfortunately, I think I see an attack path here which can be exploited. It's similar to the pulseaudio one, except that our binary isn't suid, so you can't automatically gain root privileges, but it could still lead to nasty stuff.

EDIT: After more discussion, I'm pretty sure I'm wrong, but I'll leave this up here for the future. The error below is in step 5: there's no way to switch the link on-the-fly. The problem in PA arises because of SUID, not because the link is switched during execution.

Just for an illustration, let's say that the user being attacked isn't root, but has some juicy information (e.g. passwords) stored in a directory in their HOME. The attack looks like this:

1. The attacker has regular user access on the target machine, but doesn't have access to the files.
2. The attacker links starship to a location of their choosing.
3. The victim starts up a shell.
4. The shell executes eval $(starship init bash).
5. Because of the link, the current_exe() resolves to the starship under the attacker's control.
6. The attacker replaces their copy of starship with a binary that prints chmod -R 777 ${HOME}
7. The shell evaluates source <( attacker_executable ) which runs chmod -R 777 ${HOME}.
8. Attacker waltzes into victim's home directory and steals all their bitcoinpasswords.

Note that as long as the attacker can replace the executable between steps 4 and 7, they can trick the victim into executing anything. If the victim is root, they can drop an executable that prints usermod -a -G attacker_account sudo and they'll get root.

Even if they miss their window between (4) and (7), the hardlink is going to persist, and the attacker's executable will now be executed by the victim's shell every time they draw a prompt (which is a fairly common operation). They can change the executable to read the victim's files, vandalize them, etc. etc.

Granted I don't 100% understand the pulseaudio attack (in particular, this seems like a common-enough operation that most "user-friendly" linuxes should provide some sort of protection against it), but if that attack works, I think this one will too.

If there are any security folk out there that wanna come in and tell me I've horribly misunderstood how this works, I'd be happy to hear it...

[chipbuster]

Suggesting lots of small changes, but overall I think this is very solid work. You've managed to make string replace look elegant, which I still can't ever seem to do.

The big two things for me are moving off of current_exe and making sure that the STARSHIP_EXEC in the init scripts can't be mistaken for a shell variable.

[bbigras]

I wonder why direnv (a popular Go project) doesn't seem to care about it).

https://github.com/direnv/direnv/blob/54cb3c5a91edb377b4b7387e2cda43dc39064a4a/config.go#L54-L61

https://github.com/direnv/direnv/blob/9c4aba849ba4ffe841bba57b05237dc97cfc9fda/shell_bash.go#L9-L18

https://golang.org/pkg/os/#Executable

[chipbuster]

I wonder why direnv (a popular Go project) doesn't seem to care about it).

Looking at the source of executable (which is the function underlying os.Executable), it looks like they do canonicalization of argv[0].

Rust's current_exe uses (at least on linux) the /proc/self/exe mechanism which is apparently prone to spoofing.

[chipbuster]

Looks great!

[bbigras]

    // let starship = get_starship_path()?.replace("\"", "\"'\"'\"");
    let starship = get_starship_path()?.replace("\"", "\"\\\"\"");

@chipbuster it's me or I I saw you comment on those lines but I can't find it in all the resolved conversations.

I'm thinking of using the first line to prevent shellcheck from whining.

[chipbuster]

@bbigras I'm convinced that GitHub ate something on this thread last night, but it's possible I just fumbled the keystrokes .

What's shellcheck complaining about? Some of the message it throws out are for bad practices in handwritten shell scripts, but not necessarily in autogenerated ones.

(I think I did have a comment about potentially using raw-strings instead of escapes all over the place, but I think it's a sidegrade at best, as you end up trading "mentally parsing backslashes" for "trying to figure out how many double/single quotes there are here")

[bbigras]

with replace("\"", "\"\\\"\""):

In 1.sh line 2:
source /dev/stdin <<<"$("/home/bbigras2/a 'te"\""st/starship" init bash --print-full-init)"
       ^--------^ SC1091: Not following: /dev/stdin was not specified as input (see shellcheck -x).
                                              ^-- SC2140: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A\"B\"C"?

with replace("\"", "\"'\"'\"") it's fine.

[chipbuster]

Looks like a shellcheck misfire to me (where the word B is \"), but if the other form works and keeps the linters happy, let's keep it!

[chipbuster]

@all-contributors, let's add @bbigras for the excellent code. Please and thanks!

[allcontributors]

@chipbuster

I've put up a pull request to add @bbigras! 🎉

[bbigras]

Thanks for the merge and the all-contributors PR @chipbuster.