Thwarted Linux backdoor hints at smarter hacks (2003) | Hacker News Reader

Thwarted Linux backdoor hints at smarter hacks (2003)

securityfocus.com

85 pointsmikevm52 comments

Great to see that this has devolved into a religious argument about code syntax... well done the pedantic few, but you guys are missing the bigger picture

Like:

1). Have there been any other backdoors surreptitiously slipped in that nobody has noticed?

2). Is Linux Kernel really as secure as everyone thinks it is?

I'd spend more time scouring the code looking for other backdoors and securing those than worrying about a holy war on the merits of Yoda Comparisons [Which by the way I think suck. Use a compiler that errors out or warns on assignment found where comparison is expected, code is meant to be human readable, so make it so.]

The joy about Linux being open source is that you can get your fingers and minds in the code and even if Torvalds had put in a back door at the behest of some government entity or other, it wouldn't matter - you guys have the power (and ability) to close that door. So if I were you, I'd spend more time doing that and less time bitching about other coders' syntax preferences that may not match your own.

Linus doesn't have to put government collusion in the source. Look how he strong-armed using the hardware Intel RNG recently. That's microcode you'll never see. Is this collusion? Who knows, but trusting hardware makes code reviews useless.

Correct me if I'm wrong but /dev/random gathers entropy from a number of sources including CPU RNG. If your CPU was owned it wouldn't matter. I run entropy gathering daemons especially for virtualized systems. VMs don't have enough hardware entropy and software like vpn clients will get stuck waiting for data from /dev/random.

There are a number of ways to add entropy. I've used egd and haveged. Use them. It can't hurt.

I suppose you could not load microcode on boot. I've never tried.

Yes but the code is open, you could easily remove that block of code from the kernel locally and recompile... or fork and not allow anything that you can't verify in your own fork. Just because Linus strong-armed it in to the general release kernel, doesn't mean you're required to use it. What we need I think is an open source hardware RNG made by a company that's dedicated to open source principles.

Here's the actual code: http://lwn.net/Articles/57135/ Made use of the difference between == and = in C.

This was done by going around the normal channels to get code into the kernel. Someone attacked the CVS server directly and modified the code: http://lkml.indiana.edu/hypermail/linux/kernel/0311.0/0621.h...

Was the culprit found and outed?

That the attack came through the CVS server shows how far down the stack vulnerabilities could be exploited - had the attacker compromised the CVS first, this exploit might not have been caught so easily...or... Wouldn't subverting Subversion impact the success of subsequent subterfuge substantially?

I don't think that is easily overlooked. Maybe, if there had only been one conditional, but the different '==' and '=' are jarring to me.

OTOH, I'm a pretty slow code reader and look at "pattern" and "flow" of code at least as much as I attempt to understand meaning. Probably why I'm so anal about style, indentation, and vertical white space.

If I recall correctly, this particular backdoor was put in place by an eastern european guy going by the nick sky- in #phrack on the Eris Free Network. He killed a very valuable zeroday exploit in the process, causing both grief and anger among fellow hackers. Good times, though.

This is why they use git - subversively inserting code into git is not possible like it is with CVS

Except the article is from 2003 when Git did not exist (of course, you may have meant this is why they use git now...)

And this is why you write it like this when you use C:

   (0 == current->uid)

Rather than

   (current->uid == 0) // or (current->uid = 0) in this case.

Impossible to make an assignment to a numeric lvalue. Easy to spot, easy to audit in a diff.

> And this is why you write it like this when you use C:

Sure, if you are in habit of releasing code that compiles with warnings.

This sort of pointless code twisting is intensely annoying. It solves a rare problem with a wrong tool and in a very intrusive way.

Zero is constant. You compare to it.

Huh?

   $ cat test.c
   int main(int argc, char *argv[]) {
       if (0 == argc)
           return 1;
       return 0;
   }
   $ CFLAGS="-Wall -ansi -pedantic" make test
   cc -Wall -ansi -pedantic    test.c   -o test

Nope, no warnings there.

I believe they mean that

    if (foo = 0) ...

generates a warning like this:

    warn.c:2: warning: suggest parentheses around assignment used as truth value

which you should be paying attention to, in favour of yoda-comparisons in this particular holy war :)

The code in question (see the LWN link elsewhere in this discussion) actually had parens around the expression (in a context where they looked "normal") and wouldn't have generated a warning.

That said, I agree with huhtenberg above that twisting the language conventions around to deal with this is never going to fix anything. Subtle code remains subtle in all languages, and subtle code is where security bugs lie. You can't fix "subtle" with a rulebook.

I would but if you see the rest of the thread, my gcc isn't raising this as a warning!

That in itself is sign that a mistake can be made without noticing.

Right, but if you compile with warnings as errors, then assignment within a condition won't compile, which prevents the issue.

I'd much rather go that way than write code that reads less like a human wrote it.

What compiler flag is that? Genuinely interested. Wall,pedantic,ansi don't trigger it. I've tried the following and I don't get a warning or error:

   $ cat test2.c
   int main(int argc, char *argv[]) {
       if (argc = 0)
           return 1;
       return 0;
   }
   $ make test
   cc -Wall -ansi -pedantic    test.c   -o test

