Link Detection Stack Depth FailSafe (#159)

* test: add failing test for cycle case Signed-off-by: Christopher Phillips <[email protected]> * test: test updates sym links Signed-off-by: Christopher Phillips <[email protected]> * change the filetree recursive pathset to represent open calls Signed-off-by: Alex Goodman <[email protected]> * add another cycle test Signed-off-by: Alex Goodman <[email protected]> * change filetree attempting path set to counters Signed-off-by: Alex Goodman <[email protected]> * remove comment Signed-off-by: Alex Goodman <[email protected]> * fix linting Signed-off-by: Alex Goodman <[email protected]> * feat: decrement stack depth Signed-off-by: Christopher Phillips <[email protected]> * test: remove old wip test name Signed-off-by: Christopher Phillips <[email protected]> * chore: style updates Signed-off-by: Christopher Phillips <[email protected]> * feat: move maxLinkDepth decrement to inside ancestor loop Signed-off-by: Christopher Phillips <[email protected]> * feat: move maxLinkDepth decrement into resolveNodeLinks loop Signed-off-by: Christopher Phillips <[email protected]> * feat: move detection to top and write minimal test case Signed-off-by: Christopher Phillips <[email protected]> * test: update linkResolution test to use internal value Signed-off-by: Christopher Phillips <[email protected]> --------- Signed-off-by: Christopher Phillips <[email protected]> Signed-off-by: Alex Goodman <[email protected]> Co-authored-by: Alex Goodman <[email protected]>
anchore · Feb 22, 2023 · fab1c96 · fab1c96
1 parent 529924d
commit fab1c96
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 8 deletions.
diff --git a/pkg/filetree/filetree.go b/pkg/filetree/filetree.go
@@ -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 {
@@ -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
@@ -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
@@ -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{})
@@ -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
@@ -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")
 	}
@@ -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.
@@ -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)
@@ -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...)

diff --git a/pkg/filetree/filetree_test.go b/pkg/filetree/filetree_test.go
@@ -2,6 +2,7 @@ package filetree
 
 import (
 	"errors"
+	"fmt"
 	"github.com/scylladb/go-set/strset"
 	"testing"
 
@@ -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()