Skip to content

Commit

Permalink
Link Detection Stack Depth FailSafe (anchore#159)
Browse files Browse the repository at this point in the history 
* test: add failing test for cycle case

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* test: test updates sym links

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* change the filetree recursive pathset to represent open calls

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* add another cycle test

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* change filetree attempting path set to counters

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* remove comment

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* fix linting

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* feat: decrement stack depth

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* test: remove old wip test name

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* chore: style updates

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* feat: move maxLinkDepth decrement to inside ancestor loop

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* feat: move maxLinkDepth decrement into resolveNodeLinks loop

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* feat: move detection to top and write minimal test case

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* test: update linkResolution test to use internal value

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

---------

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Co-authored-by: Alex Goodman <alex.goodman@anchore.com>
  • Loading branch information
@spiffcs @wagoodman @gnmahanth
2 people authored and gnmahanth committed Jun 15, 2023
1 parent 66b650a commit c032eae
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 8 deletions.
24 changes: 16 additions & 8 deletions pkg/filetree/filetree.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (

var ErrRemovingRoot = errors.New("cannot remove the root path (`/`) from the FileTree")
var ErrLinkCycleDetected = errors.New("cycle during symlink resolution")
var ErrLinkResolutionDepth = errors.New("maximum link resolution stack depth exceeded")
var maxLinkResolutionDepth = 100

// FileTree represents a file/directory Tree
type FileTree struct {
Expand Down Expand Up @@ -213,7 +215,7 @@ func (t *FileTree) node(p file.Path, strategy linkResolutionStrategy) (*nodeAcce
var currentNode *nodeAccess
var err error
if strategy.FollowAncestorLinks {
currentNode, err = t.resolveAncestorLinks(normalizedPath, nil)
currentNode, err = t.resolveAncestorLinks(normalizedPath, nil, maxLinkResolutionDepth)
if err != nil {
if currentNode != nil {
currentNode.RequestPath = normalizedPath
Expand All @@ -239,7 +241,7 @@ func (t *FileTree) node(p file.Path, strategy linkResolutionStrategy) (*nodeAcce
}

if strategy.FollowBasenameLinks {
currentNode, err = t.resolveNodeLinks(currentNode, !strategy.DoNotFollowDeadBasenameLinks, nil)
currentNode, err = t.resolveNodeLinks(currentNode, !strategy.DoNotFollowDeadBasenameLinks, nil, maxLinkResolutionDepth)
}
if currentNode != nil {
currentNode.RequestPath = normalizedPath
Expand All @@ -250,7 +252,7 @@ func (t *FileTree) node(p file.Path, strategy linkResolutionStrategy) (*nodeAcce

// return FileNode of the basename in the given path (no resolution is done at or past the basename). Note: it is
// assumed that the given path has already been normalized.
func (t *FileTree) resolveAncestorLinks(path file.Path, currentlyResolvingLinkPaths file.PathCountSet) (*nodeAccess, error) {
func (t *FileTree) resolveAncestorLinks(path file.Path, currentlyResolvingLinkPaths file.PathCountSet, maxLinkDepth int) (*nodeAccess, error) {
// performance optimization... see if there is a node at the path (as if it is a real path). If so,
// use it, otherwise, continue with ancestor resolution
currentNodeAccess, err := t.node(path, linkResolutionStrategy{})
Expand Down Expand Up @@ -306,7 +308,7 @@ func (t *FileTree) resolveAncestorLinks(path file.Path, currentlyResolvingLinkPa
// links until the next Node is resolved (or not).
isLastPart := idx == len(pathParts)-1
if !isLastPart && currentNodeAccess.FileNode.IsLink() {
currentNodeAccess, err = t.resolveNodeLinks(currentNodeAccess, true, currentlyResolvingLinkPaths)
currentNodeAccess, err = t.resolveNodeLinks(currentNodeAccess, true, currentlyResolvingLinkPaths, maxLinkDepth)
if err != nil {
// only expected to happen on cycles
return currentNodeAccess, err
Expand All @@ -325,7 +327,7 @@ func (t *FileTree) resolveAncestorLinks(path file.Path, currentlyResolvingLinkPa
// resolveNodeLinks takes the given FileNode and resolves all links at the base of the real path for the node (this implies
// that NO ancestors are considered).
// nolint: funlen
func (t *FileTree) resolveNodeLinks(n *nodeAccess, followDeadBasenameLinks bool, currentlyResolvingLinkPaths file.PathCountSet) (*nodeAccess, error) {
func (t *FileTree) resolveNodeLinks(n *nodeAccess, followDeadBasenameLinks bool, currentlyResolvingLinkPaths file.PathCountSet, maxLinkDepth int) (*nodeAccess, error) {
if n == nil {
return nil, fmt.Errorf("cannot resolve links with nil Node given")
}
Expand All @@ -341,7 +343,6 @@ func (t *FileTree) resolveNodeLinks(n *nodeAccess, followDeadBasenameLinks bool,
var lastNode *nodeAccess
var nodePath []nodeAccess
var nextPath file.Path

currentNodeAccess := n

// keep resolving links until a regular file or directory is found.
Expand All @@ -350,6 +351,13 @@ func (t *FileTree) resolveNodeLinks(n *nodeAccess, followDeadBasenameLinks bool,
realPathsVisited := strset.New()
var err error
for {
// we need to short-circuit link resolution that never resolves (depth) due to a cycle referencing
// maxLinkDepth is counted across all calls to resolveAncestorLinks and resolveNodeLinks
maxLinkDepth--
if maxLinkDepth < 1 {
return nil, ErrLinkResolutionDepth
}

nodePath = append(nodePath, *currentNodeAccess)

// if there is no next path, return this reference (dead link)
Expand Down Expand Up @@ -390,10 +398,10 @@ func (t *FileTree) resolveNodeLinks(n *nodeAccess, followDeadBasenameLinks bool,
return nil, ErrLinkCycleDetected
}

// get the next Node (based on the next path)a
// get the next Node (based on the next path)
// attempted paths maintains state across calls to resolveAncestorLinks
currentlyResolvingLinkPaths.Add(nextPath)
currentNodeAccess, err = t.resolveAncestorLinks(nextPath, currentlyResolvingLinkPaths)
currentNodeAccess, err = t.resolveAncestorLinks(nextPath, currentlyResolvingLinkPaths, maxLinkDepth)
if err != nil {
if currentNodeAccess != nil {
currentNodeAccess.LeafLinkResolution = append(currentNodeAccess.LeafLinkResolution, nodePath...)
Expand Down
75 changes: 75 additions & 0 deletions pkg/filetree/filetree_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package filetree

import (
"errors"
"fmt"
"github.com/scylladb/go-set/strset"
"testing"

Expand Down Expand Up @@ -1136,11 +1137,85 @@ func TestFileTree_File_ResolutionWithMultipleAncestorResolutionsForSameNode(t *t

exists, resolution, err := tr.File("/usr/local/bin/ksh", FollowBasenameLinks)
require.NoError(t, err)
/*
/usr/bin/ksh93 <-- Real file
/bin -> /usr/bin
/usr/bin/ksh -> /etc/alternatives/ksh
/etc/alternatives/ksh -> /bin/ksh93
*/

// ls -al /usr/local/bin/ksh
// /usr/local/bin/ksh -> /bin/ksh

// ls -al /bin/ksh
// /bin/ksh -> /etc/alternatives/ksh

// ls -al /etc/alternatives/ksh
// /etc/alternatives/ksh -> /bin/ksh93

// the test.... we should not stop when a small cycle for /usr/bin is done more than once
_, _, err = tr.File("/usr/local/bin/ksh", FollowBasenameLinks)
require.NoError(t, err)
assert.True(t, exists)
assert.True(t, resolution.HasReference())
assert.Equal(t, *actualRef, *resolution.Reference)
}

func TestFileTreeMaxLinkDepth(t *testing.T) {
tr := New()
_, err := tr.AddFile("/usr/bin/ksh93")
require.NoError(t, err)

_, err = tr.AddSymLink("/usr/local/bin/ksh", "/bin/ksh")
require.NoError(t, err)

_, err = tr.AddSymLink("/bin", "/usr/bin")
require.NoError(t, err)

_, err = tr.AddSymLink("/etc/alternatives/ksh", "/bin/ksh93")
require.NoError(t, err)

_, err = tr.AddSymLink("/usr/bin/ksh", "/etc/alternatives/ksh")
require.NoError(t, err)
rs := linkResolutionStrategy{}

currentNode, err := tr.node("/usr/local/bin/ksh", rs)
require.NoError(t, err)

_, err = tr.resolveNodeLinks(currentNode, !rs.DoNotFollowDeadBasenameLinks, nil, 2)
require.Error(t, err, "should have gotten an error on resolution of a dead cycle")
// require certain error
if err != ErrLinkResolutionDepth {
t.Fatalf("should have gotten an ErrLinkResolutionDepth resolving a file")
}
}

func TestFileTree_MaximumLinkResolutionExceeded(t *testing.T) {
tr := New()
ref, err := tr.AddFile("/usr/bin/ksh")

// we hard code this in filetree to 100
for i := 0; i < maxLinkResolutionDepth; i++ {
_, err = tr.AddSymLink(
file.Path(fmt.Sprintf("/usr/bin/ksh%d", i)),
file.Path(fmt.Sprintf("/usr/bin/ksh%d", i+1)),
)
require.NoError(t, err)
}

// explicitly link 10 to ksh
_, err = tr.AddSymLink("/usr/bin/ksh100", "/usr/bin/ksh")

// we should be short-circuiting the resolution of multiple SymLinks > 100
_, _, err = tr.File("/usr/bin/ksh0", FollowBasenameLinks)
require.Error(t, err)

b, fr, err := tr.File("/usr/bin/ksh90", FollowBasenameLinks)
require.NoError(t, err)
assert.True(t, b)
assert.Equal(t, ref, fr.Reference)
}

func TestFileTree_AllFiles(t *testing.T) {
tr := New()

Expand Down

0 comments on commit c032eae

Please sign in to comment.