From fab1c9638abc2c21cd53dca1f205f37d71148ee0 Mon Sep 17 00:00:00 2001 From: Christopher Angelo Phillips <32073428+spiffcs@users.noreply.github.com> Date: Wed, 22 Feb 2023 13:59:48 -0500 Subject: [PATCH] Link Detection Stack Depth FailSafe (#159) * test: add failing test for cycle case Signed-off-by: Christopher Phillips * test: test updates sym links Signed-off-by: Christopher Phillips * change the filetree recursive pathset to represent open calls Signed-off-by: Alex Goodman * add another cycle test Signed-off-by: Alex Goodman * change filetree attempting path set to counters Signed-off-by: Alex Goodman * remove comment Signed-off-by: Alex Goodman * fix linting Signed-off-by: Alex Goodman * feat: decrement stack depth Signed-off-by: Christopher Phillips * test: remove old wip test name Signed-off-by: Christopher Phillips * chore: style updates Signed-off-by: Christopher Phillips * feat: move maxLinkDepth decrement to inside ancestor loop Signed-off-by: Christopher Phillips * feat: move maxLinkDepth decrement into resolveNodeLinks loop Signed-off-by: Christopher Phillips * feat: move detection to top and write minimal test case Signed-off-by: Christopher Phillips * test: update linkResolution test to use internal value Signed-off-by: Christopher Phillips --------- Signed-off-by: Christopher Phillips Signed-off-by: Alex Goodman Co-authored-by: Alex Goodman --- pkg/filetree/filetree.go | 24 +++++++---- pkg/filetree/filetree_test.go | 75 +++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/pkg/filetree/filetree.go b/pkg/filetree/filetree.go index 141040c8..e2ea64a5 100644 --- 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 index fff09810..a6aac4a7 100644 --- 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()