search

jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.15k stars 1.73k forks source link

libsodium under TrustInSoft CI #995

Closed jakub-zwolakowski closed 3 years ago

jakub-zwolakowski commented 3 years ago

Hi @jedisct1,

I initiated the configuration of libsodium on the new tool TrustInSoft CI.

It's a source code analyzer, which analyzes execution paths (usually unit tests in your repo) to detect Undefined Behaviors along the way. Coverage includes all the defects that sanitizers ASAN, UBSan and MSan find, plus a large number of other (usually subtle) defects.

In the libsodium case, it's a good news as no Undefined Behaviors were found on the 67 analyzed execution paths (please ignore the one in red): https://ci.trust-in-soft.com/projects/jakub-zwolakowski/libsodium/3?page=1

These execution paths correspond to the tests in the /test/default folder. Each test is described in an entry of this JSON file: https://github.com/jakub-zwolakowski/libsodium/blob/tis/tis.config

Now, I can see several potential next steps. One is to run the analyses under relevant target architectures other than x86_64. For example, this issue shows a (minor) problem on the bitcoin-core/ctaes project only occurring on 16-bit architectures.

1 ) Before I get too far, is this the kind of experiment that you'd be open to?

2 ) I'm also wondering whether other projects such as libhydrogen might be more suited to this kind of test.

Thanks!

jedisct1 commented 3 years ago

Hi Jakub!

And congrats for that new tool, which is going to make TIS way easier to use!

Having the TIS CI, and more generally more static analysis is definitely a good thing!

Does it run automatically after every commit, or is there some GitHub integration to be done?

Having it run on libhydrogen as well would be even better.

Thanks for this!

jakub-zwolakowski commented 3 years ago

Thanks for your answer!

We've indeed built TIS CI in a way to make TIS easier to use. So please let us know if you encounter any difficulties using it, such feedback is very important for us!

Answering your question: as soon as the project is "added" to TIS CI, it will run automatically after every commit pushed to any branch which contains the tis.config file at the repository root.

As there's still some configuration to be done (the tis.config file notably), would you be open to a Pull Request from our side that contains this initial configuration?

We would describe all the configuration and the steps to add the project to TIS CI directly in the PR.

To get started, you could merge the PR on a test branch and experiment on it.

Thanks!

jedisct1 commented 3 years ago

Does the tls.config file need to be manually created, and more importantly, kept up to date every time a change is made to the files structure?

Not a big deal, but this looks significantly more complicated than other products that include wrappers to do it automatically (such as scan-build).

jakub-zwolakowski commented 3 years ago

Yes, for now, the tis.config file needs to be manually created and kept up-to-date with the files structure. Do you expect it to change quite so often?

guillaumemillot commented 3 years ago

Hi @jedisct1

I'm working with @jakub-zwolakowski on TIS CI.

To show you a typical TIS CI configuration, I've actually just sent you PR https://github.com/jedisct1/libhydrogen/pull/99 on project libhydrogen.

The PR covers 6 of your tests across 4 architectures (x86/ppc for little/big-endian x 32/64-bit) and it includes in the description all the steps to get started.

Again, no undefined behaviors or defects have been found so congrats!

If you want to see how it would look like otherwise, this is a BLAKE3 example, where the digest that is computed on big-endian isn't the expected one for some messages:

Can you take a look at the PR and let us know what you think?

Thanks!

guillaumemillot commented 3 years ago

@jedisct1 I've just sent you PR https://github.com/jedisct1/libsodium/pull/1003 with an initial TIS CI configuration for libsodium.

I wasn't sure which of the tests in the /test/default folder were most relevant to cover with TIS CI, so I included 56 of them (out of 80) as a first iteration: https://ci.trust-in-soft.com/projects/guillaumemillot/libsodium/1

I have a few questions, sorry, I'm not an crypto expert so I hope I didn't make any silly mistake in the configuration:

  1. libsodium requires a random stream: for the TIS CI tests, I replaced the SYS_getrandom system call in the getrandom(B,D,F) macro with a deterministic function tis_getrandom(). => Is that a good (or bad..) way to do it and are the tests still relevant? => Another way to do it would be to read from a file, that contains data extracted from /dev/urandom.
  2. To reduce the analysis time, I shortened 7 tests: their TIS CI names include "short". => Do you mind taking a quick look at those tests in the diff and telling me if they are still relevant?
  3. Now a "prospective" question: is there any test/part or configuration of your library that you think would be interesting and useful to analyze in more details with TIS CI? That could be for example more sensitive parts (in case there are parts which are more sensitive than others), recent/upcoming additions to your library so potentially less well tested or some exotic target architectures.

I focused the analysis on a single target architecture: x86-64. Once the configuration is correct, it will be possible to extend the analysis with more architectures: https://docs.ci.trust-in-soft.com/reference/supported-architectures#list

Thanks and looking forward to your insights!