Saturday, September 27, 2014

The shockingly obsolete code of bash

One of the problems with bash is that it's simply obsolete code. We have modern objective standards about code quality, and bash doesn't meet those standards. In this post, I'm going to review the code, starting with the function that is at the heart of the #shellshock bug, initialize_shell_variables().

K&R function headers


The code uses the K&R function headers which have been obsolete since the mid-1980s.


I don't think it's there to support older compilers, because other parts of the code use modern headers. I think it's there simply because they are paranoid about making unnecessary changes to the code. The effect of this is that it messes up static analysis, both simple compiler warnings as well as advanced security analysis tools.

It's also a stylistic issue. There's only one rule to coding style, which is "avoid surprising things", and this is surprising.

Ultimately, this isn't much of an issue, but a symptom that there is something seriously wrong with this code.

Global variables everywhere


Global variables are bad. Your program should have a maximum of five, for such things as the global debug or logging flag. Bash has hundred(s) of global variables.


Also note that a large number of these globals are defined in the local file, rather than including a common definition from an include file. This is really bad.

Another way of looking at the problem is looking at the functions that operate on global variables. In such cases, the functions have no parameters, as in the following:



Functions with no parameters (void) and no return should be a an extreme rarity in C. It means the function is operating on global variables, or is producing some side effect. Since you should avoid globals and side effects, such functions should virtually never exist. In Bash, such functions are pervasive.

Lol, wat?


