zip-rs / zip-old

Zip implementation in Rust
MIT License
731 stars 204 forks source link

by_index_decrypt accepting wrong password #281

Closed sawmurai closed 2 years ago

sawmurai commented 2 years ago

I encountered a possible bug when trying to write a toy password cracker. Or maybe I am just using the library in a wrong way?

by_index_decrypt sometimes accepts an invalid password and returns an Ok(Ok(ZipFile)) that can't actually be read.

// Loop over many passwords (as pw)
let mut archive = zip::ZipArchive::new(file)?;
match archive.by_index_decrypt(0, &pw.as_bytes()) {
    Ok(Ok(mut f)) => {
        let mut buffer = Vec::with_capacity(f.size() as usize);

        match f.read_to_end(&mut buffer) {
            Err(e) => {
                // Custom { kind: Other, error: "Invalid checksum" }
                // Custom { kind: InvalidInput, error: "corrupt deflate stream" }
                eprintln!("{:?}", e);
                continue;
            }
            _ => (),
        }

        println!("Password is '{pw}'");
    }
    Ok(Err(_)) => continue,
    Err(e) => eprintln!("{:?}", e),
};

read_to_end does not fail if the correct password is used. The zipfile is created on linux via zip archive.zip folder -e.

The full source, of which the snippet above was extracted, is here:

https://gist.github.com/sawmurai/74ae914885e8b0341b09ebd60e0619ab

Let me know if you need any further info :) Thank you!

zamazan4ik commented 2 years ago

@sawmurai Thank you for submitting the issue!

It will be easier for us if you provide some addional information:

sawmurai commented 2 years ago

Yes, the bug is still reproducible with the latest 0.6 release (I may say however, that the latest release introduced some nice performance improvements :) )

Anyway, I attached the zip file: test.zip

The correct password to open it is ' dondonan' (without the single quotes, with the leading space). Wrongly identified as correct for example:

opelpoenya hsu7808 prettybeng

Many others.

Thanks for taking the time! :)

Edit: Maybe I was wrong about the performance improvements. But its still fast enough :)

zamazan4ik commented 2 years ago

I have checked this archive with my unzip from Fedora 35, version 6.0.0. And it shows the following behaviour:

As you see, behaviour completely matches behaviour of the library. Maybe @Plecra can comment something. As far as I remember from sources, such a situation is fine now since for some reason we cannot verify the password before deflating.

Plecra commented 2 years ago

Yup! This is because the ZIP spec only allows us to check for a 1/256 chance that the password is correct. There are many passwords out there that will also pass the validity checks we are able to perform. This is a weakness of the ZipCrypto algorithm, due to its fairly primitive approach to cryptography. This should 100% be added to the documentation of the functions

zamazan4ik commented 2 years ago

This should 100% be added to the documentation of the functions

Got it. I will create a PR with some additional documentation for this case, and then we can close the issue, I guess.

sawmurai commented 2 years ago

This is very interesting! Thanks for the explanation :)

zamazan4ik commented 2 years ago

Closing it as answered. And I have merged updated comments about such a little tricky behaviour.