The Linux Backdoor Attempt of 2003 (2013) (opens in new tab)

If memory serves me right the CVS bug was originally discovered and exploited by a member of an infamous file sharing site. After descriptions(?) of that bug were leaked in underground circles, an east European hacker wrote up his own exploit for it. This second exploit was eventually traded for hatorihanzo.c, a kernel exploit, which was also a 0-day at the time.

The recipient of the hatorihanzo.c then tried to backdoor the kernel after first owning the CVS server and subsequently getting root on it.

The hatorihanzo exploit was left on the kernel.org server, but encrypted with an (at the time) popular ELF encrypting tool. Ironically the author of that same tool was on the forensic team and managed to crack the password, which turned out to be a really lame throwaway password.

And that's the story of how two fine 0-days were killed in the blink of an eye.

> 2) It was a warning shot from some Western agency meaning "tighten up your security".

That's an interesting theory that'd certainly make for a powerful message. Has anything like that been done before or is there any precedence for Western agencies to do these sorts of things covertly?

The NSA used to have a defensive mission. They fully compromised their ability to do that by subverting the security of American products time and time again. The Shadow Brokers disclosure alone has completely undermined any trust anyone in the industry has for the NSA.

> ...code change in the CVS copy that did not have a pointer to a record of approval. Investigation showed that the change had never been approved and, stranger yet, that this change did not appear in the primary BitKeeper repository at all...

I'll attach this here for people who read the article too quickly and think it may, somehow, have been a bug. This code was a very deliberate attack.

This is also very relevant comment:

> In addition, parentheses were not required for the final comparison. This was done to prevent compiler warnings. This looks deliberate.

It is not so easy, it is a contest, and they show you the winners.

And if you look at the "Scoring and Extra Points" section of http://underhanded-c.org/_page_id_5.html you will notice that it checks most of the boxes.

It is short, errors based on human perception (here = vs ==) are good enough, it is innocent looking under syntax highlighting, is is not platform dependent, and it even passes the "irony" check. It is just the plausible deniability that is not great, but it is still defensible with a lot of bad faith.

Because of selection bias - if they had chosen something less subtle then we wouldn't be talking about it.

GCC warns of assignment in conditional, even without -Wall or -pendantic. I don't know when it started doing that, but it seems like a sore thumb today, different in 2003 maybe?

These days it'd be harder to write code which is "easy to overlook" -- the innocent version would be something like

  if (/* ... */ || current_euid() == GLOBAL_ROOT_KUID)

