The Linux Backdoor Attempt of 2003 (2013) • Hacker News Board

Re: The Linux Backdoor Attempt of 2003 (2013)

#21

Post by smcl » Mon, Aug 10, 2020, 9:11 AM UTC

msla wrote: post #10

This is something C linters have been catching probably since there have been C linters, either from looking for that specific pattern (a lone equals sign in a conditional) or by "inventing" the notion of a boolean type long before C had one and then pretending that only comparison operators had such a type. Needless to say, the better class of compiler catches this fine. gcc 9 does with -Wall and makes it an error w…

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#22

Post by jrockway » Mon, Aug 10, 2020, 9:13 AM UTC

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.

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.

Re: The Linux Backdoor Attempt of 2003 (2013)

#23

Post by phh » Mon, Aug 10, 2020, 9:14 AM UTC

msla wrote: post #10

This is something C linters have been catching probably since there have been C linters, either from looking for that specific pattern (a lone equals sign in a conditional) or by "inventing" the notion of a boolean type long before C had one and then pretending that only comparison operators had such a type. Needless to say, the better class of compiler catches this fine. gcc 9 does with -Wall and makes it an error w…

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#24

Post by KMag » Mon, Aug 10, 2020, 9:14 AM UTC

msla wrote: post #10

This is something C linters have been catching probably since there have been C linters, either from looking for that specific pattern (a lone equals sign in a conditional) or by "inventing" the notion of a boolean type long before C had one and then pretending that only comparison operators had such a type. Needless to say, the better class of compiler catches this fine. gcc 9 does with -Wall and makes it an error w…

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#25

Post by cyphar » Mon, Aug 10, 2020, 9:20 AM UTC

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#26

Post by hyperman1 » Mon, Aug 10, 2020, 9:23 AM UTC

stareatgoats wrote: post #7

Was this a backdoor or not? Following the comments on the article and previous posts here on HN it seems the jury is out AFAICS. The crucial question to me seems to be if this condition: options == (__WCLONE|__WALL) can be willfully introduced by a bad actor, and otherwise never really occur. Unfortunately I don't know this (not familiar with Linux development) but herein lies the answer it would seem.

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#27

Post by mikkom » Mon, Aug 10, 2020, 9:25 AM UTC

WhiteSage wrote: post #8

From the article comment section: > this is not a mistake. > Assume that the coder meant == 0 what is he trying to enforce. If these 2 bits (_WCLONE and _WALL) are set and your are root then the call is invalid. The bit combination is harmless (setting WALL implies WCLONE [...]), and why would you forbid it for root only.

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.

Re: The Linux Backdoor Attempt of 2003 (2013)

#28

Post by IshKebab » Mon, Aug 10, 2020, 9:50 AM UTC

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?

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

Re: The Linux Backdoor Attempt of 2003 (2013)

#29

Post by simias » Mon, Aug 10, 2020, 9:51 AM UTC

mikkom wrote: post #27

WhiteSage wrote: post #8

From the article comment section: > this is not a mistake. > Assume that the coder meant == 0 what is he trying to enforce. If these 2 bits (_WCLONE and _WALL) are set and your are root then the call is invalid. The bit combination is harmless (setting WALL implies WCLONE [...]), and why would you forbid it for root only.

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.

I would put parentheses here, I never like mixing logical operators with other types (or even different types of logical operators). While it's of course entirely redundant here, it also makes the code easier to read IMO.

I think the parent's point is more convincing: why make this check only for root in the first place?

Re: The Linux Backdoor Attempt of 2003 (2013)

#30

Post by kryptiskt » Mon, Aug 10, 2020, 9:58 AM UTC

londons_explore wrote: post #18

brongondwana wrote: post #13

Earlier quoted context omitted.

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…

Compilers will produce slower code for this construction.

If you're switching users in a hot loop you have other problems.