Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Sudoedit can edit arbitrary files

seclists.org

Post reply on HN
Print view View original

21–30 of 61 posts

Re: Sudoedit can edit arbitrary files

#21

Post by Yajirobe »

arp242 wrote: post #13
stabbles wrote: post #5

I wonder if this bug in logic (instead of buffer overflows) would also have been less likely in a different language. Would it have been more obvious in a language where it's easier to work with dynamically allocated arrays and strings?

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...

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

Re: Sudoedit can edit arbitrary files

#22

Post by arc-in-space »

Yajirobe wrote: post #21
arp242 wrote: post #13

Earlier quoted context omitted.

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...

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

Re: Sudoedit can edit arbitrary files

#23

Post by Reubensson »

Yajirobe wrote: post #21
arp242 wrote: post #13

Earlier quoted context omitted.

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...

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

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

Re: Sudoedit can edit arbitrary files

#24

Post by lmm »

stabbles wrote: post #5

I wonder if this bug in logic (instead of buffer overflows) would also have been less likely in a different language. Would it have been more obvious in a language where it's easier to work with dynamically allocated arrays and strings?

I would say a more type-oriented mentality would make this kind of bug less likely; thinking of -- as a magic value rather than a different kind of thing from a regular argument makes it easy to forget the distinction, and a mentality where you're "sanitizing a string" is far less reliable than one where you're transforming between two distinct formats.

Re: Sudoedit can edit arbitrary files

#25

Post by tinus_hn »

DSMan195276 wrote: post #20
tinus_hn wrote: post #16

Does this really work? The command is supposed to copy the original file to a temporary file, run the edit command with the privileges of the original user and then copy the edited file over the original. Otherwise what’s stopping an attacker from telling the editor to just open another file?

Yeah I had the same confusion, the linked PDF explains it. Basically sudo determines the list of files to edit after expanding the `EDITOR` variable into separate arguments, and the `--` in the argument list (added by `sudo`) is used to determine where the file arguments provided to `sudoedit` start in the new argument list. By adding your own `--` in the `EDITOR` variable, `sudo` gets confused and thinks that `--` i…

Incredible! So the problem is not -- but the problem is that it is checking the wrong thing to begin with. Why even parse the string, sudo already had the list of files when it constructed the string..

Re: Sudoedit can edit arbitrary files

#27

Post by mananaysiempre »

eklitzke wrote: post #18
michalsustr wrote: post #14

Why would one prefer to add sudoedit X to sudoers rather than updating file access privileges of X directly? Just curious about arguments for this use case.

Sudo basically has an ACL-like system where you can specify exactly which users/groups can execute which commands as root. So you can say user foo can execute commands X, Y, and Z as root and user bar can execute commands W, Y, and Z as root, and neither user can use sudo to execute any other command as root. The ACL system isn't for sudoedit specifically, it's a general feature of sudo. As to why you can't just upda…

I recently got to reading the POSIX.1e (MAC & DAC) draft, and the DAC = ACL part is... surprisingly non-terrible. Still awkward and hampered by its existence as barely-visible metadata smeared over the whole system, as all ACLs are, but not at all the hopeless mess I expected coming from NT. (Even that might’ve been salvageable had Microsoft been willing to publish full documentation of all NT object permissions and mechanisms. Except SDDL, there is no world in which SDDL is salvageable.) Couldn’t make heads or tails of the MAC part, though.

The /etc/sudoers solution does have a usability advantage precisely in not being smeared all over the system. Even if “/etc/sudoers” and “usability” are words not often seen inside a single sentence.

Re: Sudoedit can edit arbitrary files

#28

Post by DSMan195276 »

tinus_hn wrote: post #25
DSMan195276 wrote: post #20

Earlier quoted context omitted.

Yeah I had the same confusion, the linked PDF explains it. Basically sudo determines the list of files to edit after expanding the `EDITOR` variable into separate arguments, and the `--` in the argument list (added by `sudo`) is used to determine where the file arguments provided to `sudoedit` start in the new argument list. By adding your own `--` in the `EDITOR` variable, `sudo` gets confused and thinks that `--` i…

Incredible! So the problem is not -- but the problem is that it is checking the wrong thing to begin with. Why even parse the string, sudo already had the list of files when it constructed the string..

I mean I agree, I'd say it's mostly just an issue of too much separation, they put the argument array together in one piece of code and then pass it to another piece of code that executes it with the necessary permissions. They don't pass along a separate array of files (or the location in the arguments where the files start), so the execute code attempts to figure out where they are instead.

Re: Sudoedit can edit arbitrary files

#29

Post by ilyt »

arp242 wrote: post #13
stabbles wrote: post #5

I wonder if this bug in logic (instead of buffer overflows) would also have been less likely in a different language. Would it have been more obvious in a language where it's easier to work with dynamically allocated arrays and strings?

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...

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.

Re: Sudoedit can edit arbitrary files

#30

Post by [deleted] »

ilyt wrote: post #29
arp242 wrote: post #13

Earlier quoted context omitted.

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...

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.

[deleted]
Post reply on HN