search

shell-pool / shpool

Think tmux, then aim... lower
Apache License 2.0
1.03k stars 13 forks source link

sniff shells for prompt injection #27

Closed ethanpailes closed 1 week ago

ethanpailes commented 4 weeks ago

This patch teaches shpool to discern the shell it is launching more accurately than we were doing in the past. Previously we would just look at the name of the shell binary, but it turns out this does not work well when people put exec fish in their .bashrc or something similar. There are actually some good reasons for people to do this rather than use chsh or equivalent, so we should handle this usecase.

I tried a few different approaches to this before settling on injecting a startup sentinal to the real shell, then scanning for it and sniffing based off the pid once the shell is fully started up. This ought to give the rc files enough time to get processed, and it allows us to avoid depending on an external binary like ps.

One drawback is that this won't work on non-linux, so we will need to figure out an alternative to /proc//exe for the mac port.

One might think that passing --version would work, but this won't handle execs in .rc files.

I tested this manually and the existing per-shell prompt prefix tests already cover it.

ethanpailes commented 4 weeks ago

The tests pass locally for me, but for whatever reason it seems like the fish one is not generating any output for the sniff shell in the CI environment.

I've put some debug prints in this change to try to figure out what is going on, but I'm going to take a break from trying to debug it for now.

ethanpailes commented 3 weeks ago

I've been having trouble figuring out a way to get this to work for all versions of fish.

ethanpailes commented 3 weeks ago

The final version of this patch might look pretty different, so it probably isn't worth putting too much review effort in yet. Sorry, I thought I would be able to get tests passing quicker.

ethanpailes commented 3 weeks ago

https://github.com/shell-pool/shpool/pull/28 is also running into CI-only test failures due to problems with fish. I'm starting to suspspect that the debian we are using for CI recently got a new fish version that is not playing nice for whatever reason. The issue may not be this PR.

ethanpailes commented 2 weeks ago

I got this passing, but I basically entirely re-wrote the patch. It should be ready for review now. Sorry for the delay and churn.