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

umask affects permissions in zkg bundles #196

Open
ekoyle opened this issue Jul 17, 2024 · 5 comments · May be fixed by #197
Open

umask affects permissions in zkg bundles #196

ekoyle opened this issue Jul 17, 2024 · 5 comments · May be fixed by #197

Comments

@ekoyle
Copy link

ekoyle commented Jul 17, 2024

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
Copy link
Member

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

@bbannier bbannier transferred this issue from zeek/zeek Jul 18, 2024
@ekoyle
Copy link
Author

ekoyle commented Jul 19, 2024

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
Copy link
Contributor

awelzel commented Jul 19, 2024

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
Copy link
Author

ekoyle commented Jul 19, 2024

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
Copy link
Author

ekoyle commented Jul 20, 2024

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+

@ekoyle ekoyle linked a pull request Jul 22, 2024 that will close 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

Successfully merging a pull request may close this issue.

3 participants