Prevent passing of a malicious command via DIALOGCMD env#94
Conversation
There was a problem hiding this comment.
Pull request overview
Removes user-controlled DIALOGCMD env var handling to prevent a potential privilege-escalation vector where a malicious value could be passed across the sudo boundary. The elevated re-exec no longer forwards DIALOGCMD, and the script re-detects it after elevation.
Changes:
- Unset any inherited
DIALOGCMDand only auto-detectsusedialog/dialogfromPATH. - Remove
DIALOGCMD="$DIALOGCMD"from theexec sudoinvocation so detection re-runs as root.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The failed pipeline is fine it's only TW snapshot change |
| # This prevents privilege escalation via sudo DIALOGCMD injection | ||
| unset DIALOGCMD | ||
|
|
||
| if DIALOGCMD=$(command -v susedialog 2>/dev/null); then |
There was a problem hiding this comment.
the issue is that this block should only be executed when EUID is 0. right now you run this as non-root and then make use of the evaluation result (under the attackers control) in the elavated run.
you must do this only when root in a clean environment
There was a problem hiding this comment.
Oky, I'll rework this. I'm still contemplating if it's really worth to support the legacy dialog ... maybe it would be simpler to just support susedialog also from openqa perspective ...
There was a problem hiding this comment.
Oky I think it's addressed now. I also removed fallback for legacy dialog.
| # Keep only terminal-related vars needed by dialog. | ||
| exec sudo env -i \ | ||
| PATH="/usr/sbin:/usr/bin:/sbin:/bin" \ | ||
| TERM="${TERM:-linux}" \ | ||
| COLORTERM="${COLORTERM:-}" \ |
There was a problem hiding this comment.
@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.
| unset DIALOGCMD | ||
| if DIALOGCMD=$(command -v susedialog 2>/dev/null); then |
There was a problem hiding this comment.
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)?
| # 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 |
There was a problem hiding this comment.
This one I'd actually consider. However I very often develop susedialog with this tool and run it from non /usr/bin locations ...
There was a problem hiding this comment.
Yes, I also recommend using type instead of command
| # Keep only terminal-related vars needed by dialog. | ||
| exec sudo env -i \ | ||
| PATH="/usr/sbin:/usr/bin:/sbin:/bin" \ | ||
| TERM="${TERM:-linux}" \ | ||
| COLORTERM="${COLORTERM:-}" \ |
There was a problem hiding this comment.
Here I think we might not need to preserve them working with default root variables should be fine.
Uh oh!
There was an error while loading. Please reload this page.