Comment by thrownaway2424 | Hacker News Reader

Comment by thrownaway2424 | Hacker News Reader

Demand unit tests and comments. I mean, I was incensed by the quality of the Apple code, but this code is just as bad:

  * Checks if the issuer of a certificate is a
  * Certificate Authority, or if the certificate is the same
  * as the issuer (and therefore it doesn't need to be a CA).
  *
  * Returns true or false, if the issuer is a CA,
  * or not.
  */
  static int
  check_if_ca (gnutls_x509_crt_t cert, gnutls_x509_crt_t issuer,
             unsigned int flags)
  {

Sure, returns "true" or "false" which are not things in C. This actually returns either 0 or 1. Many C library functions return 0 for OK and non-zero for error. This one returns 0 for error and 1 for OK.

Imagine you are reviewing the a call site of this code:

  if (check_if_ca(...)) { 
    // CA is valid, proceed
  }

vs.

  if (check_if_ca(...) == 0) {
    // CA is valid, proceed
  }

To the reviewer who is accustomed to strcmp, either of these might appear to be fine, meaning that the reviewer (or even the author) has to carefully check the documentation of this function.

What else is wrong with this signature? For one thing it would be easy to swap the arguments around:

  if (check_if_ca(cert, issuer, flags)) ...
  if (check_if_ca(issuer, cert, flags)) ...

What's the difference? They both look reasonable. What are the valid/relevant values of |flags|?

Would it be easier to comprehend |flags| if it were an options struct, e.g.

  GNUTLSCertificateVerificationOptions opts;
  opts.set_allow_rsa_md2(true);
  ...
  cert.Verify(opts);

?

Would it be easier to verify the correctness of a call site of check_if_ca if it looked like this:

  if (issuer_cert.IsCA() || cert == issuer_cert) ...

Where are the unit tests. The unit tests should be right here in the lib directory as whatever_unittest.c, adjacent to the source file they are testing, so you don't have to go groping around in the tree trying to find the relevant test, if it even exists. By the way, I found no test that calls this function.

Basically this code has all the same problems as the Apple code, except for the mixed tabs/spaces stuff (as far as I can tell). It would be shameful for any professional programmer or organization of programmers to create this code.

> Sure, returns "true" or "false" which are not things in C

#include <stdbool.h>

Available since C99.

If anything that likely makes the documentation's ambiguity even worse, though.

It's a strange thing, but C99 features seem very slow to catch on in the C community, particularly features conceived specifically to alleviate bad "C-isms". The only explanations I can think of are ignorance (not really an excuse after well over a decade) or possibly a subconscious fear that they somehow make a program "less portable", even though basically every compiler supports it (and besides, I've never noticed any aversion to trivial features like //-style comments).

[EDIT - maybe pride in the bad C-isms?]

This mostly due to slow support from the compiler people. 10 years after neither `clang` or `gcc` provide a complete C99 implementation [1].

[1] http://en.wikipedia.org/wiki/C99

I think that's a bit dishonest -- the standard is almost entirely supported by GCC:

http://gcc.gnu.org/c99status.html

> C99 is substantially completely supported as of GCC 4.5 (with -std=c99 -pedantic-errors used), modulo bugs, extended identifiers (supported except for corner cases when -fextended-identifiers is used), and floating-point issues (mainly but not entirely relating to optional C99 features from Annexes F and G).

... and also by clang:

http://clang.llvm.org/docs/UsersManual.html#c

> The support for standard C in clang is feature-complete except for the C99 floating-point pragmas.

Compiler/library support for C++ is much less complete(standards were published in 1998, 2003 and 2011, and none are completely implemented in any compiler) but that clearly didn't stop people from using C++. The reason is that the commonly supported subset was useful enough. The same is true of C99.

I think it was mostly Microsoft that was holding C99 adoption back by refusing to support it in their compiler suite (officially, they support C++ and C89/C90 only). That means you can't generally compile C99 code with the Microsoft C/C++ compiler, which meant you had to avoid either that compiler or (if you care about portability) the C99 standard. I don't think limited C99 support in GCC or Clang was a limiting factor to anyone in the past decade.

Compiler/library support for C++ is much less complete(standards were published in 1998, 2003 and 2011, and none are completely implemented in any compiler)

Clang begs to differ on that point http://clang.llvm.org/cxx_status.html

and gcc claims to implement "all of the major features" so you may be right on that one http://gcc.gnu.org/gcc-4.8/cxx0x_status.html

At any rate C++11 features are quite well supported IMO. Even VC++ is catching up.

> Clang begs to differ on that point http://clang.llvm.org/cxx_status.html

Impressive! I see that exported templates got scrapped in the latest standard -- they probably never supported that.

In any case, my point wasn't that Clang was bad (I think Clang is very good) but that the few limitations that might exist in Clang or GCC are probably not holding back C99 adoption.

I'll just note, that after coming back to C after a while in ruby/python/ungh, perl land everything in C99 and C11 has been a welcome change.

Seeing lack of adoption to things like just stdbool.h or even C99 initializer syntax is somewhat amusing. I understand backwards compatibility and all but there seems a general unwillingness to abandon C89 which I can only think is due to the microsoft toolchain.

As to c++ support clang has been really spearheading the implementation of new c++ standards and last I recall they even found bugs or inconsistencies. It always pays to have at least one implementation before standardizing I think.

I'm not sure which features you're thinking of. Some are no-brainers: snprintf() is a clear win, and in my experience, it's widely used. I also really like the ability to define structured objects inline using named members, sort of like the way you do in JavaScript. That can make code a lot more readable.

A lot of the rest is just sugar (e.g., C++-style comments). Moreover, many code bases already evolved patterns for dealing with the problems that many C99 features are supposed to address. Booleans are a good example: many environments already have a fine boolean_t definition, and it's not really worth it to convert existing code or introduce a second pattern for representing boolean values.

Variable-length arrays are another good example. They're fine, but alloca() is a perfectly reasonable solution to the same problem, and it's already widely used. IMO, the safety issues usually brought up around alloca() are neither more likely nor more serious than problems like allocating a gigabyte-sized object on the stack or returning a pointer to stack memory. Since competent C programmers don't generally make those mistakes (IME, of course -- I think I've never debugged a problem that turned out to be a misuse of alloca()), I find the added complexity of a new language primitive isn't worth it. (Incidentally, that's the same reason I dislike nearly all of C++.)

