The security hole in sudo

http://nakedsecurity.sophos.com/2012/05/21/anatomy-of-a-security-hole-the-break-that-broke-sudo/

by casca • 14 years ago

50 75 14 years ago

nakedsecurity.sophos.com

klez4• 14 years 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_Merch2• 14 years 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• 14 years 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.

nknight2• 14 years 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.

klez4• 14 years 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...

ajross2• 14 years 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• 14 years 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.

klez4• 14 years 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• 14 years 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• 14 years 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• 14 years 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.

lubutu2• 14 years ago

I agree. In Go break is the default, and there's an explicit 'fallthrough' statement.

joshAg2• 14 years 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).

natesm2• 14 years 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.

TazeTSchnitzel4• 14 years ago

So Java and C++ programmers don't panic and think the whole thing will fall through.

pcwalton• 14 years 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.

T-hawk• 14 years ago

Makes total sense when you remember C's assembly roots. On most architectures including the PDP-11, the program counter just moves to the next instruction. To make it do anything different, you need a jump instruction, and that's what the "break" means.

To a sibling comment: C# does permit fallthrough, just with ugly syntax. You can "goto case 2". http://stackoverflow.com/questions/174155/switch-statement-f...

AlexFromBelgium• 14 years ago

Douglas Crockford said in one of his talk on Javascript (www.yuiblog.com/crockford/) that the syntax came from Fortran. And indeed it's because assembly did the fall through thing...

dexen• 14 years ago

    switch (v) {
    case FOO_A:
    case FOO_B:
      do some_foo();
      break; // if you really want :P
    case BAR_X:
    case BAR_Y:
      do_some_bar();
    }

Here you have two implicit fallthroughs used for cleaner code.

Another use is to express one case as a subset or superset of another case:

    switch (v) {
    case FOO_BAR:
       do_some_foo();
    case BAR:
       do_some_bar();
    /*...*/
    }

IIRC, there was some C-like programming language out there (C# probably?) which only provided default fallthrough if there was no code between the cases, as in the above example.

jerf• 14 years ago

You're defending against a different claim. The idea is not that C's current way of doing things has no advantages, the idea is that it's not the optimal way. Pointing out a case in which it happens to initially be more helpful is basically a no-op. And I can think of better ways to spell even what you have there. Consider a "cases FOO_A, FOO_B" construct or something that both makes explicit what you are trying to do and still retains default break behavior. Fewer characters, clearer semantics, not hard to format for easy reading. C's way is definitely not optimal even on its own terms.

jiggy20112• 14 years 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?

lubutu2• 14 years 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.

jiggy20112• 14 years 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.

TazeTSchnitzel4• 14 years ago

Windows hasn't been "administrator by default" since Vista.

uniclaude• 14 years ago

About your file edits:

-Save your modified file to a temp file and work it out with cp or mv after.

-Use Vim, and the infamous ":w !sudo tee %".

mbell• 14 years ago

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

Nano asks you for the file name/location when you save, just save the file somewhere you have write access to then move it with sudo afterwards. No need to retype your edits.

gnaritas5• 14 years ago

If you find you can't save a file because of permissions, just suspend nano with ctrl+z, fix the permission, resume suspended job with fg, save file. No need to lose your edits or hassle with temp files.

JoachimSchipper4• 14 years ago

> I've made (edited) something (...) in nano and then gone to save and got "permission denied"

sudoedit handles this exact case. Try it, it's very convenient. (Of course, you have to set at least one of SUDO_EDITOR, VISUAL or EDITOR correctly.)

You also want to know about visudo, which will save you some oh-shit-I-locked-myself-out moments.

getsat• 14 years ago

If I have to use sudo, I always:

  sudo -i
  # do root work
  exit # or ^D

kstenerud2• 14 years 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• 14 years 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.

pnathan4• 14 years 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.

JoachimSchipper4• 14 years 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• 14 years 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.

pnathan4• 14 years ago

Sure. And if you get a static analyzer, you solve the problem more or less.

But, most of the bugs I notice in the static analysis output are not possible in the model of other languages. That's... kind of depressing.

figglesonrails3• 14 years ago

Most projects don't need C, I'll agree. But I have a hard time seeing how a language that is on a 1:1 to 1:5 statement-to-machine-instruction language will be displaced by a language that is safer while being at least as efficient and at least as expressive. If you say "but I don't need a 1:1 to 1:5 ratio..." THEN YOU'RE DOING IT WRONG.

C isn't broken. Gah. Why does everyone have such a hard time understanding that? The language does //exactly what you tell it to do//. If that's overrun a buffer, then that's what you get. If it is fallthrough a switch statement, that's what you get. If it is typecast a block of memory that is too small to hold the structure, then that's what you get. In short, for many programs, that means doing stupid things at some time. For many programs, edge cases aren't considered at all. Blame C? It let me?

For every single one of the "warts" of C, there is also a usecase. This is why it will not be "fixed" and why it isn't "flawed". To remove them is to remove a useful function that could otherwise cause more machine code to be needed. THIS IS THE KEY. Minimizing machine code (overhead).

If you aren't good at managing these edge cases, don't need native code, don't need interop with other code, can take a performance hit, or don't want to use automated tools to help you, then don't use C for large programs.

Take it for what it is: a low level, mostly portable, mask over assembly. Stop complaining about C, start complaining about programmers.

danielweber• 14 years ago

When people complain about the security of C, it's usually the buffer overflows.

But this switch/case/break statement behavior exists in lots of languages. I almost expect it. (It doesn't in Ruby and I usually have to stop and think about it when geeking out in that language.)

pnathan4• 14 years ago

Bluntly put, you miss the point, which is - C could be redesigned to preserve the characteristics that make it good, but have them off by default.

Most C code isn't that critical. Say, about 97% of the time.

a3d6g2f72• 14 years ago

setuidgid is written in C. No bugs yet reported.

Plan 9 doesn't have the concept of superuser. "sudo" is moot. Plan 9 is written in C.

Just two examples that come to mind.

Maybe it's not the language. Maybe it's the person using it.

There is never going to be a perfectly "idiot-proof" language that shares the power of a language like C. But many still seem to believe this is possible.

pnathan4• 14 years ago

Better design alleviates the problem out of the gate, for a system-specific solution. But if our best solution is people gotta get smarter... man, I don't know what to say.

As someone who supports ordinary programmers, expecting smarts is not the right track. expecting average and mistakes is the right track. If your foundation is not robust by default, your systems will not be robust by default either.

To mix metaphors: Better to design foundations that keep the gloves on by default, then when you need to, take them off. You can see that design philosophy in Mercurial.

Suppose - for argument's sake - that C had two different pointer types: checked pointers for arrays and unchecked pointers for arbitrary memory and that conversions between the two were relatively seamless. What difference would that make, 97% of the time?

rmoriz2• 14 years 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/

- toft https://github.com/exceedhl/toft/

- cucumber http://cukes.info/

- aruba https://github.com/cucumber/aruba

to write/reverse engineer examples/user stories.

raverbashing• 14 years ago

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.

rmoriz2• 14 years 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• 14 years ago

Acronym expansion: ATDD - acceptance test driven development.

robertduncan• 14 years 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• 14 years 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• 14 years 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• 14 years 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.

The security hole in sudo

Related Stories