Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password validation failing for valid passwords, on files where bit 3 of general purpose flag is set. #471

Closed
lukemalcolm opened this issue Mar 5, 2024 · 5 comments

Comments

@lukemalcolm
Copy link
Contributor

I'll shortly be putting in a Pull Request to fix the below, but I'm logging an issue for tracking purposes.

Currently, ADM-ZIP ZipCrypto (PKCRYPT method) rejects valid passwords that use the Info-Zip standard of password validation.

This specification uses the high bit of the headers modified time, instead of the crc to compare against the LSB of the decrypted 12 bit salt and is only applicable when bit 3 of the general purpose bit flag is set.

This method of password checking looks like it originated in the Info-Zip standard, is still used by a number of older zip generators and is gracefully handled by all other zip tools I tried (7zip, WinZip, Windows etc).

Spec References:

Info-Zip Standard - Line 2806
minizip-ng implementation - Line 193

In my particular case, this has occurred in zips generated [zlib-ng/minizip-ng](by https://github.com/zlib-ng/minizip-ng).

This may be the cause of #467 #385 - but without more details, it's hard to say.

More details in the forthcoming Pull Request.

@lukemalcolm
Copy link
Contributor Author

Have added a pull request #472

@dhirajkumar7029
Copy link

I am facing the same error.

I have merged the above PR changes into my local repo and checked. It's working. @lukemalcolm Can we expedite in pushing the above fix?

@KurtMar
Copy link

KurtMar commented Mar 12, 2024

@lukemalcolm have you looked at https://www.npmjs.com/package/@zip.js/zip.js?

It seems to handle encrypted files quite well. The API is a bit "different" and the documentation is not so clear, so here's some example code you could test. Not sure if its the best way to do things though:

const password = 'password'
const flattenOutputLikeAdmZip = true
const filePath = path.join(__dirname, 'encrypted_zip.zip')
const fileBody = fs.readFileSync(filePath)
const zip = new zipjs.ZipReader(new zipjs.BlobReader(new Blob([fileBody])))
const entries = await zip.getEntries({ filenameEncoding: 'utf-8' })
const results = await Promise.allSettled(R.map(async (zipEntry) => {
  if (zipEntry.directory) {
    // when storing to S3 etc. we could just skip these
  }
  const fileName = flattenOutputLikeAdmZip ? path.basename(zipEntry.filename) : zipEntry.filename
  const writer = new zipjs.Uint8ArrayWriter();
  // const textWriter = new zipjs.TextWriter();
  // getData can be undefined according to the typescript compiler
  const fileContents = zipEntry.getData ? await zipEntry.getData(writer, { password }) : new Uint8Array()
  const entryAsBuffer = Buffer.from(fileContents)
  console.log('extracted file', fileName)
  // const text = await fileContents.text()
  // console.log(text)
  return entryAsBuffer
}, entries))
return results

export const isFileEncrypted = async (fileBody: Buffer) => {
  const fileAsZipJs = new zipjs.ZipReader(new zipjs.BlobReader(new Blob([fileBody])))
  const zipJsEntries = await fileAsZipJs.getEntries()
  return zipJsEntries.length > 0 ? zipJsEntries[0].encrypted : false
}

@lukemalcolm
Copy link
Contributor Author

lukemalcolm commented Mar 13, 2024

@KurtMar I didn't look zip.js when I wrote the change.

Instead I just looked the specification documents for Zip and Info-Zip, other C and C++ implementations.

However, I've taken a look now, and it appears zip.js is doing the same thing as my pull request. The most relevant line of code in zip.js here:

https://github.com/gildas-lormeau/zip.js/blob/19c693a0aea88a2995ad0daadf57c769f2a1d9f3/index.cjs#L9561

You can see there the verification switch between crc and high bit modified time on the basis of the dataDescriptor flag (bit 3).

Anyway, we'll see what comes after @cthackers has reviewed the pull request.

@lukemalcolm
Copy link
Contributor Author

The pull request #472 was merged into 0.5.11. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants