6 ms·

Unsure if Property Testing would actually have found this issue, 2 billion bytes is a very large string and usually you wouldn't put the limit so high, the test

Unsure if Property Testing would actually have found this issue, 2 billion bytes is a very large string and usually you wouldn't put the limit so high, the tests would run for too long. Fuzzing would be a more appropriate measure to have found this issue, as usually you do fuzzing with no limit of length/size, as that's the entire point of fuzzing to expand the scope of what you're testing. Regarding test coverage, I'd argue that even caring about test coverage is giving people a false sense of security. It's easy to achieve 100% test coverage, but that doesn't actually say that you're testing the right/wrong thing, just that at one point that line of code was executed, but again, that says nothing about correctness.

ajross 4y ago
Yeah, exactly. People tend to misunderstand fuzzing, and think of it as "feeding random garbage at an API". Fuzzing, done with proper tooling, is automated coverage testing. You have a bounds overflow that depends on input data but only if condition A and B and C are true in the runtime state? Fuzzing will find it, almost always, by detecting the branching behavior of your code. So all the replies here saying "the tests didn't find it because they didn't cover the case" are missing the point. There are test paradigms that don't rely on humans to enumerate cases.
shagie 4y ago
> Fuzzing would be a more appropriate measure to have found this issue SQLite has some fairly extensive fuzzing. https://www.sqlite.org/testing.html#fuzz_testing https://www.sqlite.org/testing.html#fuzz_testing One of the sections on Fuzz testing: > 4.3 Boundary Value Tests > SQLite defines certain limits on its operation, such as the maximum number of columns in a table, the maximum length of an SQL statement, or the maximum value of an integer. The TCL and TH3 test suites both contains numerous tests that push SQLite right to the edge of its defined limits and verify that it performs correctly for all allowed values. Additional tests go beyond the defined limits and verify that SQLite correctly returns errors. The source code contains testcase macros to verify that both sides of each boundary have been tested.
- chasil 4y ago
  It sounds from the article that "-fstack-protector-all" might remove everything except the denial of service. "...arbitrary code execution is confirmed when the library is compiled without stack canaries, but unconfirmed when stack canaries are present, and denial-of-service is confirmed in all cases." Everything should be compiled with all available compiler defenses: https://manpages.debian.org/testing/devscripts/hardening-check.1.en.html https://manpages.debian.org/testing/devscripts/hardening-che...
  - chasil 4y ago
    It does appear safe with the OS package on CentOS 7. $ hardening-check /usr/lib64/libsqlite3.so.0.8.6 /usr/lib64/libsqlite3.so.0.8.6: Position Independent Executable: no, regular shared library (ignored) Stack protected: yes Fortify Source functions: yes (some protected functions found) Read-only relocations: yes Immediate binding: no, not found!
marcosdumay 4y ago
> Unsure if Property Testing would actually have found this issue, 2 billion bytes is a very large string and usually you wouldn't put the limit so high, the tests would run for too long. Yet, this is a textbook case of issues caught by static analysis.
adrianN 4y ago
It is absolutely not easy to achieve the kind of coverage that sqlite has. You need massive amounts of tests for that. Of course coverage is not sufficient, you can exercise code without checking any properties at all. But to say that it gives a false sense of security is too harsh.
- capableweb 4y ago
  > It is absolutely not easy to achieve the kind of coverage that sqlite has. It is though. Give me one project and I'll send you a test suite that "tests" 100% of executable lines. It won't have any actual assertions, but that's not what code coverage is about anyways, so hopefully you won't mind. Number of assertions per line of code is about as good metric for how well tested something is as code coverage, which means just about nil.
  - Someone 4y ago
    > Give me one project and I'll send you a test suite that "tests" 100% of executable lines. I don’t think you can do that with every project. For example, a function f may do: if (p == null) { abort(); } If all callers also do that test, there’s no way to hit that abort() line in the program. Your test suite may not be able to hit it either. Maybe f is a private function, or that test occurs on one of its intermediate values, or f accidentally has two identical tests: if (p == null) { abort(“p is null”); } if (p == null) { // copy-paste bug abort(“q is null”); } if (r == null) { abort(“r is null”); } Even if it is possible to hit that line, finding a path through the code that passes in a null value can be extremely hard. As a simple contrived counterexample, try getting 100% coverage on a function that does if(secure_hash(foo) == 0) bar();
    - phoe-krk 4y ago
      > Your test suite may not be able to hit it either. Minor nitpick, process terminations are unit-testable in general; see e.g. https://github.com/google/googletest/blob/main/docs/advanced.md#death-tests https://github.com/google/googletest/blob/main/docs/advanced...
