14 ms·

The Linux Backdoor Attempt of 2003 (2013)

298 points by t4h4 6y ago

hmottestad 6y ago
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.
- jdblair 6y ago
  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.
- jrockway 6y ago
  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.
- cyphar 6y ago
  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.
  - perihelions 6y ago
    "(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.
    - simias 6y ago
      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 !.
      - cyphar 6y ago
        The best one is "current" -- which is a macro that looks like a variable but becomes a function call and thus if you ever want to use the variable name "current" you will get build errors. :D
    - cyphar 6y ago
      Huh, I assumed (just as you did) that this would obviously not work -- but you're right that GCC ignores this and allows the assignment anyway. However it turns out that you still get a build error, and even the more explicit versions also give you a error: kernel/cred.c:763:17: error: assignment of member ‘euid’ in read-only object 763 | current_euid() = GLOBAL_ROOT_UID; | ^ kernel/cred.c:764:23: error: assignment of member ‘euid’ in read-only object 764 | current_cred()->euid = GLOBAL_ROOT_UID; | ^ kernel/cred.c:765:22: error: assignment of member ‘euid’ in read-only object 765 | current->cred->euid = GLOBAL_ROOT_UID; | ^ So it is blocked but not for the reason I thought. current_cred() returns a const pointer and all of the cred pointers in task_struct are also const. So you'd need to do something more like: ((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID; Which is well beyond "eyebrow-raising" territory.
- Cthulhu_ 6y ago
  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.
- db48x 6y ago
  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.
  - ed25519FUUU 6y ago
    := Still seems easy to overlook at a cursory glance.
    - kazinator 6y ago
      What db48x neglected to mention is that some of those languages also featured assignment as strictly a statement; it could not be a subexpression. As in: fun(x := 42); (* syntax error in Pascal *) x := 42; (* OK *) x = 42; (* hopefully a statement with no effect warning *) If assignment is a statement, it's possible to use the same token. Classic BASIC: 10 X = 5 20 IF X = 5 GOTO 10 This doesn't cause the C problem of mistaken assignment in place of a test, so it's rather ironic that C managed to shoot itself in the foot in spite of dedicating twice the number of tokens.
      - db48x 6y ago
        I'd forgotten that!
      - a1369209993 6y ago
        That is true, but has nothing to do with "==" vs "=" vs ":=". You can do: func(x = 42); /* syntax error: expression operator expected, got '=' */ x = 42; /* OK */ x == 42; /* warning: statement expression has no effect */ just fine if you require "=" to be a statement.
dang 6y ago
If curious see also 2018 https://news.ycombinator.com/item?id=18173173 https://news.ycombinator.com/item?id=18173173 Discussed at the time (of the article): https://news.ycombinator.com/item?id=6520678 https://news.ycombinator.com/item?id=6520678
stiray 6y ago
I think that this might be an typo or at least it has plausible deniability. I have changed my coding style to always put constant on left side just to avoid such an error (such typo gave me a few days of debugging multithreaded code and I have just said "Never again!!" :D)
- sild 6y ago
  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#index-Wparentheses https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#inde...
- luckylion 6y ago
  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.
- Tuna-Fish 6y ago
  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.
tester34 6y ago
I think code like this shouldn't even compile like in other languages >Operator '&&' cannot be applied to operands of type 'bool' and 'int'
- tleb_ 6y ago
  That would require some pretty big changes in the C programming language. Static analysis should detect it though, and probably does.
- josefx 6y ago
  _Bool is a relatively new addition to C, those operators return int results that are either zero or not.
jorangreef 6y ago
Something as important as "uid" should be "const".
- brongondwana 6y ago
  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).
  - londons_explore 6y ago
    Compilers will produce slower code for this construction.
    - brongondwana 6y ago
      The nice thing is, it's a pretty rare call hopefully, so that's not a big deal so long as they aren't slowing down the much more common reads.
    - kryptiskt 6y ago
      If you're switching users in a hot loop you have other problems.
  - jorangreef 6y ago
    Yep... there are ways to make it happen.
- grishka 6y ago
  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.
stareatgoats 6y ago
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.
- speedgoose 6y ago
  Definitely a door for a local privilege escalation. But since it's so obvious, we may call it a second front door.
- hyperman1 6y ago
  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 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.
  - stareatgoats 6y ago
    Ok thanks, that clinches it I think!
WhiteSage 6y ago
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.
- mikkom 6y ago
  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.
  - simias 6y ago
    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?
    - kazinator 6y ago
      The parentheses are required if == is changed to =. == has a higher precedence than &&, but = has a lower precedence. a = b && c && d means a = (b && c && d)
      - simias 6y ago
        Sure, my point was that even with the proper == comparison I'd still write the (now redundant) parens because I find it more readable that way. Actually in languages like Rust with type inference that make it cheap and non-verbose to declare intermediate values I tend to avoid complicated conditions altogether, I could rewrite the provided expression like: let invalid_options = (options == (__WCLONE|__WALL)); let is_root = (current->uid == 0); if invalid_options && is_root { // ... } One might argue that it's overkill but I find that it's more "self-documenting" that way. I find that the more experienced I get, the most verbose my code becomes. Maybe it's early onset dementia, or maybe it's realizing that it's easier to write the code than to read it. Of course you can do that in C as well but you have to declare the boolean variables, and in general it's frowned upon to intermingle code and variable declarations so you'd have to add them at the beginning of the function so it adds boilerplate etc...
msla 6y ago
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 with -Werror. Ditto clang 9. (Look at me giving version numbers as if this were recent. Any non-antediluvian C compiler worth using will do the same.) My point is, any reasonable build would at least pop up some errors for this, making it appear amateurish to me.
- smcl 6y ago
  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 https://yarchive.net/comp/linux/gcc.html
- phh 6y ago
  I don't think gcc 9 was available in 2003.
  - nurettin 6y ago
    We had gcc3, but some people were still stuck with redhat's patched 2.96 (which was officially 2.95 + some security patches)
    - hyperman1 6y ago
      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 http://gcc.gnu.org/gcc-2.96.html https://linux.web.cern.ch/docs/other/gcc296/ https://linux.web.cern.ch/docs/other/gcc296/
aborsy 6y ago
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?
- gonzo41 6y ago
  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.
  - kubanczyk 6y ago
    Parent said "in the source code" not "in the Linux source code". Given the abysmal standards of security everywhere, it's quite logical thing to assume that many parties have backdoors scattered around various OSes. A tempting target with such multiplicative benefits. I don't think it's a paranoid question and I don't think it's even a question. It's a natural assumption and I'd demand exceptionally good evidence to challenge that. Points for Linux for its openness, people will probably catch some of these.
- rectang 6y ago
  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.
  - FartyMcFarter 6y ago
    > This particular glitch was inserted via an attack on the BitKeeper repository. No, it was inserted into the CVS mirror.
baybal2 6y ago
Why attempted that backdooring attempt you think?
londons_explore 6y ago
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?
- IshKebab 6y ago
  Because of selection bias - if they had chosen something less subtle then we wouldn't be talking about it.
  - brobdingnagians 6y ago
    Maybe it is a smoke screen, put in something likely to be found and something that won't. Everyone pats themselves on the back for finding the obvious one...
    - gvjddbnvdrbv 6y ago
      This seems highly likely.
    - nkrisc 6y ago
      Why not just include the thing that won't be found and call it a day? Including a red herring to invite extra scrutiny doesn't seem wise if you're trying to hide something.
    - flingo 6y ago
      Step 1: Get 3 pigs. Step 2: Number them 1, 2, and 4. ...
  - kubanczyk 6y ago
    You've probably meant: if they had chosen something much subtler we wouldn't be talking about it.
- GuB-42 6y ago
  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 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.
blauditore 6y ago
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.
- rocqua 6y ago
  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.
  - asddubs 6y ago
    could do this instead, right? do { *dst = *src; *dst++; *src++; } while(*dst);
    - deleted 6y ago
      [deleted]
    - josefx 6y ago
      I think you are not copying the terminating nul character.
      - FartyMcFarter 6y ago
        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.
        asddubs 6y ago
        haha, I was genuinely asking if it made sense, I don't usually do C. Maybe it helped illustrate that the code is too clever for me, at least. e: I submit my revised code: do { *dst = *src; src++; dst++; } while(*(dst - 1));
gitgud 6y ago
Has this happened since the source-control was changed to git? I imagine it would be almost impossible to break into Linus Torvald's git server amend previous commits, considering each one's hashed on the previous commits...
- eru 6y ago
  If you can break SHA1, that task would be easier.
  - Cthulhu_ 6y ago
    SHA1 is close to being broken, but it's not there yet, and Git will be migrating to a better algorithm. That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right?
    - tomxor 6y ago
      > That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right? I think so, assuming the fetch algorithm is using the hashes to get the deltas which I think it does. I'm not sure about CVS but with GIT rewriting a _previous_ commit _object_ itself with different blobs but making the commit object itself have the _same_ hash by messing with it's comment wouldn't cause any difference in child commits since commits are pretty much independent other than the pointers to parent/child and incorporating that into it's hash (i.e they would have different trees so the changes would not propagate to the HEAD of the branch). I think the only way have something end up in the HEAD of a branch AND persist is to break the SHA1 of a blob (i.e a file) by inserting the extra SHA1 breaking content into the blob itself rather than a commit tree (provided that exact blob hash is part of the tree in the HEAD of a branch). Then you would also need to hope that the malicious blob is fetched by the person who writes the next commit to be based upon the HEAD of that branch AND modifies the same file blob so that it persists into the next revision of the blob... seems pretty hard to pull off - pun intended There is also the issue of pushing a blob that already exists on the remote according to the hash. Even with re-write permission GC might make that hard to do quickly.... I wonder if you would need direct access to the git server to do this. [EDIT] Thinking about swapping out SHA1 in the future, you would still want to rehash all of the blobs and trees to prevent SHA1 attacks on old blobs that are unchanged going forward to essentially prevent what I described above. If you only hashed new blobs with the new algorithm you would need to wait until every file had been touched to be safe.
davidhyde 6y ago
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.
- chmod775 6y ago
  >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.
  - ThePowerOfFuet 6y ago
    Hey, that's the combination to my luggage!
- lqet 6y ago
  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.
  - DC-3 6y ago
    Except that uninitialised memory is substantially more likely to be 0 than any other value.
    - grishka 6y ago
      Except sometimes it is not and forgetting to initialize a variable in C/C++ leads to very insidious bugs that no one can reliably reproduce.
  - kevincox 6y ago
    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.
widforss 6y ago
Won't gcc complain if you assign a variable within an if-statement?
- GuB-42 6y ago
  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.
  - tsbinz 6y ago
    The parentheses here aren't actually "extra", without them the meaning would change - since && binds tighter than = without the parentheses the left hand side of = would not be an lvalue and compilation would fail.
yyyk 6y ago
This is an obvious backdoor attempt, as the code doesn't make sense otherwise. Yet, the attempt was far too unsubtle and underspecific for agencies such as the NSA. The payoff was low compared to the possibilities - local privilege escalations were a dime-a-dozen. Worse, agencies such as the NSA have two missions: offence and defence. Adding in backdoors helps the offensive mission, but hurts the defensive mission, so it only makes sense if the backdoor isn't so easy to find. An obvious backdoor hurts the US far more than it helps the US. This one was too obvious. Some ideas: 1) A script kiddie found some way to break-in and edit CVS. The entire idea being to have something to brag about. This was caught too early to be brag-worthy (breaking ancient CVS isn't something to brag about). 2) It was a warning shot from some Western agency meaning "tighten up your security".
- badRNG 6y ago
  > 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?
  - yyyk 6y ago
    I can't point to any evidence, but two things to note: A) Even on HN the temptation has come up. e.g. some comments in posts about ransomware make a similar argument for transparently damaging and self-serving actions. Three letter agencies with much more power and ability probably had people making the same arguments. B) The payoff was extremely low compared to the possibilities. Either whomever did this was unaware of the possibilities or not really interested in a major hack. Perhaps the idea was that if this actually works, the damage isn't so big, aside from embarrassing the Linux kernel team, and when the team noticed they'd tighten up.
    - ed25519FUUU 6y ago
      This theory seems so outrageously far fetched to me. Why in the world would a "friendly" intelligence agency sneak a working backdoor into a project to "teach a lesson"?? Here's what our intelligence agencies do when they decide to "teach a lesson"[1]. It doesn't include sneaking working backdoors into software. They do THAT when they plan on using the backdoors. https://www.marketwatch.com/story/nsa-alerts-microsoft-of-major-security-flaw-in-windows-10-2020-01-14 https://www.marketwatch.com/story/nsa-alerts-microsoft-of-ma...
reactchain 6y ago
What are the chances major projects we use today aren't backdoored similarly? It's so easy to do and so hard to detect.
- coldpie 6y ago
  > 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.
bugeats 6y ago
ITT: everyone pretending they've never burned hours troubleshooting only to find a stupid `=` instead of a `==`.
- ViViDboarder 6y ago
  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
- vlovich123 6y ago
  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.
  - not2b 6y ago
    This code would not trigger under -Wall -Werror. Try it.
    - vlovich123 6y ago
      I was referring to what the parent wrote: > ITT: everyone pretending they've never burned hours troubleshooting only to find a stupid `=` instead of a `==`. In the general case that OP was talking about, not for underhanded code, my comment holds.
pyuser583 6y ago
How would git have handled the same issue? I imagine if Linus pushed to the remote repo, it would have said “your repo isn’t up to date”. But AFAIK, it doesn’t have the same sort of built in checksum checkers. If an attacker signed the commit insecurely, would git complain? Can you set git to require PGP signatures? Probably.
- woodrowbarlow 6y ago
  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.

298 points by t4h4 6y ago

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.

jdblair 6y ago
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.
jrockway 6y ago
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.
cyphar 6y ago
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.
- perihelions 6y ago
  "(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.
  - simias 6y ago
    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 !.
    - cyphar 6y ago
      The best one is "current" -- which is a macro that looks like a variable but becomes a function call and thus if you ever want to use the variable name "current" you will get build errors. :D
  - cyphar 6y ago
    Huh, I assumed (just as you did) that this would obviously not work -- but you're right that GCC ignores this and allows the assignment anyway. However it turns out that you still get a build error, and even the more explicit versions also give you a error: kernel/cred.c:763:17: error: assignment of member ‘euid’ in read-only object 763 | current_euid() = GLOBAL_ROOT_UID; | ^ kernel/cred.c:764:23: error: assignment of member ‘euid’ in read-only object 764 | current_cred()->euid = GLOBAL_ROOT_UID; | ^ kernel/cred.c:765:22: error: assignment of member ‘euid’ in read-only object 765 | current->cred->euid = GLOBAL_ROOT_UID; | ^ So it is blocked but not for the reason I thought. current_cred() returns a const pointer and all of the cred pointers in task_struct are also const. So you'd need to do something more like: ((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID; Which is well beyond "eyebrow-raising" territory.
Cthulhu_ 6y ago
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.
db48x 6y ago
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.
- ed25519FUUU 6y ago
  := Still seems easy to overlook at a cursory glance.
  - kazinator 6y ago
    What db48x neglected to mention is that some of those languages also featured assignment as strictly a statement; it could not be a subexpression. As in: fun(x := 42); (* syntax error in Pascal *) x := 42; (* OK *) x = 42; (* hopefully a statement with no effect warning *) If assignment is a statement, it's possible to use the same token. Classic BASIC: 10 X = 5 20 IF X = 5 GOTO 10 This doesn't cause the C problem of mistaken assignment in place of a test, so it's rather ironic that C managed to shoot itself in the foot in spite of dedicating twice the number of tokens.
    - db48x 6y ago
      I'd forgotten that!
    - a1369209993 6y ago
      That is true, but has nothing to do with "==" vs "=" vs ":=". You can do: func(x = 42); /* syntax error: expression operator expected, got '=' */ x = 42; /* OK */ x == 42; /* warning: statement expression has no effect */ just fine if you require "=" to be a statement.

dang 6y ago

If curious see also 2018 https://news.ycombinator.com/item?id=18173173 https://news.ycombinator.com/item?id=18173173 Discussed at the time (of the article): https://news.ycombinator.com/item?id=6520678 https://news.ycombinator.com/item?id=6520678

stiray 6y ago

I think that this might be an typo or at least it has plausible deniability. I have changed my coding style to always put constant on left side just to avoid such an error (such typo gave me a few days of debugging multithreaded code and I have just said "Never again!!" :D)

sild 6y ago
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#index-Wparentheses https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#inde...
luckylion 6y ago
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.
Tuna-Fish 6y ago
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.

tester34 6y ago

I think code like this shouldn't even compile like in other languages >Operator '&&' cannot be applied to operands of type 'bool' and 'int'

tleb_ 6y ago
That would require some pretty big changes in the C programming language. Static analysis should detect it though, and probably does.
josefx 6y ago
_Bool is a relatively new addition to C, those operators return int results that are either zero or not.

jorangreef 6y ago

Something as important as "uid" should be "const".

brongondwana 6y ago
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).
- londons_explore 6y ago
  Compilers will produce slower code for this construction.
  - brongondwana 6y ago
    The nice thing is, it's a pretty rare call hopefully, so that's not a big deal so long as they aren't slowing down the much more common reads.
  - kryptiskt 6y ago
    If you're switching users in a hot loop you have other problems.
- jorangreef 6y ago
  Yep... there are ways to make it happen.
grishka 6y ago
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.

stareatgoats 6y ago

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.

speedgoose 6y ago
Definitely a door for a local privilege escalation. But since it's so obvious, we may call it a second front door.
hyperman1 6y ago
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 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.
- stareatgoats 6y ago
  Ok thanks, that clinches it I think!

WhiteSage 6y ago

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.

mikkom 6y ago
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.
- simias 6y ago
  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?
  - kazinator 6y ago
    The parentheses are required if == is changed to =. == has a higher precedence than &&, but = has a lower precedence. a = b && c && d means a = (b && c && d)
    - simias 6y ago
      Sure, my point was that even with the proper == comparison I'd still write the (now redundant) parens because I find it more readable that way. Actually in languages like Rust with type inference that make it cheap and non-verbose to declare intermediate values I tend to avoid complicated conditions altogether, I could rewrite the provided expression like: let invalid_options = (options == (__WCLONE|__WALL)); let is_root = (current->uid == 0); if invalid_options && is_root { // ... } One might argue that it's overkill but I find that it's more "self-documenting" that way. I find that the more experienced I get, the most verbose my code becomes. Maybe it's early onset dementia, or maybe it's realizing that it's easier to write the code than to read it. Of course you can do that in C as well but you have to declare the boolean variables, and in general it's frowned upon to intermingle code and variable declarations so you'd have to add them at the beginning of the function so it adds boilerplate etc...

msla 6y ago

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 with -Werror. Ditto clang 9. (Look at me giving version numbers as if this were recent. Any non-antediluvian C compiler worth using will do the same.) My point is, any reasonable build would at least pop up some errors for this, making it appear amateurish to me.

smcl 6y ago
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 https://yarchive.net/comp/linux/gcc.html
phh 6y ago
I don't think gcc 9 was available in 2003.
- nurettin 6y ago
  We had gcc3, but some people were still stuck with redhat's patched 2.96 (which was officially 2.95 + some security patches)
  - hyperman1 6y ago
    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 http://gcc.gnu.org/gcc-2.96.html https://linux.web.cern.ch/docs/other/gcc296/ https://linux.web.cern.ch/docs/other/gcc296/

aborsy 6y ago

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?

gonzo41 6y ago
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.
- kubanczyk 6y ago
  Parent said "in the source code" not "in the Linux source code". Given the abysmal standards of security everywhere, it's quite logical thing to assume that many parties have backdoors scattered around various OSes. A tempting target with such multiplicative benefits. I don't think it's a paranoid question and I don't think it's even a question. It's a natural assumption and I'd demand exceptionally good evidence to challenge that. Points for Linux for its openness, people will probably catch some of these.
rectang 6y ago
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.
- FartyMcFarter 6y ago
  > This particular glitch was inserted via an attack on the BitKeeper repository. No, it was inserted into the CVS mirror.

baybal2 6y ago

Why attempted that backdooring attempt you think?

londons_explore 6y ago

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?

IshKebab 6y ago
Because of selection bias - if they had chosen something less subtle then we wouldn't be talking about it.
- brobdingnagians 6y ago
  Maybe it is a smoke screen, put in something likely to be found and something that won't. Everyone pats themselves on the back for finding the obvious one...
  - gvjddbnvdrbv 6y ago
    This seems highly likely.
  - nkrisc 6y ago
    Why not just include the thing that won't be found and call it a day? Including a red herring to invite extra scrutiny doesn't seem wise if you're trying to hide something.
  - flingo 6y ago
    Step 1: Get 3 pigs. Step 2: Number them 1, 2, and 4. ...
- kubanczyk 6y ago
  You've probably meant: if they had chosen something much subtler we wouldn't be talking about it.
GuB-42 6y ago
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 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.

blauditore 6y ago

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.

rocqua 6y ago
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.
- asddubs 6y ago
  could do this instead, right? do { *dst = *src; *dst++; *src++; } while(*dst);
  - deleted 6y ago
    [deleted]
  - josefx 6y ago
    I think you are not copying the terminating nul character.
    - FartyMcFarter 6y ago
      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.
      - asddubs 6y ago
        haha, I was genuinely asking if it made sense, I don't usually do C. Maybe it helped illustrate that the code is too clever for me, at least. e: I submit my revised code: do { *dst = *src; src++; dst++; } while(*(dst - 1));

gitgud 6y ago

Has this happened since the source-control was changed to git? I imagine it would be almost impossible to break into Linus Torvald's git server amend previous commits, considering each one's hashed on the previous commits...

eru 6y ago
If you can break SHA1, that task would be easier.
- Cthulhu_ 6y ago
  SHA1 is close to being broken, but it's not there yet, and Git will be migrating to a better algorithm. That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right?
  - tomxor 6y ago
    > That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right? I think so, assuming the fetch algorithm is using the hashes to get the deltas which I think it does. I'm not sure about CVS but with GIT rewriting a _previous_ commit _object_ itself with different blobs but making the commit object itself have the _same_ hash by messing with it's comment wouldn't cause any difference in child commits since commits are pretty much independent other than the pointers to parent/child and incorporating that into it's hash (i.e they would have different trees so the changes would not propagate to the HEAD of the branch). I think the only way have something end up in the HEAD of a branch AND persist is to break the SHA1 of a blob (i.e a file) by inserting the extra SHA1 breaking content into the blob itself rather than a commit tree (provided that exact blob hash is part of the tree in the HEAD of a branch). Then you would also need to hope that the malicious blob is fetched by the person who writes the next commit to be based upon the HEAD of that branch AND modifies the same file blob so that it persists into the next revision of the blob... seems pretty hard to pull off - pun intended There is also the issue of pushing a blob that already exists on the remote according to the hash. Even with re-write permission GC might make that hard to do quickly.... I wonder if you would need direct access to the git server to do this. [EDIT] Thinking about swapping out SHA1 in the future, you would still want to rehash all of the blobs and trees to prevent SHA1 attacks on old blobs that are unchanged going forward to essentially prevent what I described above. If you only hashed new blobs with the new algorithm you would need to wait until every file had been touched to be safe.

davidhyde 6y ago

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.

chmod775 6y ago
>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.
- ThePowerOfFuet 6y ago
  Hey, that's the combination to my luggage!
lqet 6y ago
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.
- DC-3 6y ago
  Except that uninitialised memory is substantially more likely to be 0 than any other value.
  - grishka 6y ago
    Except sometimes it is not and forgetting to initialize a variable in C/C++ leads to very insidious bugs that no one can reliably reproduce.
- kevincox 6y ago
  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.

widforss 6y ago

Won't gcc complain if you assign a variable within an if-statement?

GuB-42 6y ago
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.
- tsbinz 6y ago
  The parentheses here aren't actually "extra", without them the meaning would change - since && binds tighter than = without the parentheses the left hand side of = would not be an lvalue and compilation would fail.

yyyk 6y ago

This is an obvious backdoor attempt, as the code doesn't make sense otherwise. Yet, the attempt was far too unsubtle and underspecific for agencies such as the NSA. The payoff was low compared to the possibilities - local privilege escalations were a dime-a-dozen. Worse, agencies such as the NSA have two missions: offence and defence. Adding in backdoors helps the offensive mission, but hurts the defensive mission, so it only makes sense if the backdoor isn't so easy to find. An obvious backdoor hurts the US far more than it helps the US. This one was too obvious. Some ideas: 1) A script kiddie found some way to break-in and edit CVS. The entire idea being to have something to brag about. This was caught too early to be brag-worthy (breaking ancient CVS isn't something to brag about). 2) It was a warning shot from some Western agency meaning "tighten up your security".

badRNG 6y ago
> 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?
- yyyk 6y ago
  I can't point to any evidence, but two things to note: A) Even on HN the temptation has come up. e.g. some comments in posts about ransomware make a similar argument for transparently damaging and self-serving actions. Three letter agencies with much more power and ability probably had people making the same arguments. B) The payoff was extremely low compared to the possibilities. Either whomever did this was unaware of the possibilities or not really interested in a major hack. Perhaps the idea was that if this actually works, the damage isn't so big, aside from embarrassing the Linux kernel team, and when the team noticed they'd tighten up.
  - ed25519FUUU 6y ago
    This theory seems so outrageously far fetched to me. Why in the world would a "friendly" intelligence agency sneak a working backdoor into a project to "teach a lesson"?? Here's what our intelligence agencies do when they decide to "teach a lesson"[1]. It doesn't include sneaking working backdoors into software. They do THAT when they plan on using the backdoors. https://www.marketwatch.com/story/nsa-alerts-microsoft-of-major-security-flaw-in-windows-10-2020-01-14 https://www.marketwatch.com/story/nsa-alerts-microsoft-of-ma...

reactchain 6y ago

What are the chances major projects we use today aren't backdoored similarly? It's so easy to do and so hard to detect.

coldpie 6y ago
> 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.

bugeats 6y ago

ITT: everyone pretending they've never burned hours troubleshooting only to find a stupid `=` instead of a `==`.

ViViDboarder 6y ago
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
vlovich123 6y ago
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.
- not2b 6y ago
  This code would not trigger under -Wall -Werror. Try it.
  - vlovich123 6y ago
    I was referring to what the parent wrote: > ITT: everyone pretending they've never burned hours troubleshooting only to find a stupid `=` instead of a `==`. In the general case that OP was talking about, not for underhanded code, my comment holds.

pyuser583 6y ago

How would git have handled the same issue? I imagine if Linus pushed to the remote repo, it would have said “your repo isn’t up to date”. But AFAIK, it doesn’t have the same sort of built in checksum checkers. If an attacker signed the commit insecurely, would git complain? Can you set git to require PGP signatures? Probably.

woodrowbarlow 6y ago
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.