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.