You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fix [GHSA-2frx-2596-x5r6].
[GHSA-2frx-2596-x5r6]: GHSA-2frx-2596-x5r6
This uses the `sha1-checked` crate from the RustCrypto project. It’s
a pure Rust implementation, with no SIMD or assembly code.
The hashing implementation moves to `gix-hash`, as it no longer
depends on any feature configuration. I wasn’t sure the ideal
crate to put this in, but after checking reverse dependencies on
crates.io, it seems like there’s essentially no user of `gix-hash`
that wouldn’t be pulling in a hashing implementation anyway, so I
think this is a fine and logical place for it to be.
A fallible API seems better than killing the process as Git does,
since we’re in a library context and it would be bad if you could
perform denial‐of‐service attacks on a server by sending it hash
collisions. (Although there are probably cheaper ways to mount a
denial‐of‐service attack.)
The new API also returns an `ObjectId` rather than `[u8; 20]`; the
vast majority of `Hasher::digest()` users immediately convert the
result to `ObjectId`, so this will help eliminate a lot of cruft
across the tree. `ObjectId` also has nicer `Debug` and `Display`
instances than `[u8; 20]`, and should theoretically make supporting
the hash function transition easier, although I suspect further API
changes will be required for that anyway. I wasn’t sure whether
this would be a good change, as not every digest identifies an
entry in the Git object database, but even many of the existing
uses for non‐object digests across the tree used the `ObjectId`
API anyway. Perhaps it would be best to have a separate non‐alias
`Digest` type that `ObjectId` wraps, but this seems like the pragmatic
choice for now that sticks with current practice.
The old API remains in this commit, as well as a temporary
non‐fallible but `ObjectId`‐returning `Hasher::finalize()`,
pending the migration of all in‐tree callers.
I named the module `gix_hash::hasher` since `gix_hash::hash` seemed
like it would be confusing. This does mean that there is a function
and module with the same name, which is permitted but perhaps a
little strange.
Everything is re‐exported directly other than
`gix_features::hash::Write`, which moves along with the I/O
convenience functions into a new public submodule and becomes
`gix_hash::hasher::io::Write`, as that seems like a clearer name
to me, being akin to the `gix_hash::hasher` function but as an
`std::io::Write` wrapper.
Raw hashing is somewhere around 0.25× to 0.65× the speed of the
previous implementation, depending on the feature configuration
and whether the CPU supports hardware‐accelerated hashing. (The
more portable assembly in `sha1-asm` that doesn’t require the SHA
instruction set doesn’t seem to speed things up that much; in fact,
`sha1_smol` somehow regularly beats the assembly code used by `sha1`
on my i9‐9880H MacBook Pro! Presumably this is why that path was
removed in newer versions of the `sha1` crate.)
Performance on an end‐to‐end `gix no-repo pack verify` benchmark
using pack files from the Linux kernel Git server measures around
0.41× to 0.44× compared to the base commit on an M2 Max and a
Ryzen 7 5800X, both of which have hardware instructions for SHA‐1
acceleration that the previous implementation uses but this one does
not. On the i9‐9880H, it’s around 0.58× to 0.60× the speed;
the slowdown is reduced by the older hardware’s lack of SHA‐1
instructions.
The `sha1collisiondetection` crate from the Sequoia PGP project,
based on a modified C2Rust translation of the library used by Git,
was also considered; although its raw hashing performance seems
to measure around 1.12–1.15× the speed of `sha1-checked` on
x86, it’s indistinguishable from noise on the end‐to‐end
benchmark, and on an M2 Max `sha1-checked` is consistently
around 1.03× the speed of `sha1collisiondetection` on that
benchmark. The `sha1collisiondetection` crate has also had a
soundness issue in the past due to the automatic C translation,
whereas `sha1-checked` has only one trivial `unsafe` block. On the
other hand, `sha1collisiondetection` is used by both Sequoia itself
and the `gitoid` crate, whereas rPGP is the only major user of
`sha1-checked`. I don’t think there’s a clear winner here.
The performance regression is very unfortunate, but the [SHAttered]
attack demonstrated a collision back in 2017, and the 2020 [SHA‐1 is
a Shambles] attack demonstrated a practical chosen‐prefix collision
that broke the use of SHA‐1 in OpenPGP, costing $75k to perform,
with an estimate of $45k to replicate at the time of publication and
$11k for a classical collision.
[SHAttered]: https://shattered.io/
[SHA‐1 is a Shambles]: https://sha-mbles.github.io/
Given the increase in GPU performance and production since then,
that puts the Git object format squarely at risk. Git mitigated this
attack in 2017; the algorithm is fairly general and detects all the
existing public collisions. My understanding is that an entirely new
cryptanalytic approach would be required to develop a collision attack
for SHA‐1 that would not be detected with very high probability.
I believe that the speed penalty could be mitigated, although not
fully eliminated, by implementing a version of the hardened SHA‐1
function that makes use of SIMD. For instance, the assembly code used
by `openssl speed sha1` on my i9‐9880H measures around 830 MiB/s,
compared to the winning 580 MiB/s of `sha1_smol`; adding collision
detection support to that would surely incur a performance penalty,
but it is likely that it could be much more competitive with
the performance before this commit than the 310 MiB/s I get with
`sha1-checked`. I haven’t been able to find any existing work on
this; it seems that more or less everyone just uses the original
C library that Git does, presumably because nothing except Git and
OpenPGP is still relying on SHA‐1 anyway…
The performance will never compete with the >2 GiB/s that can
be achieved with the x86 SHA instruction set extension, as the
`SHA1RNDS4` instruction sadly runs four rounds at a time while the
collision detection algorithm requires checks after every round,
but I believe SIMD would still offer a significant improvement,
and the AArch64 extension seems like it may be more flexible.
I know that these days the Git codebase has an additional faster
unsafe API without these checks that it tries to carefully use only
for operations that do not depend on hashing results for correctness
or safety. I personally believe that’s not a terribly good idea,
as it seems easy to misuse in a case where correctness actually does
matter, but maybe that’s just my Rust safety bias talking. I think
it would be better to focus on improving the performance of the safer
algorithm, as I think that many of the operations where the performance
penalty is the most painful are dealing with untrusted input anyway.
The `Hasher` struct gets a lot bigger; I don’t know if this is
an issue or not, but if it is, it could potentially be boxed.
Closes: #585
0 commit comments