But the "backdoor" version would fail to compile (current_euid() is a macro but it's written to not be a permitted lvalue). You would need to write something more obvious like the following (and kernel devs would go "huh?" upon seeing the usage of current_cred() in this context)

  if (/* ... */ || current_cred()->euid = GLOBAL_ROOT_KUID)

In addition, comparisons against UIDs directly are no longer as common because of user namespaces and capabilities -- correct code would be expected to look more like

  if (/* ... */ || capable(CAP_SYS_ADMIN))

Which you can't write as an "accidental" exploit. And since most permission checks these days use capabilities rather than raw UIDs you'd need to do

  commit_creds(get_cred(&init_cred));

Which is bound to raise more than a couple of eyebrows and is really non-trivial to hide (assuming you put it somewhere as obvious as this person did).

But I will say that it would've been much more clever to hide it in a device driver which is widely included as a built-in in distribution kernels. I imagine if you managed to compromise Linus' machine (and there are ways of "hiding" changes in merge commits) then the best place would be to shove the change somewhere innocuous like the proc connector (which is reachable via unprivileged netlink, is enabled on most distribution kernels, and is not actively maintained so nobody will scream about it). But these days we also have bots which actively scan people's trees and try to find exploits (including the 0day project and syzkaller), so such obvious bugs probably would still be caught.

Absolutely. This is an example of the poor design of the C language. Other languages that were around at the time C was created choose `:=` as assignment and `=` for equality tests, making this type of typo quite impossible.

Common Lisp makes the Hamming distance even larger; equality tests are written as `(eq foo bar)`, while changing a value is `(setf foo bar)`. Common Lisp may have features which are undesirable in an OS kernel (garbage collection), but it does make the code wonderfully clear and easy to read.

You make a good point, but in a monolithic kernel the kernel is the “safe place.” Most likely the effect of this would be subtle and not necessarily long lived.

Same; my Java indoctrination is kicking in and is asking why that field is apparently public and there's no controls as to what process can set it.

That said, counterpoint, it's the kernel and performance is super important; the overhead of adding setters (etc) or an utility function like "current->isRoot()" is probably a tradeoff they made at some point.

Same! I saw the if statement, was 100% sure this was going to be an "= instead of ==" thing... and still missed it. I spent too much mental energy looking at ((__LOUD|__NOISES)) and missed the obvious "current_user = 'root'" statement.

>and, in this case, a good way to hide something malicious as an accident

The number could've been 2342 and the backdoor would've worked exactly the same way.

AFAIK only external and static variables are default initialized in C. For all other variables, the default value is undefined, so 0 is as good a choice as any other here.

Unfortunately in C and its derivatives, the safeguards would have to be external tools (static analysis, linters); it's a perfectly valid statement in code.

I wouldn't mind if languages simply mark assignments in conditions as errors. It's clever code, but clever code should be avoided in critical systems. And in general, I guess.

A classic paranoid security question.

The ace in Linux's pocket is that you're free to read it all. That can't be said for Apple, and Microsoft or any of the OS's running switches and hubs out there. Let alone all the server side cloud code.

This particular glitch was inserted via an attack on the BitKeeper repository. (EDIT: it was actually a CVS mirror of the repo.)

But for the normal contribution flow, code review isn't the only safeguard. There's also a deterrent in that should a backdoor be inserted via a contribution that went through the normal process, an audit trail exists. If the backdoor is later discovered, there would be reputation harm to the contributor.

Depending on how much an open source project knows about its contributors, it may be more or less difficult to track down a culprit, but in any case the audit trail makes such attacks more complicated.

Not if you surround the expression with extra parenthesis. And that's what they did here.

Assignments in if-statement can be useful, and that's how you prevent the compiler from complaining. That warning is intended for honest mistakes, not to catch backdoors.

> non-antediluvian C compiler

Contrary to popular opinion, Noah's C compiler was actually highly advanced, but he only brought one copy on the ark with him. No backups, and less than ideal storage conditions... you can guess what happened next. A triceratops ate the parchment tape containing the only copy of Noah CC, and Noah threw the offending triceratops off the Ark, because in his rage, he thought "I have a spare tricero". Only afterword did he realize the error in his logic, thus dooming the triceratops to extinction.

* Only found in highly divergent manuscripts, widely assumed to be late additions.

I think I recall reading that around that time (remember this is 2003) Linus was either against -Werror or against spending effort eliminate warnings. The reason being that GCC had a few false positives, and the effort of making Linux kernel build with these spurious errors was not worth the risk of breaking code that likely worked ok.

However I can't find anything where this is directly said, all I can find is a collection of Linus' early 00s emails on the subject of GCC which includes a LOT of reference to said warnings: https://yarchive.net/comp/linux/gcc.html

I would be careful with statements like this. New compilers do NOT make this a warning/error, see for example

https://godbolt.org/z/5zzz33

Note that there are parentheses around the assignment which the compiler takes as an indication that this is intentional. Also note that the parentheses are required because without them the precedence would be wrong.

I don't think gcc 9 was available in 2003.

If you can break SHA1, that task would be easier.

I would guess this is part of the reason why most modern compilers will indeed emit a warning about assignment within if, for, and while - branch checks.

At the same time, the standard implementation of strcpy is:

    while((*dst++ = *src++));

which has a legitimate reason for doing assignment inside the while condition. Then again, one could argue that the above code is 'too clever'. And I would probably agree.

I think this is why there are parantheses around current->uid = 0. gcc has the option -Wparentheses, which gives a warning if you write something like this:

  if (a = b) doSomething;

But there is no warning if you write it like this:

  if ((a = b)) doSomething;

The convention is that with these unneeded parantheses, you are signalling that you actually want the assignment here. I would assume other static code analysis tools use this convention as well.

Following the man pages:

wait4's man page points to waitpid for details, and notes wait4 is deprecated in favor of waitpid.

So see the linux notes of this: https://man7.org/linux/man-pages/man2/waitpid.2.html

  The following Linux-specific options [..] can also, since Linux 4.7, be used with waitid():
  __WCLONE  [...] This option is ignored if __WALL is also specified.
  __WALL

So to trigger this:

* You have to call a deprecated function

* With a flag that was at that time illegal (linux < 4.7)

* And a second illegal flag that is cancelled out by the first illegal flag.

This is something any userspace process can do, but no sane process should ever do.

Definitely a door for a local privilege escalation. But since it's so obvious, we may call it a second front door.

> What are the chances major projects we use today aren't backdoored similarly?

Basically zero. There is no such thing as computer security in 2020.

Yea. Who hasn’t slipped up and forgotten an equals sign... and then accidentally exploited the Linux CVS and pushed their code without approval...

We’ve all been there! /s

It has been a long time since I make sure my codebases have `-Wall -Werror`. This bug is from 2003 both when that wasn't as common & when compiler diagnostics weren't as good/reliable.

each commit's id is an integrity hash of the repository at the time of commit. git doesn't provide access control; it relies on access controls built-into whichever transport mechanisms you choose to enable (https, ssh, etc).

you can sign commits with PGP signatures and with hooks, you can reject commits that aren't signed. i believe maintainers sign commits in the linux repo.

I mean... my first reading of that is "what a dumb idea, the reason it isn't const is that there are legit reasons to switch userid".

But then I have used exactly this pattern, and it looks something like:

struct protected_stuff { int userid; ... };

void set_userid(const struct protected_stuff prot, int newuserid) { struct protected_stuff backdoor = (struct protected_stuff *)prot; backdoor->userid = newuserid; }

and then the compiler complains if you go fiddling with userid outside this function where you deliberately opened a backdoor to write to it. (and you can wrap pragmas around that function to turn off warnings).

There are legitimate reasons to change the uid at runtime. For example, some server software starts as root and then drops to a less-privileged user. Android relies on this too, zygote, the fully-initialized "blank" runtime process, runs as root and gets forked and changes uid to the corresponding unprivileged user whenever an app is launched.

_Bool is a relatively new addition to C, those operators return int results that are either zero or not.

A typo is possible if it had been submitted to normal code review, but hacking into a server to secretly modify code all but rules out an accident.

If you are using gcc you can use the -Wparentheses flag to turn on warnings for this: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#inde...

Even with the "typo" corrected, the patch makes no sense. There is no plausible deniability why it should do what it would do. It was definitely a deliberate attack.