10 ms·

The security hole in sudo

50 points by casca 14y ago

klez 14y ago
The missing break has been the sort of bug that haunts me at least twice a month at my current job. We do car diagnostics and we code in C. Endless time I would not understand why a particular path was not taken, and a missing break was the answer. Now, I can see the utility of fall-thru switch construct, but this is a kind of bug that could be avoided at the syntax level. Just don't require a break and develop another syntax for fall-thru cases.
- Bro_Merch 14y ago
  @klez What exactly do you do? The reason I ask is because 50 hours a week I'm an Auto Tech. The remaining hours that I'm awake I code. This week my project has been reverse engineering a pc based scanner that I have to learn about how it interacts with OBDII / CAN in an attempt to make a super tool that gives me specific abilities. The people I deal with during day know nothing of computers and the people I deal with at night know nothing of cars. You are the first person that I have ever heard mention both in the same post.
  - tonyarkles 14y ago
    This is pretty off-topic from the OP, but I want to point out that the ISO standard for OBD-II is a fantastic reference for this kind of project. We built an OBD-II reader for our engineering capstone project, and that ISO standard was an invaluable resource for getting things to work. If there's a university nearby, you might want to go check out their engineering library and see if they have a copy. I don't recall the ISO number off the top of my head. If you're really serious about the project, I think the book only costs around $400 or so.
    - bri3d 14y ago
      Most of the "super abilities" provided by high-end scan tool / ECU flash packages work by using manufacturer-specific data sent over the CAN bus, not OBD-II itself. Some newer and higher-end cars even use proprietary crypto keyed off of the VIN number and a shared secret to protect from access by third-party tools. There's a good-sized cottage industry in breaking proprietary ECU protocols and crypto, because there's a solid niche market in selling mechanics one $20,000 "multiflash" tool to replace their ten $10000 manufacturer-provided "diagnostic systems," especially in the high-end exotic market. I was able to find enough resource online to build an ISO OBD-II scanner as my seventh-grade final project, without needing to resort to hunting down the standard. Today there are even open-source projects like http://en.wikipedia.org/wiki/OBDuino http://en.wikipedia.org/wiki/OBDuino which can provide a solid code-based understanding of the three old OBD signaling types (PWM, VPW, ISO), as well as the basic protocol.
      - Bro_Merch 14y ago
        I'll let you all get back to topic at hand. I have to say thanks though. I never imagined getting so much helpful info. The super ability I seek seems like it would be in a less secure area because even cheap hardware store tools can do it. I am trying to remove, relocate, or hide fault codes without resetting the emissions monitors. That way as a hush hush marketing ploy of my business I can truly "guarantee to pass" and depending on the solution possibly guarantee to never fail again. I hit an idea slump in my normal utility making and decided to put my otherwise wasted time to use. Thanks Again ~
        klez 14y ago
        Of all the car I worked on (mostly Mitsubishi and Ssangyong) very few of them had non-standard procedures for erasing DTCs. I don't know what you mean by 'erasing the emission monitor', unless you mean the freeze frame, which usually gets erased when you erase DTCs with the standard 0x14 request. Anyway I work exactly as you do, by reverse-engineering official tools. And I'm not very knowledgeable about cars anyway :)
- nknight 14y ago
  If you're having this kind of problem a lot, now might be an excellent time to start using a lint tool in your development process. It'll catch these things.
  - klez 14y ago
    Long story short, coding standard and tooling are very poor at my current job. I'm leaving in a week, so it won't be a problem for me anymore...
