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

[manual][release-0.20] backport path validation #309

Closed
wants to merge 10 commits into from

Conversation

ffromani
Copy link
Contributor

@ffromani ffromani commented Dec 2, 2024

manual backport of #304 and #308

Older versions will case the lane to auto-fail

Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 41aca66)
…lease-0.20

ghactions: bump upload-artifact to v4
we missed to update one instance to v4

Signed-off-by: Francesco Romani <[email protected]>
…ions-0.20

[release-0.20][manual] ghactions: fix missing upload-artifact reference
add validation and restrictions about configuration paths,
to reduce the chance of path traversal vulnerabilities.

Most notably:
- config root path must be one of the allowed subpaths
- configlet are expected not to escape the configuratio
  directories

Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 5dbc2c1)
Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 6dca932)
Use more restrictive directory permissions for the
notification file. We don't need a world accessible
directory anyway.

X-Gosec-Scan: 2.21.4
Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 1ecd2c5)
we have some functions we call unchecked because
we are highly confident they can't fail - or we can't
deal with failure anyway. So we capture and discard
the return value in these cases to make gosec happy.

X-Gosec-Scan: 2.21.4
Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 47d3955)
We recently added filepath sanitization before
to feed it in the `os.ReadFile`.
Turns out the linter was still unhappy because
it seems it can't track the variable sanitization
across function calls bonundaries.

With this change we rework the validation
to have them in the same function block of
the `ReadFile` call. This is arguably (much)
more wasteful (in relative terms) because we
redo multiple times redundant validations,
for example when we handle the config file directory.

Still, this makes the linter demonstrably
happier and it's (hopefully) safer because
we can't call ReadFile by mistake with unsanitized
input.

In addition, configfile reading is done once at runtime,
so the extra cost should be amortized.

X-Gosec-Scan: 2.21.4
Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit 3a85191)
Add validation for `os.ReadDir` much like
we did for `os.ReadFile`.

This was not required by gosec, but we
do for consistency and trying to be future-proof.

Signed-off-by: Francesco Romani <[email protected]>
(cherry picked from commit c712cce)
@ffromani ffromani closed this Dec 2, 2024
@ffromani ffromani deleted the validate-paths-0.20 branch December 2, 2024 12:41
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