Conversation
Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Update EVE-API to include the recently added SCEP (Simple Certificate Enrollment Protocol) and PNAC (802.1x) support. Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| tarReader := tar.NewReader(gzf) | ||
| for { | ||
| header, err := tarReader.Next() |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 28 days ago
In general, to fix Zip Slip–style issues, you must ensure that paths derived from archive entries cannot escape a designated extraction root. This is typically done by (1) normalizing the archive entry path, (2) rejecting any path that is absolute, contains .. path elements, or otherwise resolves outside the intended base directory, and (3) only passing validated, safe paths to filesystem operations like os.Mkdir and os.Create.
For this code, the simplest and least invasive fix is to update resolvePath so that it both normalizes and validates the input path and returns an error if it detects traversal or an absolute path. Since all uses of header.Name flow through resolvePath, tightening its behavior will secure every filesystem operation using newPath. A robust approach is:
- Convert the incoming
curPath(e.g.,header.Name) to a slash‑normalized form and strip any leading/so that archive entries that start at the filesystem root are rejected or made relative. - Use
filepath.Cleanon the path after replacing the matchedLocationprefix withDestination, and explicitly reject any path that contains..path components or that is absolute. - Optionally, if you also want to ensure extraction stays within a well‑defined base directory, derive a base directory from
Destinationand then verify that the cleanednewPathis still within that base usingfilepath.Absandstrings.HasPrefix.
Because we cannot alter callers outside this file, the best single change is to modify resolvePath to:
- Return
os.ErrNotExist(or another error) ifcurPathcontains..path elements or attempts to be absolute. - Only return the mapped
newPathafter runningfilepath.Cleanand rechecking that it does not contain..and isn’t absolute.
This preserves existing semantics for valid paths (they still mapLocationtoDestination) while preventing maliciousheader.Namevalues from producing unsafenewPathvalues. No new imports are needed;filepathandstringsare already imported.
| @@ -75,12 +75,36 @@ | ||
| } | ||
|
|
||
| func resolvePath(curPath string, paths []FileToSave) (string, error) { | ||
| // Normalize to OS-specific path separators. | ||
| normalized := filepath.FromSlash(curPath) | ||
| // Disallow absolute paths from the archive. | ||
| if filepath.IsAbs(normalized) { | ||
| return "", os.ErrNotExist | ||
| } | ||
| // Clean the path and reject any traversal using "..". | ||
| cleaned := filepath.Clean(normalized) | ||
| // After filepath.Clean, any traversal will include ".." as a path element. | ||
| if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(os.PathSeparator)) { | ||
| return "", os.ErrNotExist | ||
| } | ||
|
|
||
| if paths == nil { | ||
| return curPath, nil | ||
| return cleaned, nil | ||
| } | ||
|
|
||
| for _, el := range paths { | ||
| if strings.HasPrefix(curPath, el.Location) { | ||
| return strings.Replace(curPath, el.Location, el.Destination, 1), nil | ||
| if strings.HasPrefix(cleaned, el.Location) { | ||
| mapped := strings.Replace(cleaned, el.Location, el.Destination, 1) | ||
| // Normalize and clean the mapped path as well. | ||
| mapped = filepath.Clean(filepath.FromSlash(mapped)) | ||
| // Reject any absolute or traversing result. | ||
| if filepath.IsAbs(mapped) { | ||
| return "", os.ErrNotExist | ||
| } | ||
| if mapped == ".." || strings.HasPrefix(mapped, ".."+string(os.PathSeparator)) { | ||
| return "", os.ErrNotExist | ||
| } | ||
| return mapped, nil | ||
| } | ||
| } | ||
| return "", os.ErrNotExist |
| } | ||
| tarReader := tar.NewReader(u) | ||
| for { | ||
| header, err := tarReader.Next() |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 28 days ago
In general, the fix is to normalize archive entry paths and ensure they do not escape the intended destination directory before using them in any filesystem operation. That means rejecting paths containing .. path elements, absolute paths, and anything that, after cleaning and joining with destination, does not remain within destination. Symlink targets (Linkname) should be treated similarly if you want to prevent links to arbitrary locations.
The best way to fix this function without changing existing behavior is:
- Introduce a small helper inside
ExtractFromTarthat, given an entry path, returns a safe absolute path underdestinationor an error if the path is unsafe. This helper should:- Use
filepath.Cleanto normalize the entry path. - Reject empty, absolute, or traversal paths (those starting with
..or containing..as a path element). - Join the cleaned name with
destination, getfilepath.Absfor both, and ensure the resulting path hasdestAbsas its prefix (reldoes not start with..and is not absolute).
- Use
- Replace the simple
pathBuilder := func(oldPath string) string { return path.Join(destination, oldPath) }with this validated builder that returns(string, error). - Update all uses:
- For directories and regular files: compute
fullPath, err := pathBuilder(header.Name)and usefullPathinMkdirAll,Lstat,Remove,OpenFile, etc. IfpathBuilderreturns an error, abort extraction with a wrapped error mentioning the entry name. - For symlinks: compute
linkPathfromheader.Nameas above. For the symlink target, either:- Keep current behavior (allow arbitrary targets) but at least keep the link itself within
destinationby validating onlyheader.Name, or - Stricter: also pass
header.Linknamethrough the same validation so links cannot point outsidedestination. Given the vulnerability class, validating both is safer and still consistent with the idea of keeping extraction confined.
- Keep current behavior (allow arbitrary targets) but at least keep the link itself within
- For directories and regular files: compute
- Use
filepathrather thanpathfor actual OS paths, since this is filesystem-related code.
All changes are within evetest/utils/tar.go, in the ExtractFromTar function region. No new imports are needed, because filepath and strings are already imported.
| return fmt.Errorf("ExtractFromTar: cannot remove old symlink: %w", err) | ||
| } | ||
| } | ||
| if err := os.Symlink(pathBuilder(header.Linkname), pathBuilder(header.Name)); err != nil { |
Check failure
Code scanning / CodeQL
Arbitrary file write extracting an archive containing symbolic links High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 28 days ago
In general, fixing this requires validating and constraining all paths derived from tar headers before using them on the filesystem, and for symlinks specifically, ensuring that the final resolved target (after following any existing symlinks) remains within the extraction root. That means: (1) prevent absolute paths and .. traversal from escaping destination, and (2) for symlink creation, check that the symlink’s effective target is still under destination using filepath.EvalSymlinks plus a Rel-based check.
For this code, the minimal, behavior-preserving fix is:
-
Introduce a helper
isRel(candidate, root string) boolthat:- Rejects absolute
candidate. - Resolves
candidaterelative torootusingfilepath.EvalSymlinks(filepath.Join(root, candidate)). - Computes
filepath.Rel(root, resolved)and ensures the result does not start with...
This matches the “GOOD” pattern from the background.
- Rejects absolute
-
Use
filepath(already imported) instead ofpathfor filesystem paths, since this code works with the local filesystem, not URL paths. In particular:- Replace
pathBuilder := func(oldPath string) string { return path.Join(destination, oldPath) }
withfilepath.Join. - This avoids subtle cross-platform issues and uses the right separator semantics.
- Replace
-
Before creating a symlink (and for safety also before creating files or directories), ensure that the target path is within
destination:- Compute
dstPath := pathBuilder(header.Name)and verifyisRel(header.Name, destination); if not, return an error. - For symlinks: verify both the link name and its target:
if !isRel(header.Name, destination) || !isRel(header.Linkname, destination) { return error }
This ensures that neither the symlink itself nor the effective resolved target can escapedestination.
- Compute
-
Keep all other logic (removal of old entries, size limiting, mode handling) unchanged.
All required pieces are contained in evetest/utils/tar.go: we only add a small helper function and adjust the ExtractFromTar implementation to use it and to use filepath.Join instead of path.Join. No new imports are needed because filepath and strings are already imported.
| @@ -18,6 +18,25 @@ | ||
| Destination string | ||
| } | ||
|
|
||
| // isRel checks whether the given candidate path, interpreted as relative to root, | ||
| // resolves (after following all symlinks) to a location within root. | ||
| func isRel(candidate, root string) bool { | ||
| if filepath.IsAbs(candidate) { | ||
| return false | ||
| } | ||
| // Resolve the candidate relative to the root, following any existing symlinks. | ||
| realpath, err := filepath.EvalSymlinks(filepath.Join(root, candidate)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| relpath, err := filepath.Rel(root, realpath) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| relpath = filepath.Clean(relpath) | ||
| return relpath != ".." && !strings.HasPrefix(relpath, ".."+string(filepath.Separator)) | ||
| } | ||
|
|
||
| // MaxDecompressedContentSize is the maximum size of a file that can be written to disk after decompression. | ||
| // This is to prevent a DoS attack by unpacking a compressed file that is too big to be decompressed. | ||
| const MaxDecompressedContentSize = 1024 * 1024 * 1024 // 1 GB | ||
| @@ -157,7 +176,7 @@ | ||
| func ExtractFromTar(u io.Reader, destination string) error { | ||
| // path inside tar is relative | ||
| pathBuilder := func(oldPath string) string { | ||
| return path.Join(destination, oldPath) | ||
| return filepath.Join(destination, oldPath) | ||
| } | ||
| tarReader := tar.NewReader(u) | ||
| for { | ||
| @@ -170,10 +189,16 @@ | ||
| } | ||
| switch header.Typeflag { | ||
| case tar.TypeDir: | ||
| if !isRel(header.Name, destination) { | ||
| return fmt.Errorf("ExtractFromTar: directory path escapes destination: %s", header.Name) | ||
| } | ||
| if err := os.MkdirAll(pathBuilder(header.Name), os.FileMode(header.Mode)); err != nil { | ||
| return fmt.Errorf("ExtractFromTar: Mkdir() failed: %w", err) | ||
| } | ||
| case tar.TypeReg: | ||
| if !isRel(header.Name, destination) { | ||
| return fmt.Errorf("ExtractFromTar: file path escapes destination: %s", header.Name) | ||
| } | ||
| if _, err := os.Lstat(pathBuilder(header.Name)); err == nil { | ||
| err = os.Remove(pathBuilder(header.Name)) | ||
| if err != nil { | ||
| @@ -197,6 +218,13 @@ | ||
| return fmt.Errorf("ExtractFromTar: outFile.Close() failed: %w", err) | ||
| } | ||
| case tar.TypeLink, tar.TypeSymlink: | ||
| // Ensure both the link itself and its target resolve within destination. | ||
| if !isRel(header.Name, destination) { | ||
| return fmt.Errorf("ExtractFromTar: symlink path escapes destination: %s", header.Name) | ||
| } | ||
| if !isRel(header.Linkname, destination) { | ||
| return fmt.Errorf("ExtractFromTar: symlink target escapes destination: %s", header.Linkname) | ||
| } | ||
| if _, err := os.Lstat(pathBuilder(header.Name)); err == nil { | ||
| err = os.Remove(pathBuilder(header.Name)) | ||
| if err != nil { |
| return fmt.Errorf("ExtractFromTar: cannot remove old symlink: %w", err) | ||
| } | ||
| } | ||
| if err := os.Symlink(pathBuilder(header.Linkname), pathBuilder(header.Name)); err != nil { |
Check failure
Code scanning / CodeQL
Arbitrary file write extracting an archive containing symbolic links High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 28 days ago
In general, to fix this class of issues you must ensure that any filesystem path derived from archive headers is validated before use. For symlinks, both the path of the link itself and the link target must be checked. This validation should be done on resolved paths (using filepath.EvalSymlinks or equivalent) to account for any pre‑existing symlinks inside the extraction directory. Only if the fully resolved paths are still within the intended extraction root should the link be created.
For this specific function, we should:
- Introduce a helper
isWithinDestination(candidate, destination string) boolthat:- Rejects absolute
candidatepaths. - Joins
destinationandcandidate. - Calls
filepath.EvalSymlinkson the joined path; if that fails, returns false. - Uses
filepath.Relfromdestinationto the resolved path and ensures the result does not start with..(afterfilepath.Clean).
- Rejects absolute
- Before creating symlinks in the
tar.TypeLink, tar.TypeSymlinkcase, call this helper on bothheader.Name(the link path) andheader.Linkname(the target). If either fails the check, return an error instead of creating the symlink. - When calling
os.Symlink, use the validated, joined values rather than recomputing them ad hoc, so we don’t accidentally diverge from what we checked. - Keep other behavior (directory creation, regular file extraction, decompression limit) unchanged.
All changes are within evetest/utils/tar.go, inside ExtractFromTar. We only need to add the small helper inside that function (or as a nested closure) using existing imports; path/filepath is already imported, so no new dependencies are required.
| @@ -159,6 +159,24 @@ | ||
| pathBuilder := func(oldPath string) string { | ||
| return path.Join(destination, oldPath) | ||
| } | ||
| // isWithinDestination checks that a path from the archive, when resolved, | ||
| // stays within the extraction destination. It also resolves any existing | ||
| // symlinks under the destination. | ||
| isWithinDestination := func(candidate string) bool { | ||
| if filepath.IsAbs(candidate) { | ||
| return false | ||
| } | ||
| joined := pathBuilder(candidate) | ||
| realPath, err := filepath.EvalSymlinks(joined) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| rel, err := filepath.Rel(destination, realPath) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return !strings.HasPrefix(filepath.Clean(rel), "..") | ||
| } | ||
| tarReader := tar.NewReader(u) | ||
| for { | ||
| header, err := tarReader.Next() | ||
| @@ -197,15 +215,22 @@ | ||
| return fmt.Errorf("ExtractFromTar: outFile.Close() failed: %w", err) | ||
| } | ||
| case tar.TypeLink, tar.TypeSymlink: | ||
| if _, err := os.Lstat(pathBuilder(header.Name)); err == nil { | ||
| err = os.Remove(pathBuilder(header.Name)) | ||
| // Validate that both the link path and its target stay within destination, | ||
| // resolving any existing symlinks under destination. | ||
| if !isWithinDestination(header.Name) || !isWithinDestination(header.Linkname) { | ||
| return fmt.Errorf("ExtractFromTar: symlink %q -> %q escapes destination %q", header.Name, header.Linkname, destination) | ||
| } | ||
| linkPath := pathBuilder(header.Name) | ||
| targetPath := pathBuilder(header.Linkname) | ||
| if _, err := os.Lstat(linkPath); err == nil { | ||
| err = os.Remove(linkPath) | ||
| if err != nil { | ||
| return fmt.Errorf("ExtractFromTar: cannot remove old symlink: %w", err) | ||
| } | ||
| } | ||
| if err := os.Symlink(pathBuilder(header.Linkname), pathBuilder(header.Name)); err != nil { | ||
| if err := os.Symlink(targetPath, linkPath); err != nil { | ||
| return fmt.Errorf("ExtractFromTar: Symlink(%s, %s) failed: %w", | ||
| pathBuilder(header.Name), pathBuilder(header.Linkname), err) | ||
| linkPath, targetPath, err) | ||
| } | ||
| default: | ||
| return fmt.Errorf( |
1 participant
Description
Provide a clear and concise description of the changes in this PR and
explain why they are necessary.
If the PR contains only one commit, you will see the commit message above:
fill free to use it under the description section here, if it is good enough.
For Backport PRs, a full description is optional, but please clearly state
the original PR number(s). Use the #{NUMBER} format for that, it makes it easier
to handle with the scripts later. For example:
Title of a backport PR must also follow the following format:
where
x.y-stableis the name of the target stable branch, andOriginal PR titleis the title of the original PR.For example, for a PR that backports a PR with title
Fix the nasty bugtobranch
13.4-stablethe title should be:[13.4-stable] Fix the nasty bug.PR dependencies
List all dependencies of this PR (when applicable, otherwise remove this
section).
How to test and validate this PR
Please describe how the changes in this PR can be validated or verified. For
example:
This will be used
The first is especially important, so, please make sure to provide as much
detail as possible.
If it's covered by an automated test, please mention it here.
Changelog notes
Text in this section will be used to generate the changelog entry for
release notes. The consumers of this are end users, not developers.
So, provide a clear and short description of what is changed in the PR from
the end user perspective. If it changes only tooling or some internal
implementation, put a note like "No user-facing changes" or "None".
PR Backports
For all current LTS branches, please state explicitly if this PR should be
backported or not. This section is used by our scripts to track the backports,
so, please, do not omit it.
Here is the list of current LTS branches (it should be always up to date):
For example, if this PR fixes a bug in a feature that was introduced in 14.5,
you can write:
Also, to the PRs that should be backported into any stable branch, please
add a label
stable.Checklist
For backport PRs (remove it if it's not a backport):
And the last but not least:
check them.
Please, check the boxes above after submitting the PR in interactive mode.