Vendir allows to use unsafe paths in configuration

[Zebradil]

This issue was initially discussed in Slack.

What steps did you take:

Here is a shell script to reproduce the issue. It runs commands during docker build, so you can safely run it, as it doesn't change any files on the host system.

#!/usr/bin/env bash

cat <<'EOF' | docker build --progress=plain --file=- .
FROM alpine:20231219
ARG TARGETOS=linux
ARG TARGETARCH=amd64
ARG VENDIR_VERSION=v0.38.0
RUN wget -q \
      https://github.com/carvel-dev/vendir/releases/download/${VENDIR_VERSION}/vendir-${TARGETOS}-${TARGETARCH} \
      -O /usr/bin/vendir
RUN chmod +x /usr/bin/vendir

WORKDIR /some/safe/place
RUN pwd
RUN ls -l /usr
RUN cat <<YAML | /usr/bin/vendir sync -f-
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: /usr
    contents:
      - path: foo
        inline:
          paths:
            foo.txt: file-content
YAML
RUN ls -l /usr
EOF

What happened:

Here is the important part of the output of the script. I added comments to explain the steps.

ℹ️ Verify the current directory: it is some nested directory, not root
#8 [5/8] RUN pwd
#8 0.449 /some/safe/place
#8 DONE 0.5s

ℹ️ List files in the /usr directory, as it will be affected later
#9 [6/8] RUN ls -l /usr
#9 0.618 total 0
#9 0.619 drwxr-xr-x    1 root     root            12 Jan 14 11:32 bin
#9 0.619 drwxr-xr-x    1 root     root           120 Dec 19 11:07 lib
#9 0.619 drwxr-xr-x    1 root     root            22 Dec 19 11:07 local
#9 0.619 drwxr-xr-x    1 root     root           402 Dec 19 11:07 sbin
#9 0.619 drwxr-xr-x    1 root     root            32 Dec 19 11:07 share
#9 DONE 0.7s

ℹ️ Run vendir sync with a config that targets the /usr directory (see the config in the script above)
#10 [7/8] RUN cat <<YAML | /usr/bin/vendir sync -f-
#10 0.674 apiVersion: vendir.k14s.io/v1alpha1
#10 0.674 directories:
#10 0.674 - contents:
#10 0.674   - inline: {}
#10 0.674     path: foo
#10 0.674   path: /usr
#10 0.674 kind: LockConfig
#10 DONE 0.7s

ℹ️ List files in the /usr directory again to see that the original files are gone
#11 [8/8] RUN ls -l /usr
#11 0.621 total 0
#11 0.621 drwx------    1 root     root            14 Jan 14 11:38 foo
#11 DONE 0.7s

The same can be achieved by targeting the /usr directory via ../../../usr path in the vendir configuration.

What did you expect:

Vendir should not allow targeting directories via absolute paths or via traversing up the directory tree.

Anything else you would like to add:

There is a list of disallowed file paths in the current code:

vendir/pkg/vendir/config/directory.go

Line 25 in 9d3b17c

disallowedPaths = []string{"/", EntireDirPath, "..", ""}

But it is limited to these exact values: /, .., . and ``.

Environment:

• vendir version: 0.38.0
• OS: Arch Linux x86_64

---

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

[Zebradil]

In the community meeting, we agreed that the fix is to disallow paths outside the current working directory (CWD). Defining the CWD is important here. If I remember correctly, this is the logic (in the order of priority):

• if the --chdir argument is supplied, its value is CWD;
• if the vendir configuration file is on the disk, its location is CWD;
• if the vendir configuration is passed via stdin, the current directory ($PWD) is CWD.

@joaopapereira I'm not entirely sure about the last two points. Could you confirm or correct this? Are there more cases?

[joaopapereira]

• if the vendir configuration file is on the disk, its location is CWD;

I assume that all paths will have to start in the vendir configuration file, so CWD is where that file is

• if the vendir configuration is passed via stdin, the current directory ($PWD) is CWD.

In this scenario, we have to assume that CWD is the current directory.

I think these are all the scenarios, @neil-hickey @cppforlife do you think we might have any other scenarios?