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

proposal: archive/zip: support for encrypted archives #12081

Closed
raichu opened this issue Aug 9, 2015 · 27 comments
Closed

proposal: archive/zip: support for encrypted archives #12081

raichu opened this issue Aug 9, 2015 · 27 comments

Comments

@raichu
Copy link

raichu commented Aug 9, 2015

ZIP file format specification includes encrypted archives.
Current implementation doesn't support writing/reading such archives.

@ianlancetaylor ianlancetaylor changed the title zip: support for encrpyted archives archive/zip: support for encrpyted archives Aug 9, 2015
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 9, 2015
@alexmullins
Copy link

I've got a working implementation of archive/zip that can read/write password protected archives. The code is at https://github.com/alexmullins/zip.

Details:
The encryption/decryption method used is WinZip AES Encryption Specification (http://www.winzip.com/aes_info.htm). There are 2 other encryption methods specified in the original Zip Spec (https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT, Sections 6 and 7) which aren't implemented. The method described in Section 6, also known as ZipCrypto, is generally considered 'weak' and isn't advisable to use. The method described in Section 7 seems to be exclusively used with PKWARE products as I haven't seen any other opensource implementations support it.

Public API additions:

  • FileHeader.DeferAuth
  • func (*FileHeader) IsEncrypted
  • func (*FileHeader) SetPassword
  • func (*Writer) Encrypt

Current roadblocks:

  • The method to turn a password into a master-key is PBKDF2 and I'm getting that functionality from the 3rd party package golang.org/x/crypto/pbkdf2.
  • Duplicating the cipher.NewCTR implementation from Go's Standard Library. (See here: https://github.com/alexmullins/zip/blob/master/crypto.go#L109). It was clarified for me that Go uses a right-aligned counter which is the standard way of doing it. WinZip AES uses a left-aligned counter. The difference between the two is:

Go CTR:
00000000000000000000000000000001
00000000000000000000000000000002

WinZip CTR:
01000000000000000000000000000000
02000000000000000000000000000000

Any feedback is welcome.
Thanks.

@bradfitz bradfitz changed the title archive/zip: support for encrpyted archives archive/zip: support for encrypted archives May 10, 2016
@yeka
Copy link

yeka commented Oct 17, 2016

I was working on a project that requires me to send a password protected zip file to a system that can only read Zip Standard Encryption. Not finding any go source for this, I manage to create a working code based on @alexmullins code. Special thanks to you Sir 👍

While it's not advisable to use Zip Standard Encryption for security reason, for those who have to work with it, you can check my code at https://github.com/yeka/zip

I hope the awesome Go Team can integrate encryption into their standard zip/archive 😄

@ghost
Copy link

ghost commented Jul 15, 2017

Any updates here? officially supporting protected files would be good enough :)

@florianpinel
Copy link

@yeka Could you add a license to your git repo?

@seankhliao seankhliao changed the title archive/zip: support for encrypted archives proposal: archive/zip: support for encrypted archives Apr 20, 2022
@seankhliao seankhliao modified the milestones: Unplanned, Proposal Apr 20, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 20, 2022
@jeffwidman
Copy link
Contributor

Sounds like there are two popular file formats as noted in #52458:

There are two common encryption formats that we should support:

  1. ZipCrypto: This is the original encryption scheme. It has widespread support, so should be added for compatibility purposes. However, it is not secure, so should come with a big disclaimer that new files should only be created for compatibility purposes and the encryption should not be trusted.
  2. WinZip’s AES: This is what WinZip and 7Zip use by default to create encrypted zip files.

#52458 also explains why an external lib isn't ideal:

Adding support for both of these formats into the standard library is relatively straightforward. Unfortunately, as it stands today, it’s impossible to add support for encryption as a wrapper around the standard library archive/zip. A custom compressor/decompressor handler approach doesn’t work because encryption needs at minimum access to the FileHeader. In the case of WinZip AE-2 it even needs to disable the CRC. So libraries have to completely fork archive/zip.

@ianlancetaylor
Copy link
Contributor

