5 ms·

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 langu

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?

mattpallissard 4y ago
Doubtful, failing to sanitize your inputs plagues memory safe languages too.
- deafpolygon 4y ago
  It's kind of tiring to hear about memory safe languages (mainly rust here) being put on a pedestal, as if it will solve all our software woes. Not to mention, C++ /can/ be memory safe if you use memory-safe routines. I'm just waiting for articles to come in, this year.. "why 2023 was not the year of Rust". Not hating on Rust - just the evangelism.
dllthomas 4y ago
I don't see a change to language, per se, that would have helped, really. A system with more of an object capabilities model could have helped, though. The goal wasn't really "let the user run their editor as root (when they ask for it)", but "let the user work with this particular file from their editor (when they ask for it)".
JoshTriplett 4y ago
With my Rust hat on: I don't think that Rust would have solved this. It might have made the code in question easier to understand, as you note, but this kind of error can still happen in any language.
arp242 4y ago
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/0274a4f3b403162a37a10f199c989f3727ed3ad4 https://github.com/sudo-project/sudo/commit/0274a4f3b403162a...
- Yajirobe 4y ago
  goto statement in something as important as sudo? Seriously? Talk about bad practices.
  - arc-in-space 4y ago
    goto to a cleanup/error handling block at the end of a function is a fairly common C idiom
  - Reubensson 4y ago
    Don't really see a problem using goto there. Simplyfies control flow and error handling. Assuming we are talking about same piece of code.
  - aflag 4y ago
    I think if you read the kennel's code you're in to a big surprise.
  - jazzyjackson 4y ago
    just because something's old doesn't mean it's bad
- ilyt 4y ago
  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 4y ago
    [deleted]
- thayne 4y ago
  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.
lmm 4y ago
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.
- endgame 4y ago
  Good point. A "parse, don't validate" approach to handling the contents of `$EDITOR` would look like `parseEditorEnv :: String -> Maybe ProgramPath`, and that parse should fail.
  - dllthomas 4y ago
    But that's the wrong thing. The problem is not that EDITOR must be a program, but (apparently) that they're parsing the expansion of EDITOR (along with other stuff) to figure out what file to operate on.
endgame 4y ago
Ironically, even C has the tools to avoid this. You just need to be running OpenBSD: https://man.openbsd.org/unveil.2 https://man.openbsd.org/unveil.2
- dllthomas 4y ago
  I don't think that works here. The editor is supposed to be able to access other files.
creatonez 4y ago
This is more of a shellshock-like bug than anything complicated involving memory. Just a really stupid misconfiguration oversight.
dllthomas 4y ago
On reflection, I think C is a language where this kind of error is less likely. In languages where it's easy to cram strings together and parse the results, people are more likely to do it. Those things are a bit of a pain in C, so people are more likely to do things another (and in this case better) way. Of course, that was obviously no guarantee.

mattpallissard 4y ago

Doubtful, failing to sanitize your inputs plagues memory safe languages too.

deafpolygon 4y ago
It's kind of tiring to hear about memory safe languages (mainly rust here) being put on a pedestal, as if it will solve all our software woes. Not to mention, C++ /can/ be memory safe if you use memory-safe routines. I'm just waiting for articles to come in, this year.. "why 2023 was not the year of Rust". Not hating on Rust - just the evangelism.

dllthomas 4y ago

I don't see a change to language, per se, that would have helped, really. A system with more of an object capabilities model could have helped, though. The goal wasn't really "let the user run their editor as root (when they ask for it)", but "let the user work with this particular file from their editor (when they ask for it)".

JoshTriplett 4y ago

With my Rust hat on: I don't think that Rust would have solved this. It might have made the code in question easier to understand, as you note, but this kind of error can still happen in any language.

arp242 4y ago

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/0274a4f3b403162a37a10f199c989f3727ed3ad4 https://github.com/sudo-project/sudo/commit/0274a4f3b403162a...

Yajirobe 4y ago
goto statement in something as important as sudo? Seriously? Talk about bad practices.
- arc-in-space 4y ago
  goto to a cleanup/error handling block at the end of a function is a fairly common C idiom
- Reubensson 4y ago
  Don't really see a problem using goto there. Simplyfies control flow and error handling. Assuming we are talking about same piece of code.
- aflag 4y ago
  I think if you read the kennel's code you're in to a big surprise.
- jazzyjackson 4y ago
  just because something's old doesn't mean it's bad
ilyt 4y ago
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 4y ago
  [deleted]
thayne 4y ago
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.

lmm 4y ago

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.

endgame 4y ago
Good point. A "parse, don't validate" approach to handling the contents of `$EDITOR` would look like `parseEditorEnv :: String -> Maybe ProgramPath`, and that parse should fail.
- dllthomas 4y ago
  But that's the wrong thing. The problem is not that EDITOR must be a program, but (apparently) that they're parsing the expansion of EDITOR (along with other stuff) to figure out what file to operate on.

endgame 4y ago

Ironically, even C has the tools to avoid this. You just need to be running OpenBSD: https://man.openbsd.org/unveil.2 https://man.openbsd.org/unveil.2

dllthomas 4y ago
I don't think that works here. The editor is supposed to be able to access other files.

creatonez 4y ago

This is more of a shellshock-like bug than anything complicated involving memory. Just a really stupid misconfiguration oversight.

dllthomas 4y ago

On reflection, I think C is a language where this kind of error is less likely. In languages where it's easy to cram strings together and parse the results, people are more likely to do it. Those things are a bit of a pain in C, so people are more likely to do things another (and in this case better) way. Of course, that was obviously no guarantee.