hashes icon indicating copy to clipboard operation
hashes copied to clipboard

sha1-checked: `ubc_check` is not used

Open newpavlov opened this issue 1 year ago • 1 comments

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

newpavlov avatar Jun 16 '24 23:06 newpavlov

Ack. Will take a look in the next days

dignifiedquire avatar Jun 17 '24 20:06 dignifiedquire

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

dignifiedquire avatar Jul 03 '24 09:07 dignifiedquire

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=

cr-marcstevens avatar Jul 03 '24 10:07 cr-marcstevens

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

dignifiedquire avatar Jul 03 '24 11:07 dignifiedquire