What the proposal process needs is a description of the new API that would be required to implement this. Anybody want to try to write that down? I haven't looked at the work mentioned above by @alexmullins .

@ahilananantha
Copy link

ahilananantha commented Apr 21, 2022

A first take at the API changes:

type EncryptionMethod int

const (
	NoEncryption EncryptionMethod = iota
	ZipCrypto
	WinZipAES128
	WinZipAES192
	WinZipAES256
)

type FileHeader struct {
	/* ... */

	Password func() []byte

	Encryption EncryptionMethod

	WinZipAEVersion int

	/* ... */
}

The three additional fields to FileHeader change the behavior when writing a zip.

When FileHeader.Encryption is not equal to NoEncryption and FileHeader.Password is non-nil, the specified encryption method is used with the password bytes returned by by calling FileHeader.Password..

While FileHeader.Encryption is one of the Winzip methods, the AE file format "version" must be decided upon. WinZip chooses between AE-1 and AE-2 using logic described here:

https://www.winzip.com/en/support/aes-encryption/#winzip11

If FileHeader.WinZipAEVersion is 0 in a FileHeader passed to Writer.CreateHeader(), an automatic decision is taken. A value of 1 or 2 would be taken as choosing AE-1 and AE-2 respectively. Any other value is erroneous.

When reading a zip file, the FileHeader.Encryption and FileHeader.WinZipAEVersion fields will be populated. FileHeader.WinZipAEVersion will be 0 in the case of no encryption or ZipCrypto.

Prior to calling File.Open() on a file marked as encrypted, the user must specify a function for FileHeader.Password. If nil, the open will fail with an invalid password error. If non-nil and the function returns a password that fails to decrypt the file, the same invalid password error will be returned.

Updated : had neglected the use of the password on the read path.

@ianlancetaylor
Copy link
Contributor

CC @dsnet

@ianlancetaylor
Copy link
Contributor

I don't know how zip encryption works. If it requires a password, how should that password be supplied when reading a zip archive?

@ahilananantha
Copy link

ahilananantha commented Apr 22, 2022

@ianlancetaylor good point, I missed that part. FileHeader.Password would also be called in the read path. It would have to be set prior to calling File.Open. This is modeled on @alexmullins 's fork.

	zReader, err := zip.NewReader(data, size)
	// ...
	for _, file := range zReader.File {
		// We can see that it's encrypted
		if file.Encryption != zip.NoEncryption {
			// and set appropriate per file password
			file.Password = func() []byte {
				return []byte("password")
			}
		}
		rc, err := file.Open()
		// ...
	}

@ahilananantha
Copy link

I didn't mention this, since it's something the user would not have to do. Writer.CreateHeader() would be expected to update FileHeader.Flags to indicate encryption. In the same that FileHeader.Flags gets updated when FileHeader.NonUTF8 gets set by the user.

@alexmullins
Copy link

Hi, wanted to chime in. One roadblock that was hit when I initially was looking to integrate this into the std zip package is that there's a dependency on golang.org/x/crypto/pbkdf2. Not sure if there's a way around this problem.

@jeffwidman
Copy link
Contributor

One roadblock that was hit when I initially was looking to integrate this into the std zip package is that there's a dependency on golang.org/x/crypto/pbkdf2. Not sure if there's a way around this problem.

Not sure if you remember, but you raised that as an issue over in #13979. Two members of the go org chimed in to say that given that it's ~40 LOC, vendoring or copy/pasting is probably fine.

@ianlancetaylor
Copy link
Contributor

If necessary we can vendor it into the standard library. See $GOROOT/src/vendor/golang.org/x/crypto for other vendored crypto packages.

@iangudger
Copy link
Contributor

@rsc any chance a decision could be made on this proposal?

@ior308
Copy link

ior308 commented Sep 15, 2023

Frankly, I am puzzled that a standard golang library has such a big gap on the zip protocol that its possible use on a large scale vanished.

@rsc
Copy link
Contributor

rsc commented Oct 11, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@jeffwidman
Copy link
Contributor