- ajross 14y ago
  Or just don't use switch. If it's a small expression, an if/else chain takes no more lines of code and is not less efficient. If it's a large expression, it might be better expressed with a function pointer table. I'm not a zealot on this stuff, and write switch statements regularly (though I do strongly prefer if chains for most stuff) but if you're really hitting this regularly I think a change in process might be in order.
  - DHowett 14y ago
    It can be slightly less efficient when (on supporting architectures) the compiler could automatically generate a jump table in place of stacked conditionals, which it cannot do for a chain of ifs.
    - ajross 14y ago
      This is a myth that won't die. Jump tables can't be branch predicted, so they're generally significantly slower unless the number of cases is very high. And it's not true that the compiler "cannot" emit a jump table for an if chain anyway. A bunch of if/else-if expressions of the form "some_var == constant" can certainly be detected (though again, it probably isn't).
  - klez 14y ago
    Unfortunately, I'm not able to decide to do this. Most of our code is machine-generated, and our boss doesn't want to touch the generator in general.
- altrego99 14y ago
  I can empathize. I've always felt that implementation of switch statement in C is a bomb waiting to explode. One of the many good decisions that went into C# is reversing how it works; in other words making it compulsory to explicitly state when you need control to fall from one case to another.
- toemetoch 14y ago
  Always close your stuff before filling it. E.g. instead of typing ... switch (uint8_t bla){ case 0: stuff; ... ... and filling everything as you go along, first type the skeleton ... switch () {case: break; default: break;} ... and then fill everything in. It's anal/pedantic but it becomes second nature really fast. Do it for everything (loops, structs, functions,...). I once lost 2 entire days of my life I kid you not hunting an if (a = b) that had to be a == b and decided to change my approach.
EvanAnderson 14y ago
I've always felt like the switch statement's "break" behavior should have been the implicit case, with a statement to trigger fallthrough if you really wanted it. I wonder what percentage of the time fallthrough is actually desired in switch statements. I would assume it's wildly in the minority.
- lubutu 14y ago
  I agree. In Go break is the default, and there's an explicit 'fallthrough' statement.
  - joshAg 14y ago
    Go was not designed to be portable assembler. break directly maps to a jump statement in assembler. fallthrough does not. Since C is designed to be portable asembler, it makes sense to include break as a keyword instead of a fallthrough keyword (and break would still be needed anyway for for loops and while loops).
- natesm 14y ago
  C# doesn't have fallthrough at all, but it won't compile without "break;". I'm really not sure what they were going for there.
  - TazeTSchnitzel 14y ago
    So Java and C++ programmers don't panic and think the whole thing will fall through.
  - pcwalton 14y ago
    You can "goto" any case label in C#, which works like fallthrough but is more flexible (since you can jump anywhere). At the same time, it avoids the refactoring hazard associated with fallthrough, in which changing the order of the switch labels can change the behavior of the code. Of the languages that have "switch" (not counting ML/Haskell-style pattern matching here), C#'s approach seems ideal.
jiggy2011 14y ago
Sudo has always felt like a bit of a blunt instrument to me. For example having to type the root password every time I want to run a vaguely administrative program or install something etc gets old fast. Not to mention the number of times I've made a bunch of edits to something like a conf file in nano and then gone to save and got "permission denied" where the only solution seems to be to exit nano and run it again (and do the edits again) with sudo. Surely it should be able to prompt me for the password at save time?
- lubutu 14y ago
  Sudo has authentication timeouts built in, so you don't have to type in your password (your password, not root's) every time you want to run an administrative program. Also, the fact that you have to sudo is one of the reasons why Unix is more secure than the Windows "administrator by default" approach. As for your last point, you're complaining about nano. It would be perfectly possible for an editor to sudo when writing to disk.
  - jiggy2011 14y ago
    This is something that seems to be the case on vi and emacs too, I have never seen a unix program which prompts me for sudo password at the moment that it needs to do a privileged operation rather than requiring running via sudo.
    - nknight 14y ago
      I have, but it was a patched version. I think there's a sense, which I generally agree with, that random applications asking for your password is something we don't want users to get used to.
    - TheCapn 14y ago
      When I vi a read-only file: "/etc/mail/aliases" [Read only] 36 lines, 1023 characters Its simply a matter of remaining keen when doing admin tasks. Sure I have to close/re-open with sudo but its prompting me up front that I won't be able to save it.
kstenerud 14y ago
Would be nice if compilers offered a warning for missing breaks in a switch statement, and a pragma to mark fallthrough: switch(something) { case 1: // Do stuff break; case 2: #pragma switch_fallthrough case 3: // Do stuff break; #pragma switch_nodefault }
- there 14y ago
  lint will warn about those cases unless they are marked in the code with /* FALLTHROUGH */. I think even if you're not using lint, such comments are good practice.
pnathan 14y ago
Yet another avoidable bug. Over time, I've come to believe C is deadly flawed, and greenfield projects should seriously investigate languages that are better designed. Trivial errors in C like dangling elses, fallthroughs in switches, and easy buffer-overruns by default say to me that C's broken, has been broken, and will continue being broken. For non-realtime apps, I would recommend pretty much anything but C; for realtime apps, I would be looking into things like SystemC or D and choose appropriately. This class of avoidable bug should not be happening in 2012.
- JoachimSchipper 14y ago
  C does have issues, but lots of people are complaining about things that are easily solved by using the proper tools. Turn on compiler warnings and/or use a lint tool. (For reference, a project I have at hand is compiled with -std=c99 -pedantic -W -Wall -Wno-sign-compare -Wno-unused-parameter -Wbad-function-cast -Wcast-align -Wchar-subscripts -Wfloat-equal -Wmissing-declarations -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow -Wstrict-prototypes -Wwrite-strings -Wundef -Werror and uses lint to check for improper conversion between types.)
  - hesselink 14y ago
    So it's not broken, only broken by default. Because having to add three lines of command line flags to get sane behavior doesn't seem like a good default to me.
    - figglesonrails 14y ago
      Truthfully, C does what you tell it to do. As it turns out, most people don't want it to do what they tell it to do, they want it to do what is in their head. Those warnings are "did you really mean what you just told me to do?". For some people, the answer is "no, what, wait!" -- those are what they are for. Not that the behavior is insane, just "less common". I don't think people fully grasp the idea that C does what you tell it to do when they first start using it. If you told C "make me a sandwich", it'd give a warning about not having enough mustard to cover your entire body and an error about not have a large enough bun type to fit you in.
rmoriz 14y ago
I still wonder why they don't do ATDD. Even when you use a statically typed language there is plenty of room for failure. I recommend using: - vagrant http://vagrantup.com/ http://vagrantup.com/ - toft https://github.com/exceedhl/toft/ https://github.com/exceedhl/toft/ - cucumber http://cukes.info/ http://cukes.info/ - aruba https://github.com/cucumber/aruba https://github.com/cucumber/aruba to write/reverse engineer examples/user stories.
- raverbashing 14y ago
  No Enough with the nonsense. TDD is great for some tools, in some situations. It's not a "solve all". And now comes "ATDD", people should start working instead on inventing acronyms. vagrant? really, REALLY? Try QEMU. Try chroot News flash, sudo is not a RoR application. Especially sudo with all the magic that goes behind executing a process as another user.
  - rmoriz 14y ago
    "Especially sudo with all the magic that goes behind executing a process as another user." Your last sentence kills your standpoint totally. Please check out the mentioned "toft". It's lxc inside vagrant and designed to test complex infrastructure code from the outside in. Nothing web related.
- dredmorbius 14y ago
  Acronym expansion: ATDD - acceptance test driven development.
robertduncan 14y ago
Why was the incredibly important caveat -- this vulnerability only affects configurations which are using netmask-based permissions -- left to the last paragraph?
m_for_monkey 14y ago
This snippet is ugly anyway. The case-bodies are not indented, the one-liner if-bodies are missing their brackets, asking for more trouble.
vampirechicken 14y ago
C is fine. C is C. If you want a better C, create one. You could name it something clever like C+=1, or C++.
joeld42 14y ago
This is why I always put a "fall through" comment in place of break if I intend to omit it, and always put a blank line after each case. Once I got in the habit of this, it makes spotting this bug pretty easy.

50 points by casca 14y ago

klez 14y ago

The missing break has been the sort of bug that haunts me at least twice a month at my current job. We do car diagnostics and we code in C. Endless time I would not understand why a particular path was not taken, and a missing break was the answer. Now, I can see the utility of fall-thru switch construct, but this is a kind of bug that could be avoided at the syntax level. Just don't require a break and develop another syntax for fall-thru cases.

Bro_Merch 14y ago
@klez What exactly do you do? The reason I ask is because 50 hours a week I'm an Auto Tech. The remaining hours that I'm awake I code. This week my project has been reverse engineering a pc based scanner that I have to learn about how it interacts with OBDII / CAN in an attempt to make a super tool that gives me specific abilities. The people I deal with during day know nothing of computers and the people I deal with at night know nothing of cars. You are the first person that I have ever heard mention both in the same post.
- tonyarkles 14y ago
  This is pretty off-topic from the OP, but I want to point out that the ISO standard for OBD-II is a fantastic reference for this kind of project. We built an OBD-II reader for our engineering capstone project, and that ISO standard was an invaluable resource for getting things to work. If there's a university nearby, you might want to go check out their engineering library and see if they have a copy. I don't recall the ISO number off the top of my head. If you're really serious about the project, I think the book only costs around $400 or so.
  - bri3d 14y ago
    Most of the "super abilities" provided by high-end scan tool / ECU flash packages work by using manufacturer-specific data sent over the CAN bus, not OBD-II itself. Some newer and higher-end cars even use proprietary crypto keyed off of the VIN number and a shared secret to protect from access by third-party tools. There's a good-sized cottage industry in breaking proprietary ECU protocols and crypto, because there's a solid niche market in selling mechanics one $20,000 "multiflash" tool to replace their ten $10000 manufacturer-provided "diagnostic systems," especially in the high-end exotic market. I was able to find enough resource online to build an ISO OBD-II scanner as my seventh-grade final project, without needing to resort to hunting down the standard. Today there are even open-source projects like http://en.wikipedia.org/wiki/OBDuino http://en.wikipedia.org/wiki/OBDuino which can provide a solid code-based understanding of the three old OBD signaling types (PWM, VPW, ISO), as well as the basic protocol.
    - Bro_Merch 14y ago
      I'll let you all get back to topic at hand. I have to say thanks though. I never imagined getting so much helpful info. The super ability I seek seems like it would be in a less secure area because even cheap hardware store tools can do it. I am trying to remove, relocate, or hide fault codes without resetting the emissions monitors. That way as a hush hush marketing ploy of my business I can truly "guarantee to pass" and depending on the solution possibly guarantee to never fail again. I hit an idea slump in my normal utility making and decided to put my otherwise wasted time to use. Thanks Again ~
      - klez 14y ago
        Of all the car I worked on (mostly Mitsubishi and Ssangyong) very few of them had non-standard procedures for erasing DTCs. I don't know what you mean by 'erasing the emission monitor', unless you mean the freeze frame, which usually gets erased when you erase DTCs with the standard 0x14 request. Anyway I work exactly as you do, by reverse-engineering official tools. And I'm not very knowledgeable about cars anyway :)
nknight 14y ago
If you're having this kind of problem a lot, now might be an excellent time to start using a lint tool in your development process. It'll catch these things.
- klez 14y ago
  Long story short, coding standard and tooling are very poor at my current job. I'm leaving in a week, so it won't be a problem for me anymore...
ajross 14y ago
Or just don't use switch. If it's a small expression, an if/else chain takes no more lines of code and is not less efficient. If it's a large expression, it might be better expressed with a function pointer table. I'm not a zealot on this stuff, and write switch statements regularly (though I do strongly prefer if chains for most stuff) but if you're really hitting this regularly I think a change in process might be in order.
- DHowett 14y ago
  It can be slightly less efficient when (on supporting architectures) the compiler could automatically generate a jump table in place of stacked conditionals, which it cannot do for a chain of ifs.
  - ajross 14y ago
    This is a myth that won't die. Jump tables can't be branch predicted, so they're generally significantly slower unless the number of cases is very high. And it's not true that the compiler "cannot" emit a jump table for an if chain anyway. A bunch of if/else-if expressions of the form "some_var == constant" can certainly be detected (though again, it probably isn't).
- klez 14y ago
  Unfortunately, I'm not able to decide to do this. Most of our code is machine-generated, and our boss doesn't want to touch the generator in general.
altrego99 14y ago
I can empathize. I've always felt that implementation of switch statement in C is a bomb waiting to explode. One of the many good decisions that went into C# is reversing how it works; in other words making it compulsory to explicitly state when you need control to fall from one case to another.
toemetoch 14y ago
Always close your stuff before filling it. E.g. instead of typing ... switch (uint8_t bla){ case 0: stuff; ... ... and filling everything as you go along, first type the skeleton ... switch () {case: break; default: break;} ... and then fill everything in. It's anal/pedantic but it becomes second nature really fast. Do it for everything (loops, structs, functions,...). I once lost 2 entire days of my life I kid you not hunting an if (a = b) that had to be a == b and decided to change my approach.

EvanAnderson 14y ago

I've always felt like the switch statement's "break" behavior should have been the implicit case, with a statement to trigger fallthrough if you really wanted it. I wonder what percentage of the time fallthrough is actually desired in switch statements. I would assume it's wildly in the minority.

lubutu 14y ago
I agree. In Go break is the default, and there's an explicit 'fallthrough' statement.
- joshAg 14y ago
  Go was not designed to be portable assembler. break directly maps to a jump statement in assembler. fallthrough does not. Since C is designed to be portable asembler, it makes sense to include break as a keyword instead of a fallthrough keyword (and break would still be needed anyway for for loops and while loops).
natesm 14y ago
C# doesn't have fallthrough at all, but it won't compile without "break;". I'm really not sure what they were going for there.
- TazeTSchnitzel 14y ago
  So Java and C++ programmers don't panic and think the whole thing will fall through.
- pcwalton 14y ago
  You can "goto" any case label in C#, which works like fallthrough but is more flexible (since you can jump anywhere). At the same time, it avoids the refactoring hazard associated with fallthrough, in which changing the order of the switch labels can change the behavior of the code. Of the languages that have "switch" (not counting ML/Haskell-style pattern matching here), C#'s approach seems ideal.

jiggy2011 14y ago

Sudo has always felt like a bit of a blunt instrument to me. For example having to type the root password every time I want to run a vaguely administrative program or install something etc gets old fast. Not to mention the number of times I've made a bunch of edits to something like a conf file in nano and then gone to save and got "permission denied" where the only solution seems to be to exit nano and run it again (and do the edits again) with sudo. Surely it should be able to prompt me for the password at save time?

lubutu 14y ago
Sudo has authentication timeouts built in, so you don't have to type in your password (your password, not root's) every time you want to run an administrative program. Also, the fact that you have to sudo is one of the reasons why Unix is more secure than the Windows "administrator by default" approach. As for your last point, you're complaining about nano. It would be perfectly possible for an editor to sudo when writing to disk.
- jiggy2011 14y ago
  This is something that seems to be the case on vi and emacs too, I have never seen a unix program which prompts me for sudo password at the moment that it needs to do a privileged operation rather than requiring running via sudo.
  - nknight 14y ago
    I have, but it was a patched version. I think there's a sense, which I generally agree with, that random applications asking for your password is something we don't want users to get used to.
  - TheCapn 14y ago
    When I vi a read-only file: "/etc/mail/aliases" [Read only] 36 lines, 1023 characters Its simply a matter of remaining keen when doing admin tasks. Sure I have to close/re-open with sudo but its prompting me up front that I won't be able to save it.

kstenerud 14y ago

Would be nice if compilers offered a warning for missing breaks in a switch statement, and a pragma to mark fallthrough: switch(something) { case 1: // Do stuff break; case 2: #pragma switch_fallthrough case 3: // Do stuff break; #pragma switch_nodefault }

there 14y ago
lint will warn about those cases unless they are marked in the code with /* FALLTHROUGH */. I think even if you're not using lint, such comments are good practice.

pnathan 14y ago

Yet another avoidable bug. Over time, I've come to believe C is deadly flawed, and greenfield projects should seriously investigate languages that are better designed. Trivial errors in C like dangling elses, fallthroughs in switches, and easy buffer-overruns by default say to me that C's broken, has been broken, and will continue being broken. For non-realtime apps, I would recommend pretty much anything but C; for realtime apps, I would be looking into things like SystemC or D and choose appropriately. This class of avoidable bug should not be happening in 2012.

JoachimSchipper 14y ago
C does have issues, but lots of people are complaining about things that are easily solved by using the proper tools. Turn on compiler warnings and/or use a lint tool. (For reference, a project I have at hand is compiled with -std=c99 -pedantic -W -Wall -Wno-sign-compare -Wno-unused-parameter -Wbad-function-cast -Wcast-align -Wchar-subscripts -Wfloat-equal -Wmissing-declarations -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow -Wstrict-prototypes -Wwrite-strings -Wundef -Werror and uses lint to check for improper conversion between types.)
- hesselink 14y ago
  So it's not broken, only broken by default. Because having to add three lines of command line flags to get sane behavior doesn't seem like a good default to me.
  - figglesonrails 14y ago
    Truthfully, C does what you tell it to do. As it turns out, most people don't want it to do what they tell it to do, they want it to do what is in their head. Those warnings are "did you really mean what you just told me to do?". For some people, the answer is "no, what, wait!" -- those are what they are for. Not that the behavior is insane, just "less common". I don't think people fully grasp the idea that C does what you tell it to do when they first start using it. If you told C "make me a sandwich", it'd give a warning about not having enough mustard to cover your entire body and an error about not have a large enough bun type to fit you in.

rmoriz 14y ago

I still wonder why they don't do ATDD. Even when you use a statically typed language there is plenty of room for failure. I recommend using: - vagrant http://vagrantup.com/ http://vagrantup.com/ - toft https://github.com/exceedhl/toft/ https://github.com/exceedhl/toft/ - cucumber http://cukes.info/ http://cukes.info/ - aruba https://github.com/cucumber/aruba https://github.com/cucumber/aruba to write/reverse engineer examples/user stories.

raverbashing 14y ago
No Enough with the nonsense. TDD is great for some tools, in some situations. It's not a "solve all". And now comes "ATDD", people should start working instead on inventing acronyms. vagrant? really, REALLY? Try QEMU. Try chroot News flash, sudo is not a RoR application. Especially sudo with all the magic that goes behind executing a process as another user.
- rmoriz 14y ago
  "Especially sudo with all the magic that goes behind executing a process as another user." Your last sentence kills your standpoint totally. Please check out the mentioned "toft". It's lxc inside vagrant and designed to test complex infrastructure code from the outside in. Nothing web related.
dredmorbius 14y ago
Acronym expansion: ATDD - acceptance test driven development.

robertduncan 14y ago

Why was the incredibly important caveat -- this vulnerability only affects configurations which are using netmask-based permissions -- left to the last paragraph?

m_for_monkey 14y ago

This snippet is ugly anyway. The case-bodies are not indented, the one-liner if-bodies are missing their brackets, asking for more trouble.

vampirechicken 14y ago

C is fine. C is C. If you want a better C, create one. You could name it something clever like C+=1, or C++.

joeld42 14y ago

This is why I always put a "fall through" comment in place of break if I intend to omit it, and always put a blank line after each case. Once I got in the habit of this, it makes spotting this bug pretty easy.