Hacker News Reader

Comment by arp242 | Hacker News Reader

Looking at the patch[1], probably not. There isn't really a lot of complex string handling involved; it's basically just forgetting to forbid "--". I don't really see how any language choice could help you with this.

[1]: https://github.com/sudo-project/sudo/commit/0274a4f3b403162a...

View on HN

I think there is fundamental design mistake when EDITOR string being badly escaped causing this bug

It has one job

* read file as priviledged user * copy it to temporary file * run editor as unpriviledged user * copy the changed file back

The fact lack of escaping somehow makes sudoedit try to edit file passed in EDITOR variable is extremely shoddy coding.

It shouldn't have to forbid that. The editor and privileged files shouldn't be in the same string. It should just be appending the list of temporary files to the editor command, and running that unprivileged.

You don't even need a temporary file; opening the file directly in sudoedit then passing /dev/fd/N to the spawned edit process after dropping privileges would work (a-la capabilities). But sudoedit being implemented in terms of sudo makes it hard.

edit: apparently things are more complex and sudoedit already runs the command unprivileged; the attack is in filename expansion in sudoedit itself.

Besides the fact that that wouldn't actually have prevented this bug (which you acknowledge in your edit), /dev/fd/N is a linuxism, so wouldn't work on other unices. And has slightly different semantics than the current implementation, where your changes to the file aren't actually updated in the original privileged file until after you exit the editor.

goto statement in something as important as sudo? Seriously? Talk about bad practices.

goto to a cleanup/error handling block at the end of a function is a fairly common C idiom

Don't really see a problem using goto there. Simplyfies control flow and error handling. Assuming we are talking about same piece of code.

I think if you read the kennel's code you're in to a big surprise.

just because something's old doesn't mean it's bad

Reply on news.ycombinator.com