Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
su.c: replace getopt with ad-hoc flag processing
In preparation for supporting --exec-command I was testing the robustness of "--" handling and it became apparent that things are currently a bit broken in `su`. Since "--" is currently of limited utility, as the subsequent words are simply passed to the shell after "-c","command_string", it seems to have gone unnoticed for ages. However, with --exec-command, it's expected that "--" would be an almost required separator with every such usage, considering the following flags must be passed verbatim to execve() and will likely begin with hyphens looking indistinguishable from any other flags in lieu of shell interpolation to worry about. For some practical context of the existing situation, this invocation doesn't work today: ``` $ su --command ls -- flags for shell No passwd entry for user 'flags' $ ``` This should just run ls as root with "flags","for","shell" forwarded to the shell after "-c","ls". The "--" should block "flags" from being treated as the user. That particular issue isn't a getopt one per-se, it's arguably just a bug in su.c's implementation. It *seemed* like an easy fix for this would be to add a check if argv[optind-1] were "--" before treating argv[optind] as USER. But testing that fix revealed getopt was rearranging things when encountering "--", the "--" would always separate the handled opts from the unhandled ones. USER would become shifted to *after* "--" even when it occurred before it! If we change the command to specify the user, it works as-is: ``` $ su --command ls root -- flags for shell Password: testfile $ ``` But what's rather surprising is how that works; the argv winds up: "su","--command","ls","--","root","flags","for","shell" with optind pointing at "root". That arrangement of argv is indistinguishable from omitting the user and having "root","flags","for","shell" as the stuff after "--". This makes it non-trivial to fix the bug of omitting user treating the first word after "--" as the user, which one could argue is a potentially serious security bug if you omit the user, expect the command to run as root, and the first word after "--" is a valid user, and what follows that something valid and potentially destructive not only running in unintended form but as whatever user happened to be the first word after "--". So, it seems like something important to fix, and getopt seems to be getting in the way of fixing it properly without being more trouble than replacing getopt. In disbelief of what I was seeing getopt doing with argv here, I took a glance at the getopt source and found the following: ``` /* The special ARGV-element '--' means premature end of options. Skip it like a null option, then exchange with previous non-options as if it were an option, then skip everything else like a non-option. */ if (d->optind != argc && !strcmp (argv[d->optind], "--")) ``` I basically never use getopt personally because ages ago it annoyed me with its terrible API for what little it brought to the table, and this brings it to a whole new level of awful.
- Loading branch information