@rsc does ☝️ mean the core go team is interested in the feature?

I'm on paternity leave so have a little extra time in my life right now and happy to take a stab at clarifying the API design + implementing it (probably leveraging @alexmullins initial work ☝️), but first want to ensure the feature is of interest to the maintainers.

@zephyrtronium
Copy link
Contributor

@jeffwidman A proposal being in active status means that the proposal team will review the proposal, clarify details, and wait for consensus on whether and how it should be implemented. If it's accepted, an implementation can be merged into the standard library. You can find a full description of the proposal process here: https://github.com/golang/proposal#readme.

If you intend to contribute an implementation based on someone else's work, make sure you understand the terms of the CLA.

@miquella
Copy link

As it turns out, I've been working on a new module for reading and writing zip files (https://github.com/miquella/zipenc). I would happily donate any portion of it, if it would be helpful.

I was hopeful that I could use an existing implementation, but had various troubles getting the existing implementations to work (e.g. missing password validation, poor key initialization, compilation issues, etc.).

Reluctantly, I started a new implementation.

Reimplementing substantial portions of archive/zip sounded very unappealing, but so did maintaining a fork of it. Instead, I opted for a middle-ground that could still be troublesome: wrapping the "raw" file interfaces. Although it hasn't been without trouble, it has largely worked well so far.

The primary trouble is accessing the registered (de)compressors, which is why the module's current state only supports uncompressed files (Store method).

I have tests and things coming, but wanted to get the basic version (which I believe works) posted. Only uncompressed zipcrypto is supported for now, since that's what I needed first.


Note: Although I've tried to implement things according to the APPNOTE spec, it has been absolutely invaluable to have other implementations to reference. So thank you to all of those who came before!

Here are a few that I've referenced while working on this project:

@rsc
Copy link
Contributor

rsc commented Oct 24, 2023

Above there is a comment about needing a different CTR mode where the counter is written in little-endian order instead of big-endian order. This can be done without forking the crypto/cipher.NewCTR code with something along these lines:

type invBlock struct {
	cipher.Block
}

func reverse(b []byte) {
	for i, j := 0, len(b)-1; i < j; i, j = i+1, j-1 {
		b[i], b[j] = b[j], b[i]
	}
}

func (b *invBlock) Encrypt(dst, src []byte) {
	reverse(src)
	b.Block.Encrypt(dst, src)
}

func (b *invBlock) Decrypt(dst, src []byte) {
	b.Block.Decrypt(dst, src)
	reverse(src)
}

func NewInvCTR(block cipher.Block, iv []byte) cipher.Stream {
	return cipher.NewCTR(&invBlock{block}, iv)
}

@rsc
Copy link
Contributor

rsc commented Oct 24, 2023

I am not sure what the path forward here looks like. It appears that any API would have to take into account:

  1. Each file entry can have a separate encryption password.
  2. Each file entry can have a separate encryption certificate as well.
  3. The overall zip file can have a separate encryption password for the central directory (used to find out about the other files).
  4. The overall zip file can have a separate encryption certificate as well.
  5. There are a large variety of defined encryption algorithms, and we don't want to make archive/zip depend on them all.

zip.FileHeader is meant to be pure data about the header; it seems wrong to drop in a func field.

I wonder what the minimal set of changes would be to allow code to "bring your own encryption" instead, a bit like how we allow registering compressors and decompressors.

@starius
Copy link

starius commented Nov 15, 2023

@rsc CTR mode of encryption never calls block.Decrypt, so invBlock.Decrypt can panic.

@rsc
Copy link
Contributor

rsc commented Feb 7, 2024

I think we still don't have a path forward here. I am leaning toward declining this for now. It is not that important and we don't know what to do.

@jeffwidman
Copy link
Contributor

Agree. I've looked at this a little from a design standpoint and didn't see an ergonomic API so I shelved it intending to circle back later in case I was missing something but no clean solution is jumping out at me.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2024

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Feb 14, 2024

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Declined
Development

No branches or pull requests