What's necessary is to get at least two experienced cryptographers thinking about every piece of security-critical code, because that's what will prevent bugs. It's also why security audits are so expensive: they work.

The code absolutely should have had clearer comments, since if every step were commented then some of the return value confusion may have been avoided. The style of the code itself probably isn't as terrible as it seems at first glance, though.

It's also not easy to write useful unit tests for a security stack.

> What's necessary is to get at least two experienced cryptographers thinking about every piece of security-critical code

Both of the recent bugs do not require a cryptographer to find. They are just logic bugs that any reasonable unit test would find.

If not expert cryptographers, than at least expert C programmers. I was trying to be diplomatic, but blunt is perhaps better for clarity: no expert C programmer should have any trouble reading and comprehending the syntax of the code in question, which is mainly what the parent poster was calling out. https://www.gitorious.org/gnutls/gnutls/commit/0fba2d908da6d...

It's probably tempting to think that unit testing might save us from security failures. People have written many words on both sides. But how many of us have actually tried it? No one who hasn't tried (and tried hard) to see whether unit testing works has any business arguing for (or against) unit testing. We simply don't have enough data to know whether it's valuable. And the fact that unit testing works in domains other than security isn't evidence that unit testing would produce any kind of benefit in computer security.

What's getting lost in the noise here, I think, is that it's important to zoom out and think about a broad overview of how difficult compsec is. Think about all the different situations that require security. Even something as seemingly simple and straightforward as an OS-level copy-paste mechanism requires thinking about security.

Could you write unit tests to cover all security situations? How about all possible situations? If not all possible situations, then why? What's the difference between a situation that you can unit test and one that you can't? Is it possible to write a unit test that checks whether there's a sequence of steps a user could take to exploit an OS-level copy-paste mechanism? Can you unit test the efficacy of a TCP/IP stack? Is it possible to unit test whether the Debian OpenSSL PRNG is being fed with sufficient bits of entropy? Can you unit test whether the colors of your website are pleasing?

