fix(barbuk): strip control characters from prompt segment output - #2397
Conversation
The python_venv segment falls back to reading pyproject.toml's requires-python field with no character restrictions, and every segment's output was concatenated into PS1 unfiltered. A crafted pyproject.toml could inject terminal escape sequences into the prompt simply by cd-ing into the directory. Strips control characters from each segment's output in __prompt-command, the single point all segments already pass through, before it's appended to PS1.
|
This is a good find, thanks. This will remove all control characters, even color. This also does not protect against non raw control character, e.g, Maybe we can sanitize with awk directly ? $ grep requires-python pyproject.toml
requires-python = ">=3.12" # Normal
requires-python = "\e]0;change title\a" # non raw control char
requires-python = "^K^?^B" # raw control char (ctrl+v backspace, ctrl+v escape)$ awk -F'"' '/^requires-python/ {gsub(/([^\11\12\15\40-\176]|\\[eE]\])/, ""); print $0}' pyproject.toml
requires-python = "0;change title\a"
requires-python = ">=3.12"
requires-python = "" |
…put" This reverts commit 0237810.
…segment The previous fix stripped control characters from every segment's rendered output in __prompt-command, which also stripped legitimate ANSI color codes (e.g. the git segment's branch/status coloring), breaking prompt coloring for every user, not just the vulnerable one. It also only handled raw control bytes, not the same escape sequence written as literal backslash-letter text (e.g. "\e]0;...\a"), which some shells/echo modes can still expand. Sanitize at the actual source instead: strip anything outside printable ASCII, plus literal backslash-letter escape sequences, directly in the awk extraction of pyproject.toml's requires-python field, the one value that's genuinely untrusted. Every other segment's output, including its own legitimate color codes, is left untouched. Thanks @BarbUk for catching both issues.
|
Thanks for the careful review, both points are right. Pushed a fix that reverts the blanket strip in python_info=$(awk -F'"' '/^requires-python/ {gsub(/[^\40-\176]|\\[a-zA-Z]/, "", $2); print $2}' pyproject.toml)This strips both raw control bytes and the same sequence written as literal backslash-letter text (e.g. Re-verified locally: git branch coloring is intact again ( |
|
Just checked why the git prompt was impact by the raw char cleaning and not the other segment. The Returning the git prompt as a string, we can sanitize using your first proposal. Sanitizing should use the control code class, it's simplier: Updated code should look like: diff --git a/themes/barbuk/barbuk.theme.bash b/themes/barbuk/barbuk.theme.bash
index e3f658f7..c79dbf36 100644
--- a/themes/barbuk/barbuk.theme.bash
+++ b/themes/barbuk/barbuk.theme.bash
@@ -106,7 +106,7 @@ function __git-upstream-remote-logo_prompt() {
function git_prompt_info() {
git_prompt_vars
- echo -e "on $SCM_GIT_CHAR_ICON_BRANCH $SCM_PREFIX$SCM_BRANCH$SCM_STATE$SCM_GIT_AHEAD$SCM_GIT_BEHIND$SCM_GIT_STASH$SCM_SUFFIX "
+ echo "on $SCM_GIT_CHAR_ICON_BRANCH $SCM_PREFIX$SCM_BRANCH$SCM_STATE$SCM_GIT_AHEAD$SCM_GIT_BEHIND$SCM_GIT_STASH$SCM_SUFFIX "
}
function __exit_prompt() {
@@ -317,6 +317,7 @@ function __prompt-command() {
for segment in $BARBUK_PROMPT; do
local info
info="$(__"${segment}"_prompt)"
+ info="${info//[[:cntrl:]]/}"
[[ -n "${info}" ]] && PS1+="${info}"
done |
… echo -e git_prompt_info used 'echo -e' to expand its own color codes, which were stored as literal backslash-escape text (e.g. lib/colors.bash's bold_red) rather than real bytes. Applying the blanket control- character strip in __prompt-command without first removing that -e flag broke coloring, since by the time the strip ran, echo -e had already turned the git segment's own colors into real bytes indistinguishable from a malicious payload. Dropping -e means git_prompt_info's own escape codes are no longer expanded from literal text at echo time (removing the injection surface that flag represented for any future untrusted input reaching this function), and re-adds the blanket [[:cntrl:]] strip at __prompt-command, this time safely, since no segment's construction path relies on echo -e turning literal text into real bytes anymore. Verified live: git branch coloring intact in a raw pipe-pane capture (same bytes as before this change), and the original PoC's injected control bytes still stripped from the python_venv segment.
|
Good catch. Applied both changes you suggested: dropped |
|
@BarbUk are you ready to give it an official green review for me to merge? |
|
LGTM |
Fixes #2396
The
python_venvsegment falls back to readingpyproject.toml'srequires-pythonfield (viaawk, no character restrictions on the value) when no active virtualenv is detected, and every segment's output was concatenated intoPS1unfiltered in__prompt-command. A craftedpyproject.tomlcould inject terminal escape sequences into the prompt simply bycd-ing into the directory, sincepython_venvis part of the defaultbarbuksegment list.Change
Strips control characters from each segment's output in
__prompt-command, the single point every segment's output already passes through before being appended toPS1, rather than only inpython_venv, so any future segment that reads file content is covered too.Testing
pyproject.tomlwhoserequires-pythonfield contained a raw OSC title-set escape sequence, confirmed via atmux pipe-panecapture (hex-dumped, not just visually inspected) that the raw escape bytes reached the terminal adjacent to the theme's own SGR color codes.$'\x00') silently matched nothing, since bash strings can't hold a literal NUL byte and it collapses out of the range at parse time. Starting at$'\x01'fixed it; caught this by testing the substitution in isolation rather than trusting it from source alone.