pfdietz 4y ago
Coverage testing is just a special case of a more general concept: mutation testing (where the mutation is "if you reach this line/branch, blow up.") Perhaps mutation testing could have revealed the test suite was lacking in a test detecting that bug (for example, a mutation that changed a type from unsigned to signed or vice versa.)
- Quekid5 4y ago
  I'm so sad that so few test frameworks/libraries offer mutation testing. (And its profiling cousin Causal Profiling.) Admittedly the some languages might make them very hard to do, but the payoff would be so, so sweet.
  - taeric 4y ago
    My team at work took it out of our product. :( I couldn't make strong arguments that it was providing value, as it had turned into the slow test that always passed, by the time new folks joined. I do think it has a lot of merit, though.
    - pfdietz 4y ago
      To be clear... mutation testing is a way to assess the power of a test suite, not a way to test software itself (in this it is analogous to using statement/branch coverage to assess a test suite). Do you mean the tests added to achieve high mutation coverage were not worth the time it took to run them? The mutation process itself is not something that would be part of CI. Doing all that mutation and generating the tests to kill non-equivalent mutants is very time consuming, I will admit.
      - taeric 4y ago
        On the code base in question, the full but with mutation tests was only 8 minutes. Not snappy, but hardly a disaster.
        4y ago
admax88qqq 4y ago
Boundary Value tests are standard practice when doing extensive testing. Bugs rarely occur for typical input values but more often occur at the "boundaries" or limits of the input domain. That being said I don't really fault SQlites testing here. The key takeaway for me is that even with exhaustive testing you can still have security issues like this, so perhaps what is needed is a language or methodology change. If SQLite, a quality codebase with exhaustive tests, can have memory vulnerabilities, maybe we need to leave C behind.
- ghoward 4y ago
  SQLite is used in places that Rust has no compiler for and that no other language has a compiler for. One severe CVE in about 20 years is no reason to rewrite in another language and remove support for some platforms.
  - admax88qqq 4y ago
    I'm not saying we need to rewrite SQLite and I'm not sure why everyone is assuming I suggested rust. But people need to consider how much testing SQLite did and still hit memory vulnerabilities when they consider picking up C for a new project.
  - nicoburns 4y ago
    In the long term, I hope C stops being the default language that hardware vendors provide for their hardware.
    - ghoward 4y ago
      Me too, but I also hope Rust is not that default language.
  - threatofrain 4y ago
    Just how many machines are we talking about? Does anyone have estimates?
    - 4y ago
jonhohle 4y ago
> It's easy to achieve 100% test coverage If code hasn’t been written to be testable, 100% coverage is not _easy_, by any stretch. Any legacy code base written without unit tests that I’ve come across required significant refactoring to get any basic assertions I place, let alone any reasonable coverage. I also think there is some value in high coverage, even without assertions. At a minimum it’s a guarantee that code has been run before production/distribution. That’s probably a lot more than can be said about a lot of code.

ajross 4y ago

Yeah, exactly. People tend to misunderstand fuzzing, and think of it as "feeding random garbage at an API". Fuzzing, done with proper tooling, is automated coverage testing. You have a bounds overflow that depends on input data but only if condition A and B and C are true in the runtime state? Fuzzing will find it, almost always, by detecting the branching behavior of your code. So all the replies here saying "the tests didn't find it because they didn't cover the case" are missing the point. There are test paradigms that don't rely on humans to enumerate cases.

shagie 4y ago

> Fuzzing would be a more appropriate measure to have found this issue SQLite has some fairly extensive fuzzing. https://www.sqlite.org/testing.html#fuzz_testing https://www.sqlite.org/testing.html#fuzz_testing One of the sections on Fuzz testing: > 4.3 Boundary Value Tests > SQLite defines certain limits on its operation, such as the maximum number of columns in a table, the maximum length of an SQL statement, or the maximum value of an integer. The TCL and TH3 test suites both contains numerous tests that push SQLite right to the edge of its defined limits and verify that it performs correctly for all allowed values. Additional tests go beyond the defined limits and verify that SQLite correctly returns errors. The source code contains testcase macros to verify that both sides of each boundary have been tested.

chasil 4y ago
It sounds from the article that "-fstack-protector-all" might remove everything except the denial of service. "...arbitrary code execution is confirmed when the library is compiled without stack canaries, but unconfirmed when stack canaries are present, and denial-of-service is confirmed in all cases." Everything should be compiled with all available compiler defenses: https://manpages.debian.org/testing/devscripts/hardening-check.1.en.html https://manpages.debian.org/testing/devscripts/hardening-che...
- chasil 4y ago
  It does appear safe with the OS package on CentOS 7. $ hardening-check /usr/lib64/libsqlite3.so.0.8.6 /usr/lib64/libsqlite3.so.0.8.6: Position Independent Executable: no, regular shared library (ignored) Stack protected: yes Fortify Source functions: yes (some protected functions found) Read-only relocations: yes Immediate binding: no, not found!

marcosdumay 4y ago

> Unsure if Property Testing would actually have found this issue, 2 billion bytes is a very large string and usually you wouldn't put the limit so high, the tests would run for too long. Yet, this is a textbook case of issues caught by static analysis.

adrianN 4y ago

It is absolutely not easy to achieve the kind of coverage that sqlite has. You need massive amounts of tests for that. Of course coverage is not sufficient, you can exercise code without checking any properties at all. But to say that it gives a false sense of security is too harsh.

capableweb 4y ago
> It is absolutely not easy to achieve the kind of coverage that sqlite has. It is though. Give me one project and I'll send you a test suite that "tests" 100% of executable lines. It won't have any actual assertions, but that's not what code coverage is about anyways, so hopefully you won't mind. Number of assertions per line of code is about as good metric for how well tested something is as code coverage, which means just about nil.
- Someone 4y ago
  > Give me one project and I'll send you a test suite that "tests" 100% of executable lines. I don’t think you can do that with every project. For example, a function f may do: if (p == null) { abort(); } If all callers also do that test, there’s no way to hit that abort() line in the program. Your test suite may not be able to hit it either. Maybe f is a private function, or that test occurs on one of its intermediate values, or f accidentally has two identical tests: if (p == null) { abort(“p is null”); } if (p == null) { // copy-paste bug abort(“q is null”); } if (r == null) { abort(“r is null”); } Even if it is possible to hit that line, finding a path through the code that passes in a null value can be extremely hard. As a simple contrived counterexample, try getting 100% coverage on a function that does if(secure_hash(foo) == 0) bar();
  - phoe-krk 4y ago
    > Your test suite may not be able to hit it either. Minor nitpick, process terminations are unit-testable in general; see e.g. https://github.com/google/googletest/blob/main/docs/advanced.md#death-tests https://github.com/google/googletest/blob/main/docs/advanced...

pfdietz 4y ago

Coverage testing is just a special case of a more general concept: mutation testing (where the mutation is "if you reach this line/branch, blow up.") Perhaps mutation testing could have revealed the test suite was lacking in a test detecting that bug (for example, a mutation that changed a type from unsigned to signed or vice versa.)

Quekid5 4y ago
I'm so sad that so few test frameworks/libraries offer mutation testing. (And its profiling cousin Causal Profiling.) Admittedly the some languages might make them very hard to do, but the payoff would be so, so sweet.
- taeric 4y ago
  My team at work took it out of our product. :( I couldn't make strong arguments that it was providing value, as it had turned into the slow test that always passed, by the time new folks joined. I do think it has a lot of merit, though.
  - pfdietz 4y ago
    To be clear... mutation testing is a way to assess the power of a test suite, not a way to test software itself (in this it is analogous to using statement/branch coverage to assess a test suite). Do you mean the tests added to achieve high mutation coverage were not worth the time it took to run them? The mutation process itself is not something that would be part of CI. Doing all that mutation and generating the tests to kill non-equivalent mutants is very time consuming, I will admit.
    - taeric 4y ago
      On the code base in question, the full but with mutation tests was only 8 minutes. Not snappy, but hardly a disaster.
      - 4y ago

admax88qqq 4y ago

Boundary Value tests are standard practice when doing extensive testing. Bugs rarely occur for typical input values but more often occur at the "boundaries" or limits of the input domain. That being said I don't really fault SQlites testing here. The key takeaway for me is that even with exhaustive testing you can still have security issues like this, so perhaps what is needed is a language or methodology change. If SQLite, a quality codebase with exhaustive tests, can have memory vulnerabilities, maybe we need to leave C behind.

ghoward 4y ago
SQLite is used in places that Rust has no compiler for and that no other language has a compiler for. One severe CVE in about 20 years is no reason to rewrite in another language and remove support for some platforms.
- admax88qqq 4y ago
  I'm not saying we need to rewrite SQLite and I'm not sure why everyone is assuming I suggested rust. But people need to consider how much testing SQLite did and still hit memory vulnerabilities when they consider picking up C for a new project.
- nicoburns 4y ago
  In the long term, I hope C stops being the default language that hardware vendors provide for their hardware.
  - ghoward 4y ago
    Me too, but I also hope Rust is not that default language.
- threatofrain 4y ago
  Just how many machines are we talking about? Does anyone have estimates?
  - 4y ago

jonhohle 4y ago

> It's easy to achieve 100% test coverage If code hasn’t been written to be testable, 100% coverage is not _easy_, by any stretch. Any legacy code base written without unit tests that I’ve come across required significant refactoring to get any basic assertions I place, let alone any reasonable coverage. I also think there is some value in high coverage, even without assertions. At a minimum it’s a guarantee that code has been run before production/distribution. That’s probably a lot more than can be said about a lot of code.