
The Linux Backdoor Attempt of 2003 (2013) - t4h4
https://freedom-to-tinker.com/2013/10/09/the-linux-backdoor-attempt-of-2003/
======
yyyk
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".

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

~~~
dzdt
This sounds like a very interesting tale. Are there more details written
somewhere? How close to first-person is your source of information?

~~~
sslalready
Not that I'm aware of, but I wish. Memory is getting hazy these days. AFAIK
the kernel.org breaches were made by the kind of hackers doing it for fun and
games (if you get that thing) and not the kind working for nation states. I'm
sure you can (or at least, at some point could) find others who know more
details at your favorite compsec conf.

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

------
londons_explore
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
Because of selection bias - if they had chosen something less subtle then we
wouldn't be talking about it.

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

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

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

~~~
cyphar
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
_" (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
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
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

------
dang
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)

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

~~~
lqet
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
Except that uninitialised memory is substantially more likely to be 0 than any
other value.

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

------
aborsy
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?

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

~~~
wycy
Assignments in conditionals can be handy handy, but I think it's better when
there's a keyword for it. The Rust/Swift `if let` syntax is pretty nice for
this.

    
    
        if let userID = 0 {}
    

vs

    
    
        if userID == 0 {}
    

The let syntax makes this error more obvious.

~~~
kibwen
Since you're using Rust as an example there, worth noting that unlike in C the
assignment operator in Rust does not evaluate to the assigned value (it
evaluates to the unit value `()` instead). In combination with the fact that
`if` expressions in Rust require their conditions to be bools (the language
has no automatic coercion to bool), this means that `if foo = 0` is guaranteed
to be a type error.

(This difference in the behavior of the assignment operator is a result of
Rust's ownership semantics; since assignment transfers ownership, having the
assignment operator evaluate to the assigned value would actually result in
the original assignment being entirely undone!)

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

~~~
GuB-42
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
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.

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

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

~~~
skocznymroczny
Is that an ancient predecessor to HolyC?

------
gitgud
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
If you can break SHA1, that task would be easier.

~~~
Cthulhu_
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?

~~~
db48x
Even if you could break SHA1, it's unlikely that your replacement source code
would look like it was human-written. Instead, it's going to look like human-
written source code containing kilobytes or megabytes of random-looking
comments. The comments will only be there to change the hash of the new
content back to the hash of the original content. It's not going to be subtle
at all.

~~~
flingo
Why would it require that much data? I always thought you wouldn't need to add
or change more bytes than are in the output.

Also, git hashes aren't just based on source code. You can add that data
anywhere that git uses to generate the hash.

~~~
db48x
That's true of a CRC code, but hashes are a lot harder to break.

Git hashes each file, and puts those hashes into a tree object, like a
directory listing. Then it hashes the trees, recursively back up to the root
of the repository. Finally the hash of the root tree is put in the commit
object, and the commit object is hashed. Thus the two places you can put
additional data to be hashed are the file contents (either in existing files
or new files), or in the commit message. You can get a few free bits by
adjusting less obvious things like the commit timestamp or the author's email
address, but not nearly enough to make your forged commit have the same hash
as an existing commit.

~~~
flingo
I'm still not following why it'd require so much data? I thought the goal was
to have the commit hash collide with an existing commit hash, is that not
enough?

I looked around, and it seems like the right place to hide the added data is
in the "trailer" section of the commit. It's where signed-off-by lives and is
used to generate the commit hash.

You might want to come up with a plausible reason for random data to go in
there though. (likely using a header that wouldn't normally get printed out)

~~~
db48x
In a CRC-style code, you're essentially adding up all the bytes and letting it
overflow the counter, so that the counter is a fixed size (usually 16 or 32
bits). Then you add a few more bytes, exactly the same size as the counter, so
that the data bytes plus the extra bytes add up to zero. The extra bytes are
delivered along with the data bytes, so that the recipient can repeat the
calculation and verify that the total is still zero. If you modify the data,
it is trivial to recalculate the CRC code so that the total is still zero.

Hashes are much, much more complex, and they're non-linear. Each bit of the
hash output is intended to depend on every single bit of the input, so that
changing a single bit in the input creates a radically different hash output.

In a paper published this year,
[https://eprint.iacr.org/2020/014.pdf](https://eprint.iacr.org/2020/014.pdf),
the authors Gaëtan Leurent and Thomas Peyrin changed the values of 825 bytes
out of a 1152 byte PGP key in order to generate a new key with the same
signature (aka, the same hash). It only cost about $45k, too.

------
blauditore
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
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
could do this instead, right?

    
    
        do {
           *dst = *src;
           *dst++;
           *src++;
        } while(*dst);

~~~
josefx
I think you are not copying the terminating nul character.

~~~
FartyMcFarter
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
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));

~~~
asveikau
Looking at src[-1] would work but feels like poor form. My advice is stop
trying to make do ... while work here, it isn't looking good.

I feel like more idiomatic iterating through a string tends to loop on src not
being a terminator.

    
    
        while (*src)
        {
           *dst = *src;
           ++src;
           ++dst;
        }
    
        *dst = '\0';
    

I feel like this is idiomatic C but needlessly verbose. Most people would
combine the increment with the assignment. And most people would recognize
putting it in the while condition as a common strcpy.

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

~~~
hyperman1
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
Ok thanks, that clinches it I think!

------
reactchain
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
> 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
ITT: everyone pretending they've never burned hours troubleshooting only to
find a stupid `=` instead of a `==`.

~~~
vlovich123
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
This code would not trigger under -Wall -Werror. Try it.

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

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

~~~
brongondwana
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
Compilers will produce slower code for this construction.

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

------
tester34
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_
That would require some pretty big changes in the C programming language.
Static analysis should detect it though, and probably does.

------
baybal2
Why attempted that backdooring attempt you think?

------
stiray
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)

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

