zeek / package-manager

A package manager for Zeek
https://docs.zeek.org/projects/package-manager
Other
42 stars 27 forks source link

Aliases can cause package breakage #168

Closed bbannier closed 10 months ago

bbannier commented 1 year ago

Adding aliases to a package allows to @load packages under alternative names. It seems we currently do not check whether that creates a conflict by aliasing another package, e.g.,

# The default BTest setup for zeek/package-manager` sets up dummy packages
# `foo` and `bar`. Adjust `foo` so it aliases `bar` as well.

# @TEST-EXEC: bash %INPUT

# @TEST-EXEC: zkg install foo bar
# @TEST-EXEC: btest-diff scripts/foo/__load__.zeek
# @TEST-EXEC: btest-diff scripts/bar/__load__.zeek

cd packages/foo
# echo 'aliases = foo bar' >> zkg.meta # HERE: Baseline changes if this line is enabled.
git commit -am 'Add aliases'

If the line marked HERE is enabled the baseline changes and scripts/bar/__load__.zeek now points to scripts/foo/__load__.zeek. In a real world scenario this could e.g., cause packages to not work correctly, or allow a malicious package to take over another package.

Looking at aggregate.meta in the default package source I currently see no instances of this being an issue, but we should fix zkg so it rejects cases where this could be an issue (e.g., reject installation of a package which aliases another installed package, and reject installation of a package which is aliased by another package). We cannot fix this in the package source since users might use sources not under our control, or a package source's aggregate.meta could lag.

awelzel commented 10 months ago

One can also use aliases = ../../../../../../../vmlinuz or so to delete/replace existing symlinks on the system, assuming enough permissions.

However, an actual malicious package can delete, symlink or possibly inject things within its build and test command left and right as the user running zkg. With @unload one can also prevent loading of another package within Zeek scripts separately.

So, yes, this can lead to surprising/annoying behavior and the checks seem useful, but Priority High seems a bit unreasonable.