Skip to content

Prevent passing of a malicious command via DIALOGCMD env #94

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

Open
lkocman wants to merge 3 commits into
base: main
Choose a base branch
from
+18 −13
Open

Prevent passing of a malicious command via DIALOGCMD env #94

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c20f33
Prevent passing of a malicious command via DIALOGCMD env
lkocman May 19, 2026
b038142
Drop dialog and set dialogcmd after sudo
lkocman May 20, 2026
40085ea
Merge branch 'main' into sanity
lkocman May 20, 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
31 changes: 18 additions & 13 deletions opensuse-migration-tool
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,6 @@ for tool in "${REQUIRED_TOOLS[@]}"; do
fi
done

if [[ -n "${DIALOGCMD:-}" ]] && command -v "$DIALOGCMD" >/dev/null 2>&1; then
DIALOGCMD=$(command -v "$DIALOGCMD")
elif DIALOGCMD=$(command -v susedialog 2>/dev/null); then
:
elif DIALOGCMD=$(command -v dialog 2>/dev/null); then
:
else
echo "Neither susedialog nor dialog is installed."
echo "Please run: sudo zypper install susedialog"
exit 1
fi

# Ensure Bash version is 4.0+
if ((BASH_VERSINFO[0] < 4)); then
echo "This script requires Bash 4.0 or higher." >&2
Expand Down Expand Up @@ -191,13 +179,30 @@ function check_x86_64_v2_support() {
# Elevated permissions check unless DRYRUN is set
if [ -z "${DRYRUN:-}" ]; then
if [ "$EUID" -ne 0 ]; then
exec sudo DIALOGCMD="$DIALOGCMD" "$0" "$@"
# Re-run with sudo in a clean environment.
# Keep only terminal-related vars needed by dialog.
exec sudo env -i \
PATH="/usr/sbin:/usr/bin:/sbin:/bin" \
TERM="${TERM:-linux}" \
COLORTERM="${COLORTERM:-}" \
Comment on lines +183 to +187
Copy link
Copy Markdown
Member

@dirkmueller dirkmueller May 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lkocman please explicitly source the proxy information (iirc /etc/sysconfig/proxy) ?

passing down the proxy information would make sense though. many enterprise users are behind a corporate proxy/security gateway.

Comment on lines +183 to +187
Copy link
Copy Markdown
Collaborator Author

@lkocman lkocman May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I think we might not need to preserve them working with default root variables should be fine.

"$0" "$@"
fi

# Requires elevated permissions or test will always fail
test -w / || { echo "Please run the tool inside 'transactional-update shell' on Immutable systems."; exit 1; }
fi

# Resolve susedialog only after privilege handling.
# This prevents using a value resolved in an unprivileged context in the elevated run.
unset DIALOGCMD
if DIALOGCMD=$(command -v susedialog 2>/dev/null); then
Comment on lines +197 to +198
Copy link
Copy Markdown
Member

@dirkmueller dirkmueller May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use this suggestion? I agree it would be safer, but I'm not sure why we need to probe in the first place (rather than just hardcode)?

Copy link
Copy Markdown
Collaborator Author

@lkocman lkocman May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I'd actually consider. However I very often develop susedialog with this tool and run it from non /usr/bin locations ...

Copy link
Copy Markdown
Member

@dirkmueller dirkmueller May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also recommend using type instead of command

:
else
echo "susedialog is required but not installed."
echo "Please run: sudo zypper install susedialog"
exit 1
fi


# System-specific options
if [[ "$NAME" == "openSUSE Leap Micro" ]]; then
Expand Down
Loading