RustCrypto / hashes

Collection of cryptographic hash functions written in pure Rust
1.75k stars 238 forks source link

sha1-checked: `ubc_check` is not used #594

Closed newpavlov closed 2 days ago

newpavlov commented 2 weeks ago

It looks like due to an oversight the ubc_check module is not used. Calling the Builder::use_ubc has no effect on hash computation.

This results in CI failures.

cc @dignifiedquire

dignifiedquire commented 2 weeks ago

Ack. Will take a look in the next days

dignifiedquire commented 2 days ago

It looks like due to an oversight the ubc_check module is not used.

As far as I can tell it is very much used

Calling the Builder::use_ubc has no effect on hash computation.

It does, calling use_ubc, sets use_ubc to either true or false, resulting in this branch being triggered or not:

https://github.com/RustCrypto/hashes/blob/master/sha1-checked/src/compress.rs#L691

The actual dead code warnings I am seeing are the following:

   Compiling sha1-checked v0.11.0-pre (/Users/dignifiedquire/opensource/rustcrypto/hashes/sha1-checked)
warning: fields `dv_type`, `dv_k`, `dv_b`, and `maski` are never read
  --> sha1-checked/src/ubc_check.rs:43:9
   |
42 | pub struct Info {
   |            ---- fields in this struct
43 |     pub dv_type: u32,
   |         ^^^^^^^
44 |     pub dv_k: u32,
   |         ^^^^
45 |     pub dv_b: u32,
   |         ^^^^
46 |     pub testt: Testt,
47 |     pub maski: i32,
   |         ^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

Which go back to the original C code, and I filed an issue there to better understand the reason for this: https://github.com/cr-marcstevens/sha1collisiondetection/issues/90

cr-marcstevens commented 2 days ago

The last commit removes dvtype, dvK and dvB from the internal tables entirely. While that is fine, these are also the actual identifiers for the tested disturbance vector. For documentation purposes might I suggest that in the place of the removed code lines, comments are added of the form /// disturbance vector: type= K= B=

dignifiedquire commented 2 days ago

@cr-marcstevens you are right, made https://github.com/RustCrypto/hashes/pull/600 to improve the situation