Bug 244181 - unzip: Add passphrase and GLIBC build support
Summary: unzip: Add passphrase and GLIBC build support
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Yoshihiro Takahashi
Keywords: needs-qa, patch
: 253726 (view as bug list)
Depends on:
Reported: 2020-02-17 03:37 UTC by Mingye Wang
Modified: 2021-09-25 16:39 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback? (des)
koobs: maintainer-feedback? (ak)
koobs: mfc-stable12?
koobs: mfc-stable11?

Support glibc builds (916 bytes, patch)
2020-02-17 03:37 UTC, Mingye Wang
no flags Details | Diff
Add passphrase support (2.95 KB, patch)
2020-02-17 03:38 UTC, Mingye Wang
no flags Details | Diff
Mention ZIPX in unzip(1) (702 bytes, patch)
2020-02-17 03:38 UTC, Mingye Wang
no flags Details | Diff
Add passphrase support (v2) (3.26 KB, patch)
2020-02-18 09:26 UTC, Mingye Wang
no flags Details | Diff
Add passphrase support (v3) (3.39 KB, patch)
2020-02-19 03:50 UTC, Mingye Wang
no flags Details | Diff
Add passphrase support (2.97 KB, patch)
2020-03-24 09:30 UTC, Alex Kozlov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mingye Wang 2020-02-17 03:37:56 UTC
Created attachment 211703 [details]
Support glibc builds

I would like to add the attached series of patches to unzip:

1. glibc support, since some people on libarchive's GitHub page is showing interest in this replacement util.
2. passsphrase support, both in the unsafe command-line entry form (-P) and in the safe getpassphrase(3) form.
3. minor edit to the manpage, just to remind people that we can decompress fancy ZIPX archives if libarchive can.

I thought about adding the filename iconv stuff from info-zip's unzip beta 6.10b, but doing stuff like iconv_open is a bit too much for me now. It *is* a nice feature though, at least according to the Linux users who came up with unzip-iconv in the first place. I might just add it sometime.

The patch is based against FreeBSD GitHub src tree. The last commit in usr.bin/unzip should be 7551d83.
Comment 1 Mingye Wang 2020-02-17 03:38:21 UTC
Created attachment 211704 [details]
Add passphrase support
Comment 2 Mingye Wang 2020-02-17 03:38:46 UTC
Created attachment 211705 [details]
Mention ZIPX in unzip(1)
Comment 3 Alex Kozlov freebsd_committer 2020-02-17 14:10:27 UTC
The 'support glibc' and 'mention zipx' patches looks alright, but I think you forgot to free passphrase_buf in 'add passphrase' patch.
Comment 4 Mingye Wang 2020-02-18 09:26:00 UTC
Created attachment 211738 [details]
Add passphrase support (v2)

Oops! Here is the patch with the free() added.
Comment 5 Mingye Wang 2020-02-18 10:38:21 UTC
Expect a passphrase v3 -- it simply doesn't make sense to free in main() when the only cause of malloc() is planted by unzip().
Comment 6 Yuri Pankov freebsd_committer 2020-02-18 11:14:03 UTC
You might want to open this at https://reviews.freebsd.org so it's easier to comment on the code.
Comment 7 Mingye Wang 2020-02-19 03:50:13 UTC
Created attachment 211753 [details]
Add passphrase support (v3)
Comment 8 Alex Kozlov freebsd_committer 2020-03-24 09:30:50 UTC
Created attachment 212674 [details]
Add passphrase support

It seems that unzip fails to build with the last passphrase support patch. I fixed it a little, please check it out.
Comment 9 Thomas Hurst 2020-04-10 15:32:12 UTC
+		memset(passphrase_buf, 0, PPBUFF_SIZE);
+		free(passphrase_buf);

This should probably be using explicit_bzero() to prevent the compiler from optimizing it away.

tar(1) has the same issue.
Comment 10 Alex Kozlov freebsd_committer 2020-04-12 18:59:38 UTC
The explicit_bzero(3) is not portable. So it can't be used in libarchive and it's also absent in NetBSD, which is (still ?) upstream for our unzip.
Comment 11 Yuri Pankov 2020-04-12 19:43:13 UTC
What about memset_s(), which is in C11?
Comment 12 Thomas Hurst 2020-04-13 00:06:58 UTC
Looks like NetBSD wants explicit_memset(), which also isn't portable.
Comment 13 Conrad Meyer freebsd_committer 2020-04-13 00:51:25 UTC
readpassphrase() in the same patch is no more portable than explicit_bzero(), so you should just use explicit_bzero.
Comment 14 Alex Kozlov freebsd_committer 2020-04-13 06:57:26 UTC
Yes, memset_s() should work.

The libarchive's cpio and bsdtar use portable version from libarchive_fe if necessary (see use.bin/tar/Makefile). I left readpassphrase() because Free/Open/NetBSD have it. Can be easily changed back to lafe_readpassphrase().
Comment 15 Yoshihiro Takahashi freebsd_committer 2021-02-23 13:44:47 UTC
*** Bug 253726 has been marked as a duplicate of this bug. ***
Comment 16 Yoshihiro Takahashi freebsd_committer 2021-02-23 14:01:56 UTC
I found a problem with your patch to add passphrase support.
In passphrase_callback(), passphrase_buf is (re)defined as a local variable,
so the global passphrase_buf is not changed here.

I have fixed it.  Please see https://reviews.freebsd.org/D28892

BTW, I except GLIBC part at this time.  I think that it should be done by
libarchive's repository first.
Comment 17 Alex Kozlov freebsd_committer 2021-02-23 15:02:45 UTC
Thanks. That was silly braino. As for GLIBC, as I wrote in 253726, it needs more work and testing. The NetBSD opted for use defines with getpass/getpass_r/readpassphrase but I'm not sure it's the best variant or even if it will work on all/most linux distros.
Comment 18 commit-hook freebsd_committer 2021-09-25 16:39:41 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=a4724ff48108840416c83f10e15d666ac8d78937

commit a4724ff48108840416c83f10e15d666ac8d78937
Author:     Yoshihiro Takahashi <nyan@FreeBSD.org>
AuthorDate: 2021-09-25 16:32:42 +0000
Commit:     Yoshihiro Takahashi <nyan@FreeBSD.org>
CommitDate: 2021-09-25 16:32:42 +0000

    unzip: sync with NetBSD upstream to add passphrase support

    - Add support for password protected zip archives.
      We use memset_s() rather than explicit_bzero() for more portable
      (See PR).
    - Use success/failure macro in exit()
    - Mention ZIPX format in unzip(1)

    Submitted by:   Mingye Wang and Alex Kozlov (ak@)
    PR:             244181
    Reviewed by:    mizhka
    Obtained from:  NetBSD
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D28892

 usr.bin/unzip/unzip.1 | 10 ++++++--
 usr.bin/unzip/unzip.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 66 insertions(+), 10 deletions(-)