Skip to content

Contain handler_folder within the build context - #40

Open
welteki wants to merge 3 commits into
openfaas:masterfrom
welteki:fix-handler-folder-containment
Open

welteki wants to merge 3 commits into
openfaas:masterfrom
welteki:fix-handler-folder-containment

Conversation

@welteki

@welteki welteki commented Sep 7, 2026

Copy link
Copy Markdown
Member

Description

Validate handler-folder paths to reject absolute paths and traversal outside
the function's build context. Require function names to be single directory
components before clearing the context.

Also fix extra-path validation accepting similarly named sibling directories,
and derive copy destinations from validated relative paths to prevent writes
outside the context.

Motivation and context

handler_folder from a language template's template.yml was joined onto the
build context without a containment check, so a value containing .. segments
could create and copy the function handler tree outside of the build context
with the build user's privileges. This allows a malicious or compromised
template to write or overwrite arbitrary files on the machine running the
build. The existing pathInScope check covered copyExtraPaths, but not
handler_folder.

How has this been tested

  • Regression tests cover traversal, outside-file preservation, extra-path
    containment and scope-root rejection.
  • Reproduced the escape with faas-cli build --shrinkwrap against a crafted
    template and verified the build now fails safely with an error and no files
    are written outside of the build context.

@reviewfn

This comment has been minimized.

@welteki
welteki force-pushed the fix-handler-folder-containment branch from a294983 to 0eb56cf Compare September 7, 2026 15:01
@reviewfn

This comment has been minimized.

@welteki
welteki force-pushed the fix-handler-folder-containment branch from 0eb56cf to 31b063d Compare September 8, 2026 09:39
@reviewfn

This comment has been minimized.

@welteki
welteki force-pushed the fix-handler-folder-containment branch from 31b063d to c50e182 Compare September 8, 2026 10:03
@reviewfn

This comment has been minimized.

@derek derek Bot added the no-dco label Sep 8, 2026
@derek

derek Bot commented Sep 8, 2026

Copy link
Copy Markdown

Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project.

Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken.

💡 Shall we fix this?

This will only take a few moments.

First, clone your fork and checkout this branch using the git CLI.

Next, set up your real name and email address:

git config --global user.name "Your Full Name"
git config --global user.email "you@domain.com"

Finally, run one of these commands to add the "Signed-off-by" line to your commits.

If you only have one commit so far then run: git commit --amend --signoff and then git push --force.
If you have multiple commits, watch this video.

Check that the message has been added properly by running "git log".

@welteki
welteki force-pushed the fix-handler-folder-containment branch from 7744b9b to 62555dc Compare September 8, 2026 10:11
@derek derek Bot removed the no-dco label Sep 8, 2026
@reviewfn

This comment has been minimized.

@reviewfn

This comment has been minimized.

@welteki
welteki force-pushed the fix-handler-folder-containment branch from 62555dc to 28552ae Compare September 8, 2026 11:09
@reviewfn

This comment has been minimized.

`handler_folder` from a language template's template.yml was joined onto the
build context without a containment check, so a value containing `..` segments
could create and copy the function handler tree outside of the build context
with the build user's privileges. This allows a malicious or compromised
template to write or overwrite arbitrary files on the machine running the
build.

CreateBuildContext now resolves the handler destination with filesystem-aware
path operations and requires it to be a strict descendant of the intended
per-function build root. Absolute paths, path traversal and destinations
reached through a symbolic link that escapes the build context are rejected,
and no directories are created for an out-of-scope destination.

Add tests for path traversal, hidden traversal, absolute paths, prefix
siblings and symlink escapes.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <han@openfaas.com>
CreateBuildContext removes an existing per-function build directory
before recreating it. Joining an unchecked function name allowed a
direct SDK caller to use path separators or traversal to move that
destructive operation outside BuildDir.

Treat BuildDir as the caller-authorized root and require functionName to
be one portable filesystem path component before calling RemoveAll.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <han@openfaas.com>
Extra-path validation accepted sibling directories sharing the project's
name prefix, allowing reads outside the project. The copy destination
also reused the raw input: ../project/shared/sentinel could resolve
inside the project but write outside the function's build context.

Use filepath.Rel and filepath.IsLocal to check lexical containment and
reject the scope root, including differently capitalized Windows paths.
Derive each copy destination from the validated relative source path and
validate each extra path immediately before copying it.

Regression tests cover prefix-sibling rejection, copying to the expected
location while preserving an outside sentinel, and scope-root rejection.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <han@openfaas.com>
@welteki
welteki force-pushed the fix-handler-folder-containment branch from 28552ae to 467d4eb Compare September 8, 2026 11:54
@reviewfn

reviewfn Bot commented Sep 8, 2026

Copy link
Copy Markdown

AI Pull Request Overview

Disclaimer: This review was generated by automated AI and may contain errors. Do not trust its outputs without human verification.

Summary

  • Adds validation for per-function build context paths before RemoveAll runs.
  • Constrains non-dockerfile handler overlays to local paths under the function build context.
  • Reworks extra path copying to use the validated scope-relative destination path.
  • Fixes prefix-sibling path checks by using filepath.Rel and filepath.IsLocal instead of string-prefix matching.
  • Adds unit coverage for traversal, absolute paths, sibling-prefix paths, scope-root rejection, and destination overwrite prevention.

Approval rating (1-10)

9/10. The changes address the scoped path traversal risks with focused validation and relevant regression coverage.

Summary per file

Summary per file
File path Summary
builder/builder.go Validates function names, handler overlays, and extra path destinations before copying.
builder/builder_test.go Adds regression tests for traversal rejection and safe copy destinations.

Overall Assessment

The patch is narrowly targeted and improves the build-context containment model in the areas touched by the PR. The move from prefix checks to filepath.Rel plus filepath.IsLocal closes the sibling-prefix bypass for extra paths, and validating the function build path before os.RemoveAll removes a destructive traversal hazard. The handler overlay check is also applied before directory creation, so rejected overlay paths do not create or overwrite out-of-context destinations. I did not find blocking issues in the scoped diff.

Detailed Review

Detailed Review

No blocking findings.

The changed CreateBuildContext flow validates functionName before constructing and clearing the build directory, which is the right ordering for avoiding traversal-driven deletion outside the configured build directory.

The handlerFolderWithinScope helper uses local-path validation before joining with the build root, so absolute paths and .. escapes are rejected before MkdirAll or handler copying can affect out-of-scope destinations.

The updated pathInScope returns both the absolute source and scope-relative path, which avoids reusing raw extra-path input containing .. segments for destination construction. This closes the destination escape covered by the new regression test.

The added tests exercise the important edge cases introduced by this patch, including sibling-prefix paths, build-context deletion order, rejected handler overlays, and destination placement for accepted extra paths.

AI agent details.

Agent processing time: 1m40.085s
Environment preparation time: 3.246s
Total time from webhook: 1m45.974s

@welteki
welteki marked this pull request as ready for review September 8, 2026 12:01
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 this pull request may close these issues.

1 participant