Hacker News Reader

Comment by londons_explore | Hacker News Reader

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?

View on HN

It is not so easy, it is a contest, and they show you the winners.

And if you look at the "Scoring and Extra Points" section of http://underhanded-c.org/_page_id_5.html you will notice that it checks most of the boxes.

It is short, errors based on human perception (here = vs ==) are good enough, it is innocent looking under syntax highlighting, is is not platform dependent, and it even passes the "irony" check. It is just the plausible deniability that is not great, but it is still defensible with a lot of bad faith.

now I'm wondering if syntax highlighting shouldn't somehow make an assignment inside an if statement (and the variants) a bright red, or something like that.

You may be interested in linters.

Or IDEs, many of which will continually lint code.

It should really be forbidden by the compiler these days, or at least a very loud warning.

It would break these kinds of constructs, which are common.

  if ((fd = open(...)) != -1) {
      /* do something with fd */
  } else {
      perror("open");
  }

The compiler outputs a warning when you have something like "if (a = b)". If that's what you mean (it sometimes is), you have to write it "if ((a = b))" to silence the warning.

TBF, that doesn't actually work - you have to write:

  int fd; /* fd must be declared in order to be assigned */
  if ((fd = open(...)) != -1) {
      /* do something with fd */
  } else {
      perror("open");
  }

which would be better written as:

  int fd = open(...);
  if (fd != -1) /* etc */

It would be nice for scoping reasons to be able to write something like:

  if ((int fd = open(...)) != -1) /* etc */

but

  if ((int fd == open(...)) != -1) /* etc */

isn't valid code.

Exactly my thoughts, I was about to comment on that but I was too lazy so I omitted the declaration. Obviously, this is HN, so someone had to point it out ;)

Anyways, I am a bit torn about the second option. I like the idea of putting the call inside the if clause as it makes for a very explicit error handling but the uninitialized declaration is ugly. What I do in practice tends to depend on the situation but it is rarely satisfying.

Your last suggestion would be ideal, but as you said, it is invalid code unfortunately.

Maybe this

  for (int fd = open(...); fd != -1; fd = -1) {
      /* do stuff */
  }

Just kidding, don't do that.

  > for (int fd = open(...); fd != -1; fd = -1) {

that doesn't correctly (or rather at all) handle the failure case. You should do (IIRC):

  #define LET(...) for(__VA_ARGS__,_tmp[0],*_once=_tmp; _once ;_once=0)
  /* ^^^ goes in a header file */
  LET(int fd = open())
    {
    if(fd == -1) { /* handle error, return or break */ }
    /* do stuff */
    }

C++17 has this! See https://en.cppreference.com/w/cpp/language/if.

Here's a classic:

  char *strcpy(char *d, char *s) {
      char *r = d;
      while (*d++ = *s++);
      return r;
  }

(If you don't need the return value, this becomes even nicer.)

What about:

while((i = getchar()) != EOF) putchar(i);

This type of thing seems to be "encouraged"...

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

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

This seems highly likely.

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.

Step 1: Get 3 pigs. Step 2: Number them 1, 2, and 4. ...

You've probably meant: if they had chosen something much subtler we wouldn't be talking about it.

GCC warns of assignment in conditional, even without -Wall or -pendantic. I don't know when it started doing that, but it seems like a sore thumb today, different in 2003 maybe?

It only warns if the assignment doesn't have an extra pair of parentheses. These were added in this case, to silence the warning (so the attack would not be noticed). The parentheses are also needed in this case to get the precedence right, but they won't be needed if '==' were written, so anyone coding this by accident would immediately be warned of the mistake.

Oh wow, I didn't know that. Sneaky.

Reply on news.ycombinator.com