5 ms·

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 certifica

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.

sillysaurus3 13y ago
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.
- onedognight 13y ago
  > 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.
  - sillysaurus3 13y ago
    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/0fba2d908da6d0df821991ea5fdbeeda0f4ff089 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.
epsylon 13y ago
> Sure, returns "true" or "false" which are not things in C #include <stdbool.h> Available since C99.
- leoc 13y ago
  If anything that likely makes the documentation's ambiguity even worse, though.
- dTal 13y ago
  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?]
  - spiralpolitik 13y ago
    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 http://en.wikipedia.org/wiki/C99
    - sltkr 13y ago
      I think that's a bit dishonest -- the standard is almost entirely supported by GCC: http://gcc.gnu.org/c99status.html 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 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.
adsr 13y ago
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.

sillysaurus3 13y ago

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.

onedognight 13y ago
> 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.
- sillysaurus3 13y ago
  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/0fba2d908da6d0df821991ea5fdbeeda0f4ff089 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.

epsylon 13y ago

> Sure, returns "true" or "false" which are not things in C #include <stdbool.h> Available since C99.

leoc 13y ago
If anything that likely makes the documentation's ambiguity even worse, though.
dTal 13y ago
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?]
- spiralpolitik 13y ago
  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 http://en.wikipedia.org/wiki/C99
  - sltkr 13y ago
    I think that's a bit dishonest -- the standard is almost entirely supported by GCC: http://gcc.gnu.org/c99status.html 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 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.

adsr 13y ago

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.