Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Linux Backdoor Attempt of 2003 (2013)

freedom-to-tinker.com

Post reply on HN
Print view View original

51–60 of 144 posts

Re: The Linux Backdoor Attempt of 2003 (2013)

#51

Post by lqet »

blauditore wrote: post #37

I'm curious, wouldn't this also be caught by static code analysis tools, at least today? An assigment inside an if condition is both, most likely a mistake, and fairly easy to detect automatically.

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.

Re: The Linux Backdoor Attempt of 2003 (2013)

#52

Post by perihelions »

cyphar wrote: post #25
hmottestad wrote: post #2

I admit that I read the code and completely overlooked the single equals sign. Makes me wonder why it would be so easy to change the userid. Shouldn’t there be some safeguards in place to stop the userid from being updated from unsafe places.

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 cur…

"(current_euid() is a macro but it's written to not be a permitted lvalue)"

I'm not an expert at C. I followed up on this kernel macro out of curiosity, and it was a confusing learning experience because it turns out the forbidden assignment

    ({ x; }) = y;
is silently permitted by GCC (for example, with -Wall --std={c99,c11,c18}), and does actually assign x=y. Even though that's expressly prohibited by the C standard (-Wpedantic).

I assume this is old news to C programmers, but its insidiousness surprised me.

Re: The Linux Backdoor Attempt of 2003 (2013)

#53

Post by DC-3 »

lqet wrote: post #47
davidhyde wrote: post #41

A uid of 0 being root is just such a bad idea to begin with because 0 is a default value of so many data types. It’s an accident waiting to happen and, in this case, a good way to hide something malicious as an accident.

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.

Except that uninitialised memory is substantially more likely to be 0 than any other value.

Re: The Linux Backdoor Attempt of 2003 (2013)

#54

Post by kevincox »

lqet wrote: post #47
davidhyde wrote: post #41

A uid of 0 being root is just such a bad idea to begin with because 0 is a default value of so many data types. It’s an accident waiting to happen and, in this case, a good way to hide something malicious as an accident.

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.

That's not quite true. While it is undefined 0 is a fairly common value for memory and registers meaning that your "undefined" values is likely 0 a higher than average amount of the time.

Re: The Linux Backdoor Attempt of 2003 (2013)

#55

Post by hyperman1 »

nurettin wrote: post #35
phh wrote: post #23

Earlier quoted context omitted.

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

We had gcc3, but some people were still stuck with redhat's patched 2.96 (which was officially 2.95 + some security patches)

It was worse than that. They took whatever unreleased code was in the gnu repository on a random day, and started patching that. gcc 2.96 was known for miscompiling all sorts of stuff. GNU caught a lot of flack for a compiler they didn't even release.

AFAK Red Hat did this as they wanted to support ia64, but no (released) gcc version had a backend for it.

2 sides of this story:

http://gcc.gnu.org/gcc-2.96.html

https://linux.web.cern.ch/docs/other/gcc296/

Re: The Linux Backdoor Attempt of 2003 (2013)

#56

Post by FartyMcFarter »

rectang wrote: post #45
aborsy wrote: post #12

There should be safe guards against such errors. Even with approval, the reviewer may not notice it. Which brings up the question: how many more root-based backdoors are there now in the source 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 t…

> This particular glitch was inserted via an attack on the BitKeeper repository.

No, it was inserted into the CVS mirror.

Re: The Linux Backdoor Attempt of 2003 (2013)

#58

Post by FartyMcFarter »

josefx wrote: post #50
asddubs wrote: post #42

Earlier quoted context omitted.

could do this instead, right? do { *dst = *src; *dst++; *src++; } while(*dst);

I think you are not copying the terminating nul character.

Unless the first character was null, in which case it would be ignored by the condition... Also, you don't need to dereference a pointer in order to increase it.

The grandparent post's code is just nonsensical.

Re: The Linux Backdoor Attempt of 2003 (2013)

#59

Post by simias »

perihelions wrote: post #52
cyphar wrote: post #25

Earlier quoted context omitted.

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 cur…

"(current_euid() is a macro but it's written to not be a permitted lvalue)" I'm not an expert at C. I followed up on this kernel macro out of curiosity, and it was a confusing learning experience because it turns out the forbidden assignment ({ x; }) = y; is silently permitted by GCC (for example, with -Wall --std={c99,c11,c18}), and does actually assign x=y. Even though that's expressly prohibited by the C standard…

Example #5984 of why I don't like the kernel's convention of masquerading macros as function by making them lowercase. I wasted so much time deciphering weird compile errors or strange behaviour only to finally realize that one of the function calls in the offending code was actually a macro in disguise.

It's especially bad when some kernel macros, such as wait_event, don't even behave like a function would (evaluating the parameter repeatedly).

One more thing Rust got right by suffixing macros with a mandatory !.

Re: The Linux Backdoor Attempt of 2003 (2013)

#60

Post by GuB-42 »

londons_explore wrote: post #16

The underhanded C contest shows it is so easy to insert backdoors into C code that even someone staring at the code for a while wouldn't find. So why did this attacker choose such an obvious 'typo' rather than a subtle flaw in a large patch set?

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.

Post reply on HN