The most interesting answer to those questions isn't "yes" or "no." The most interesting answer is "maybe." Because "maybe" means nobody knows, and that's fundamentally interesting. It's possible that it's possible to write unit tests for domains that nobody has thought of yet. (Yes, even tests that automatically infer whether your website's colors are pleasing to humans.)

Could unit testing have saved us from this one particular case? Maybe. But is it possible to write unit tests in the general case? Maybe not. But you, I, and everyone else doesn't know until we actually try. It could be impossible, or if not impossible then infeasible, or if not infeasible in a dynamic language then infeasible in C libraries. On the flipside, if you investigate, and you come up with a way of writing unit tests in the general case for security libraries, then you may have just discovered a breakthrough in how cryptographers can write security libraries. Because if it's possible to write useful unit tests in the general case for security libraries, then you will have demonstrated something that most cryptographers generally disbelieve, the same way that people disbelieve that it's possible to unit test whether colors are pleasing.

So get to work! There's nobody stopping anyone from right now going and trying to write those unit tests. Try it, write a blog post about your experience and the hardships, write about what you've learned. I guarantee it will be interesting.

All I'm saying is that we should be experienced before we advocate or dismiss an idea, and the only way to get experience is to try it out.

I haven't looked at the gnutls code in depth, but I looked at Apple's 'goto fail' code. I have a write-up here: http://pgcode.blogspot.de/2014/02/lots-has-been-said-about-a...

Cryptographic code introduces new concerns (e.g. is the code safe from timing attacks, or leaking information via cache usage,...). But it is also still code, and susceptible to common code errors. Both this bug and the Apple one are simple coding errors.

It is definitely possible to write unit tests that can help catch errors like this. I wrote some unit tests for the Apple code while refactoring it. Anyone trying to write unit tests to cover all execution paths in SSLVerifySignedServerKeyExchange would have caught that bug (since there was unreachable code). I suspect the same is the case here.

The bigger take-away for me was that the code could be improved a lot. If you have a look at the code after my refactoring, it is dramatically simpler, in my opinion.

Your blog post is an excellent example of how code like this should be cleaned up.

Unit testing is not terra incognita, it's industry best practice. Personally I write at least twice as many lines of code in tests as I do in the implementation under test. That's the nature of test-driven development.

If you doubt that TLS can be tested, may I direct your attention to the extensive suite of unit tests in Go's crypto package. For example http://golang.org/src/pkg/crypto/x509/x509_test.go and http://golang.org/src/pkg/crypto/x509/verify_test.go among many other source files.

Nobody claimed that we can fix all security problems with unit testing. Unit testing should be used in addition to auditing, reviewing, and other techniques. However, unit testing can dramatically improve the quality of code, including avoiding security pitfalls and mistakes.

What we need to do is to include mandatory unit testing in all security-critical code, and ensure good coverage.

This particular bug is a bug that would have easily been caught by anyone writing rudimentary unit tests.

It's sad that there are no regression tests for these changes either, meaning we remain susceptible to such bugs in the future.

Pedantic point: A unit test would not have found these. Presumably in a unit test you would mock check_if_ca. Only an integration test would have found this.

Sorry for picking on your comment in particular. I could have replied to any number of comments in this thread.

If you require comments to try to help programmers follow a confused set of return value conventions, I submit to you that comments aren't the real solution, since they're minimally helpful in addressing the real problem.

THIS^

Incidentally, would this bug have been flagged by a compiler warning?

I don't think so. The compiler would have no idea that 1 and 0 were not the 1 and 0 that the human was expecting, surely?

In C, no. Even an enum type in C can be compared with integers without any warnings.

On the other hand you could say that this function returning 'false' isn't really an error, and that the idiom is similar to the isdigit(), isalpha() etc type functions in ctype.h. And fwiw, bool is a thing in C99 with stdbool.h, although this function returns an int.

Reply on news.ycombinator.com