0.1.5 (older version) Thoroughness: Medium Understanding: Medium
by HeroicKatora on 2020-01-07
This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev.
The current version of secret_integers is 0.1.7.
Lib.rs has been able to verify that all files in the crate's tarball, except Cargo.lock,
are in the crate's repository. Please note that this check is still in beta, and absence of this confirmation does not mean that the files don't match.
Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories, so there is a possibility that published crates have a misleading repository URL, or contain different code from the code in the repository.
To review the actual code of the crate, it's best to use cargo crev open secret_integers. Alternatively, you can download the tarball of secret_integers v0.1.7 or view the source online.
The crate contains no comments on how the supposed constant time operations are actually achieved. There is no mention of a memory model, no discussion of references, black boxes, speculation, optimization, etc. And on top of these theoretical shortcomings the crate also contains no tests to verify any of their assumptions. This means: no
asmoutput review, no smoke tests for operation's time dependencies and a general lack of even functional tests.The
classifymethod is just a glorified constructor and even contains a generic which will in general have no guarantees and is totally misplaced here. Anddeclassifydoesn't really do anything since the whole type state can always be dereferenced into the underlying type if the compiler pleases (and, oh, it will).An unexplained fill-horn of converters is added. Since they do not perform any more robust sequencing or basic optimization barriers, one might as well use the
classifyconstructor instead. They are even declared#[inline]!Some allocating to-bytes converters are offered. Despite plainly leaking all values into the global heap without any protection or zeroing after expiration, they have no documentation, unlike much less dangerous methods.
Despite the fact that several operations are disallowed as a 'security feature', an inconsistent other set is allowed. Overflow checked operations will, almost by definition, at least contain conditional moves if compiled naively using the built-ins. But for some unexplained reason, the division operation of all is untrusted!
Many other such ops -- with invalid and panicking inputs or similar -- exist: Rotations will probably do a branch on the input value, additions overflow check and complain etc. in debug code. Maybe the bitwise operations are not totally broken.
WORST! Some operations are transliterated from WireGuard. But they contain TYPOS that plainly contradict newly introduced variable names. How do they even work? It is not explained.
By the way, the code is copied from a GPL licensed project (WireGuard) but the crate itself LOOKS like an MIT license but actually mentions APACHE within the text. So this might also be a licensing risk for the future.