GCC version: gcc version 4.7.2 (Debian 4.7.2-5)

  huh@px:/tmp$ cat a.c
  int main(int argc, char *argv[]) {
         if (argc = 0)
             return 1;
         return 0;
     }
  huh@px:/tmp$ gcc -Wall a.c
  a.c: In function 'main':
  a.c:2: warning: suggest parentheses around assignment used as truth value

Doesn't do that for me - what GCC ver and environment. Very odd! Digging in docs. Thanks for info.

It's some old-ish Debian on x86. GCC is 4.3.2.

But this is a such commonly-recognized pitfall that I actually don't know a single modern production compiler that does not generate this warning.

(edit) Just checked 4.7.1 and it generates the warning.

Thanks. I think this must be a config issue on this box. It makes me wonder what other warnings it is not telling me about.

Notice that you cat test2.c and then compile test.c.

Well spotted! Thanks for pointing this out!

I will now go and hit myself with a LART for a bit to remember not to do that again.

However the same issue happens in a lot of C-like languages, and not all of them have compilers that inform you of errors. But the habit works in all of them.

There are two different ways to catch this class of bug. Both have value.

Or run a linter or static checker which disallows assignments in conditionals as part of your build and/or checkin process. They'll catch this and other fun C "gotchas" without relying on human vigilance.

That's the failsafe (one I believe heavily in as well). But getting it right up front is still the best approach.

Makes one appreciate languages like Pascal which differentiate between the equality operator '=' and the assignment operator ':='.

I wonder why the decision was made in C to use '=' and '=='? I'm betting it has something to do with the laziness of the programmers :).

The problem is not in == vs =, it's in allowing assignment as an expression. Go fixes this, for example, by now allowing them: if a = 0 {} gives compile error.

The problem is that == and = look similar. Banning assignment as expression only makes the language less expressive. Common Lisp fixes this, by having assignment and comparison look completely different: (setf variable value) vs (eql variable value)

Bit of a bummer, I often find using assignment as an expression useful, like if (x = y as String) == null or such in c#. I get the feeling Go sacrifices a bit too much freedom.

I'm not a professional programmer by any stretch, but wouldn't this problem completely go away if the C and C++ compilers detect a single = inside an if statement and outright refuse it. Either that or automatically substitute the assignment with checking for instances inside the if-condition.

The reason I'm asking is, because I don't know if there's ever a case where you'd want to assign right inside the if-condition. Is there?

There can be:

    if (Foo *foo = getFoo()) { // getFoo returns null if there is no foo
        // There is a foo
    }

However, this problem is basically entirely addressed by compiler warnings, which generally ask you to add an extra set of parens for this case.

Or you could also blame it on not having branch statements which enforce the use of a boolean type.

Edit: Actually, forget I said this. It's a dumb statement. 'if(a = true)' would always pass, regardless of the value of 'a'.

You don't do that because it makes the code less readable. If you codebase is really big you have to read the code all the time and it takes more brain power to translate your version. Also gcc has a warning vor assignments in if, which are imho a bad habit.

Thanks for this.

I don't actually use C, although I know a few languages with C-like syntax (Java, JS, AS2.0) and I'm learning Go & planning to learn D.

This small change will be really helpful in all these languages. Even if I never make this error again it will help stop people who alter my code from making this error.

Perhaps they should teach it in more programming textbooks, as I've not come across it in anything from HeadFirst to online tutorials to Deitel & Deitel.

FYI, sometimes called "Yoda condition". I _think_ I was taught to do this when I learnt C (~20 years ago). Wikipedia indicates that the name at least has been around since 2005.

It's not necessary in Java (though it's still a common habit). Assignment to a non-boolean is impossible in an if statement in Java, and you can make the compiler warn you about accidental assignments.

Still, assigning a boolean rather than comparing it is still possible and, possibly, a source of bugs.

This is the same procedure (or rather should be) in PHP. Also, better to switch to === or !== for exact comparisons since that also checks type.

(I'll leave out the other warts of PHP since that's been discussed here and elsewhere ad nauseum)

Good point i always wondered why people write it like that it makes it harder to read.

I see what you're saying, but (0 == current->uid) still makes my eyes bleed

I agree with you, but it works and it's saved me hours and hours of crying in front of gdb.

Please don't do that...

That what? Why not?

Writing (0 == a) instead of (a == 0) to check a's equality with 0.

It sacrifices readability. I'm sure there are tools (if not one can easily write one) that warn you for accidental assignment when comparison was meant instead.

Moreover, the (0 == a) trick only works for constants (which you can't assign values to, hence the compiler will complain), but not for variables. I.e. you can still do if (a = b) accidentally, when comparison was intended.

Not familiar with Linux kernel development practices, but wouldn't have any semi-capable static analysis tool caught this?

Yes, nowadays gcc warns you about assignments in conditionals if you enabled it (e.g. with -Wall). But it probably didn't in 2003.

This assignment is wrapped in parentheses. GCC will not issue a warning.

> wouldn't have any semi-capable static analysis tool caught this?

Using tools to find "errors" can be problematic. see, for example, the Debian random number bug. (https://www.schneier.com/blog/archives/2008/05/random_number...)

> These lines were removed because they caused the Valgrind and Purify tools to produce warnings about the use of uninitialized data in any code that was linked to OpenSSL.

Original thread: http://marc.info/?t=106806555100004&r=1&w=2

Reply on news.ycombinator.com