umask affects permissions in zkg bundles

[ekoyle]

The umask set by a user affects permissions of files on disk, which are then honored when creating the zkg bundle tarball. Setting a umask to something like 0027 is a good security practice, but can create bundles that cause problems in common zeek installations.

It would be very useful if zkg could create identical bundles regardless of a user's umask (or the permissions of the files on disk), or at least emit a warning if scripts aren't world readable when creating a bundle.

[bbannier]

There seem to be two issues here:

• zkg install does not respect the original file permissions in the package and they are always installed with the default umask
• zkg bundle does not preserve permission

[ekoyle]

I think the problem is that zkg bundle does preserve permissions (when the repo is cloned, the umask causes file permissions to be more restrictive on disk and then zkg copies these permissions into the tar). Also, it appears the original owner/group information is in the tar file as well.

Looking at the code, zkg install is likely honoring the file permissions from the tarball, which I think is dangerous. If that is the case, there is a reasonable chance that an attacker could put setuid files or device nodes in a package with unsafe permissions which could have some pretty significant security implications (especially if zkg is run as root), but might not be apparent to someone who is only looking for malicious code.

A possible solution would be to set the umask to 022 and then use .extractall(..., filter="data") which should mostly ignore permissions and owners in the tar file -- see here -- however this wasn't added until python3.12 (could be years before that is the minimum version across all supported distros).

[awelzel]

Nice.

Setting a umask to something like 0027 is a good security practice, but can create bundles that cause problems in common zeek installations.

So initially it's a usability issue where the user Zeek runs under can't read unbundled scripts?

A possible solution would be to set the umask to 022 and then use .extractall(..., filter="data") which should mostly ignore permissions and owners in the tar file -- see here -- however this wasn't added until python3.12 (could be years before that is the minimum version across all supported distros).

Ignoring the user/group and suid bits from the tarfile seems reasonable and allowlisting certain file types. I'm a bit uncertain about x flags, we may want to keep those (think helper scripts).

So maybe tars --no-same-permissions open-coded using Tarfile.extractfile() with some extra filtering?

[ekoyle]

So initially it's a usability issue where the user Zeek runs under can't read unbundled scripts?

Yes. If a bundle is created on a system with restrictive permissions, those permissions are applied while unbundling, and this generally leads to permissions issues with zeek unless you are unbundling as the same user zeek runs as.

Ignoring the user/group and suid bits from the tarfile seems reasonable and allowlisting certain file types. I'm a bit uncertain about x flags, we may want to keep those (think helper scripts).

So maybe tars --no-same-permissions open-coded using Tarfile.extractfile() with some extra filtering?

I think the filter="data" approach is smart about the execute bits: basically, if the user x bit is unset, it won't allow the x bit to be set for the group or other. For bundles, I think we could be more restrictive. It might make more sense to copy the r and x bits from the user to group/other, and set w to 0 for them. I think this would generally end up with permissions like -rwxr-xr-x or -rw-r--r--, regardless of how unusual the permissions are within the tar file.

I'll go take a look at the wrapper and see how much effort it would be to implement this.

[ekoyle]

It appears the filter= option for extraction has been backported all the way to python 3.8, however many OSes don't upgrade to newer patch versions. Debian bookworm (latest stable, extended support ends in 2028) is on python 3.11.2, but the fix wasn't introduced until 3.11.4.

I wonder if we could just include a more recent tarfile.py under a different name...

Versions which include the new feature:

3.8.17
3.9.17
3.10.12
3.11.4
3.12+