The the first step in this function is to initialize the "environmental variables" (the ones that get executed causing the #shellshock vuln), so the first code is a for loop "for all variables". This loop contains a really weird syntax:


This is painful for so many reasons, the most noteworthy of which is that instead of incrementing the index in the third clause of the for loop, that clause is empty and instead the programmer does it in the second clause. In other words, it should look like this:


(Astute readers will note that this change isn't precisely identical, but since 'string_index' is used nowhere else, either inside or after the loop, that slight change in meaning is moot).

There is really no excuse for this sort of programming. In terms of compiler optimizations, it makes no difference in performance. All it does is confuse programmers later who are trying to read your spaghetti code. To be fair, we all have brain farts where we do something weird like this -- but it seems like oddities like this are rather common in bash.

I suspect the reason the programmer did this was because they line was getting rather long, and short lines are easier to read. But the fault here is poor choice of variable names. There is no need to call the index variable 'string_index' since it's used nowhere else except on line 329. In such cases, the variable 'i' is far superior. It communicates to the reader that you are doing the expected thing of simply enumerating 'env[]', and that the index variable is unimportant except as to index things. This is really more of a stylistic issue and isn't terribly important, but I use it to hammer home the point that the less surprising the code, the better. The code should've looked like this:


Finally, the middle clause needs some work. The expected operation here is a comparison not an assignment. This will cause static analyzers to throw up nasty warning messages. I suppose you could call this a "false positive", since the code means to do an assignment here, but here's the rule of modern C programming: you write to make static analyzers happy. Therefore, the code needs to be changed to:


The lesson here is that enumerating over a NULL-terminates list of strings is a damn common pattern in C. The way you do it should look like the same way that everybody does it, and the above snippet code is the pattern that most everyone uses. When you don't do the expected, you confuse everyone, from code reviewers to static analyzers.

No banned functions


Today, we know not to use dangerous functions like strcpy(). strncpy(), sprintf(), and so forth. While these functions can be used safely by careful programmers, it's simply better to ban their use altogether. If you use strcpy(), the code reviewer has to check each and every instance to make sure you've used it safely. If you've used memcpy(), they don't.

The bash code uses these dangerous functions everywhere, as in the following lines:


I've included enough lines to demonstrate that their use of strcpy() is safe (char_index is the length of the name string). But that this particular instance is safe isn't the issue -- the issue is that it's really difficult for code reviewers to verify that it's safe. Simply using the safer 'snprintf()' would've been much easier to verify:


One thing to remember when doing code is that writing it to make it clear to security-reviewers tends to have the effect of making code clearer for everyone else as well. I think the above use of snprintf() is much clearer than strcpy() -- as well as being dramatically safer.

Conclusion


In response to #shellshock, Richard Stallman said the bug was just a "blip". It's not, it's a "blimp" -- a huge nasty spot on the radar warning of big things to come. Three more related bugs have been found, and there are likely more to be found later. The cause isn't that a programmer made a mistake, but that there is a systematic failure in the code -- it's obsolete, having been written to the standards of 1984 rather than 2014.

Where to go from here


So now that we know what's wrong, how do we fix it? The answer is to clean up the technical debt, to go through the code and make systematic changes to bring it up to 2014 standards.

This will fix a lot of bugs, but it will break existing shell-scripts that depend upon those bugs. That's not a problem -- that's what upping the major version number is for. The philosophy of the Linux kernel is a good one to emulate: documented functionality describing how the kernel should behave will be maintained until Linus's death. Undocumented, undefined behavior that stupid programs depend upon won't be maintained. Indeed, that's one of the conflicts between Gnu and Linux: Gnu projects sometimes change documented behavior while at the same time maintaining bug-compatibility.

Bash isn't crypto, but hopefully somebody will take it on as a project, like the LibreSSL cleanup effort.

17 comments:

Richard Blaine said...

Good article

I have a personal preference for not using 'i' as an index - mostly because it's hard to search for, I tend to use 'ii' or 'jj'

I've been out of coding for a while and need to get back up to speed, is there a book/site you'd recommend for secure programming practices?

Thanks,

eloj said...

It's a simple induction variable, there should never be a reason to search for it.

As for the string concatenation I say go one step further and use asprintf() or similar (perhaps wrapped to abort if the allocation fails). Platforms which doesn't have that family of functions should probably add them.

(As for memcpy I disagree that reviewers would be able to glance over it; I can't tell you the number of times someone's used sizeof(t) instead of sizeof(*t) for instance -- which is why I prefer to use the sizeof(struct T) instead of the dereference form)

Robert Graham said...

I was kinda stretching a big criticizing the name of the variable, because the real issue is that the pattern should look like a standard enumeration, but they did weird things to make it look like something else -- particulary weird things that break static analyzers.

Mike Keen said...

There is no need to call the index variable 'string_index' since it's used nowhere else except on line 329. In such cases, the variable 'i' is far superior.

Couldn't disagree more. I like to use descriptive names for my loop counters to describe what it is an index to. If you're going to criticize a project as old as Bash, try and maintain a little bit of credibility by not attempting to impose your personal preferences on us as "best practices".

Numknacker said...

Considering this is a C program, the choice of `i` as an array index is probably the one of least surprise, especially as this index variable is used quite locally, so just the usage of `i` or `string_index` is explained by its usage. I would argue that `string_index` is actually not much more specific than `i` in this case, because `i` is conventionally used for indices.


However, I let us not miss the point here, code should see some refactoring, especially to cope with advances in tooling, platforms and requirements to sftware that change over time. There are projects that have a hard time getting and accepting contributions. Like vim (which has lead to the neovim fork), the GNU project is quite hesitant to accept patches/pull requests. In times of Github, the GNU project is nowadays more a dinosaur stuck in the old ways, or use the cathedral/bazar analogy it is petrifying into a cathedral.

I would love to see GNU prosper receiving merge requests for all these issues mentioned and more. Yet to my knowledge to become a GNU contributor involves signing lots of paperwork. Nothing that you really want to do when contributing to open source in your spare time.

João Costa said...

Common Robert, strlen(), really?
:P

Michael Campbell said...

> Couldn't disagree more. I like to use descriptive names for my loop counters to describe what it is an index to.

That's because you resort to your personal prejudices rather than well established practices. 'i' is used NOWHERE ELSE, and is one of the oldest idiomatic constructs out there - the point is make things look like the thing they are. With i being an index in a loop, used ONLY as that, and incremented where EVERYONE ELSE IN THE WORLD does it, it looks like an enumeration over that array. Using your "C in 21 days" book variable names makes it no harder for static analysis, but still a little bit surprising for the reader. The reader now has some cognitive load (albeit small) remembering that you used something out of the ordinary, and that construct now becomes a bag of inter-related things, rather than one main idea.

> ... try and maintain a little bit of credibility by not attempting to impose your personal preferences on us as "best practices".

Pot, meet kettle.

Mark Mullin said...

Having had an ATT Unix kernel license in the '80s - the for loop syntax where the increment is combined with the access is quite common in some places - code efficiency was much higher by not having two separate places where the index was accessed - that said, that was then, and this is now - now it needs to be rewritten from scratch, not refactored - it's on the wrong side of the optimization stage

Richard Blaine said...

Ah, it brings back memories, programmers can be such an arrogant bunch (yeah I include myself in that).

It might be more useful to simply allow that everyone has personal preferences and that we don't all work the same way. If the name is not misleading, it's not a worth beating to death. Yeah I'd prefer everyone did it MY way but it's not going to happen... Honestly, it's probably better because I may still have some habits that I've carried since the early 70's - and if you've never seen code from the early 70's/80's - Let's just say coding practices were somewhat less well thought out way back when...

There are pro's and con's to starting from scratch. If you're going to be touching every function anyway a fresh start might be the better way to go.

like2movejitmovejit said...

Check out Shill when it comes out, it's a shell written in Racket with fine grain security controls. Or use OpenBSD's shell which has been carefully audited unlike Bash.

Also blame all those idiots who wrote /bin/bash scripts instead of calling Dash/Ash/KSH

Richard Parratt said...

If you break the time-honoured interfaces, you'll cause bugs in scripts that should have been patched, but haven't.
That may in turn create their own security vulnerabilities.
Much of **nix has stuff that is very antiquated - try doing low level event based coding, for instance. The trouble is we're kinda stuck in 1984, it seems. (For the new stuff. Much of the conceptual basis is 1978 era)

Gustavo said...

“It might be more useful to simply allow that everyone has personal preferences and that we don't all work the same way.”

This sounds great, and certainly will get you a medal amongst youth libertarians.

However, as far as I know, it's been a long time since open source expanded from the niches of libertarians and grew up.

Now, we all, open source fans, get happy when we know that, for example, a government decides to switch all their closed source software in favor of open source.

But now we're no longer in the reign of idealism (at least, not only), but of very concrete – some would say “serious”, but this is a very ugly word, isn't it? - processes dealing with real people that are suddenly affected by the quality of the systems that intend to manage some process that they depend upon.

I hope that idealism can co-exist with quality, and quality points to standards when you have computational environments bigger than daddy's home.

Great article.

Joe Smith said...

looks like we're up to 5 total reported bugs since the 24th.

hypergeometric said...

There are many ways C code can be made safer. Many have be ignored or abandoned because they are not expectations "the real world" uses.

(1) Write portable code, like ANSI C, or write mostly portable code and limit use of extensions (including GNU) to selected places.

(2) Use assert.

(3) When using devices like "strcpy", be sure the buffer it is in is allocated and then has a NUL as its last character.

(4) Allocate memory off the stack rather than malloc whenever possible.

(5) Insist upon zero compilation errors and warnings from the compiler.

(6) Every module should have a set of tests devised by its developer for checking its operation. This set of tests should exercise boundary conditions. It should have a clearly stated set of correctness requirements, enforced by assert. Each time a bug is found in the module, even during development, another test trapping that case should be added to the list of test cases. The test cases should become a part of the documentation for the module.

Haimez said...

You've made my day, sir. Accurate, biting, perfect.

Jean-Rémi Desjardins said...

Start by using another language than C.

jptigrou said...

we are talking about a shell. it's one of the most executed pieces of code in term of percentage of cpu, and no one seems to take performance into account.

I'm pretty sure that snprintf is 10 times slower than a couple of strcpy's.