Constant-time tests for libsecp256k1

[unseddd]

Tests for constant-time violations in a few functions taking secret data as input.

No violations found 🚀

[unseddd]

Instructions for generating LLVM bitcode, and running the test are in README_PITCHFORK.md.

[real-or-random]

Hi, this looks very interesting but can you provide some background? What's pitchfork? What does it do? How does it work?

[unseddd]

pitchfork is a crate for testing cryptographic libraries for constant-time violations. It is based on the haybale crate, a SAT solver written in Rust.

Basically, pitchfork parses LLVM bitcode, converts it (in this case) to boolector BitVectors, and checks for constraint satisfaction (here, that the code runs in constant-time).

These are still, ultimately, software checks. So, weird compiler optimizations, hardware side-channels, etc. could still lead to non-constant-time code. However, these checks are an extra assurance that the code is protected from timing side-channels.

[unseddd]

Looks like the CI environment would need to be changed to run pitchfork tests properly.

haybale requires llvm-sys and boolector, so LLVM(+headers) and boolector (C-lib) need to be installed as shared libraries. Is that something the project would be willing to add?

[elichai]

Hi,
First of all that's a cool project, and thanks for bringing it to out attention and contributing :)

Second, currently this doesn't do anything, because the function names are wrong, and inspecting the output of project.all_functions() it seems like the functions we actually want aren't there because of inlining and lack of monomorphism.

After fixing those and running, it fails on the C symbols, because doing cargo rustc -- -g --emit llvm-bc only emits llvm bytecode for the rust side, not the C side.
I might try later to generate llvm bytecode for the C part (using clang) and run this analysis on that and see what does it actually do

[real-or-random]

After fixing those and running, it fails on the C symbols, because doing cargo rustc -- -g --emit llvm-bc only emits llvm bytecode for the rust side, not the C side.
I might try later to generate llvm bytecode for the C part (using clang) and run this analysis on that and see what does it actually do

Independently of this, maybe upstream secp256k1 is a better place to run this in CI. Rust code here should never work on secrets (but pass it to the C code).

[elichai]

This is actually pretty cool,
I tested rustsecp256k1_v0_1_1_ecdsa_sign and got the following output:

Results for rustsecp256k1_v0_1_1_ecdsa_sign:

verified paths: 0
constant-time violations found: 1

(for detailed block-coverage stats, rerun with PITCHFORK_COVERAGE_STATS=1 environment variable.)

rustsecp256k1_v0_1_1_ecdsa_sign is not constant-time
First constant-time violation encountered:

`OtherError`: Constant-time violation: control-flow may be influenced by secret data

Backtrace:
  #1: {/home/elichai2/gits/rust-secp256k1/target/debug/deps/output.bc: rustsecp256k1_v0_1_1_ecdsa_sign, bb %81, terminator}
         (/home/elichai2/gits/rust-secp256k1/secp256k1-sys/depend/secp256k1/src/secp256k1.c, line 426, col 19)

note: For a dump of the path that led to this error, rerun with the environment variable `HAYBALE_DUMP_PATH` set to:
        `LLVM` for a list of the LLVM basic blocks in the path
        `SRC` for a list of the source-language locations in the path
        `BOTH` for both of the above
      To get source-language locations, the LLVM bitcode must also contain
      debuginfo. For example, C/C++ or Rust sources must be compiled with the
      `-g` flag to `clang`, `clang++`, or `rustc`.

note: For a dump of variable values at time of error, rerun with `HAYBALE_DUMP_VARS=1` environment variable.

note: to enable detailed logs, ensure that debug-level logging messages are visible.
  See the documentation for your chosen logging backend (e.g., env_logger, log4rs, etc)
  for help with configuration.

note: To enable debug-level logging messages when `progress_updates` is
      enabled in `PitchforkConfig`, use the `debug_logging` setting

And if we look at line 426 we see: !rustsecp256k1_v0_1_1_scalar_is_zero(&sec) (this is harmless and fixed in bitcoin-core/secp256k1#710)

@real-or-random idk, I think upstreams CI is already quite busy, and this library is only in rust so adding it here might be easier (if not I might set it up on a third party github)

[real-or-random]

@real-or-random idk, I think upstreams CI is already quite busy, and this library is only in rust so adding it here might be easier (if not I might set it up on a third party github)

I see that point. One problem is that this won't catch issues early enough, I think. We'll see the problem only when we update the secp256k1 subtree here (which is probably much later than the code was committed or even released upstream).

[elichai]

That's also fair, I wonder if I can do some travis cron job on a branch here or on a fork that will update to upstream everyday, run all the tests and run these ct tests. although merging to master in libsecp might also be too late? (and I will need to actually notice the Travis email saying it failed)

[real-or-random]

Yeah, but to be honest, I feel then it's easier to just add it upstream. Sure, the CI is large already but why not. I guess a rust dependency is not an issue when it's just in Travis. (Just my, we'd need to see upstream of course.)

[unseddd]

Yeah, but to be honest, I feel then it's easier to just add it upstream

@real-or-random @elichai I considered making the pull upstream, but did not know how they would feel about Rust code in a pure-C project. Happy to make a PR, and maybe put it under a contrib tree?

Second, currently this doesn't do anything, because the function names are wrong

Yeah, wasn't sure about if the C functions were coming through... re: your failing test on upstream, I had problems with false-positives behind references when testing subtle. Like you said though, already fixed upstream.

Thanks for the feedback 😄

[real-or-random]

Yeah, but to be honest, I feel then it's easier to just add it upstream

@real-or-random @elichai I considered making the pull upstream, but did not know how they would feel about Rust code in a pure-C project. Happy to make a PR, and maybe put it under a contrib tree?

To be honest, I don't know what the other contributors to upstream think about Rust code in the repo. I'm not sure whether I'd like that, but I'd be okay if it's just for the CI in some ci folder or something like this. Another question is whether we want to have a second way way to verify constant timeness given that we have valgrind tests already. AFAIU this here captures more traces. I can talk to them and/or create an issue there.

But before we talk about this, a much important issue is declassification, which we rely on with our current valgrind tests. Has anybody run the tests here successfully at all against the current master? For example, test_ct_sign() should actually fail, see PLSysSec/haybale-pitchfork#3 for context. We'd need to make this work, no matter if we want this here or upstream.

[elichai]

But before we talk about this, a much important issue is declassification, which we rely on with our current valgrind tests. Has anybody run the tests here successfully at all against the current master? For example, test_ct_sign() should actually fail, see PLSysSec/haybale-pitchfork#3 for context. We'd need to make this work, no matter if we want this here or upstream.

That's why I wanted to run this on master, but I didn't got to doing it, I hope I'll be able to do it today

[unseddd]

That's why I wanted to run this on master, but I didn't got to doing it, I hope I'll be able to do it today

I will also try running this against master, with the advice from above.