bug: potential side effect in getTypeFromComponents function

[nullifysecurity]

Good morning Mike,

I believe there is a bug in path_specs.FromGenericComponentList that can unintentionally mutate the underlying array backing the Components slice of a *accessors.OSPath passed to it. This side effect can cause subsequent operations using the same OSPath instance to fail.

I first observed this behavior in v0.75.6 while investigating failures in upload_azure, where accessor.LstatWithOSPath was unable to stat a file when provided a filestore path using the fs accessor.

Observed Behavior

When passing a filestore path (for example, one returned from create_flow_download) to the file argument of upload_azure, the following error is produced:

fs:/downloads/C.3c4f2d6cfc5d7219/F.D5NVNO28H7U54/DESKTOP-HLOF40N-C.3c4f2d6cfc5d7219-F.D5NVNO28H7U54.zip

upload_azure: Unable to stat fs:/downloads/C.3c4f2d6cfc5d7219/F.D5NVNO28H7U54/DESKTOP-HLOF40N-C.3c4f2d6cfc5d7219-F.D5NVNO28H7U54: stat /tmp/gui_datastore/downloads/C.3c4f2d6cfc5d7219/F.D5NVNO28H7U54/DESKTOP-HLOF40N-C.3c4f2d6cfc5d7219-F.D5NVNO28H7U54: no such file or directory

Root Cause Analysis

The issue appears to originate in the private getTypeFromComponents function, which is called by path_specs.FromGenericComponentList:

velociraptor/file_store/path_specs/utils.go

Lines 110 to 114 in 9096e25

	last_component := components[len(components)-1]
	// Fallback, use the extension to deduce the type.
	fs_type, name := api.GetFileStorePathTypeFromExtension(last_component)
	return append(components[:len(components)-1], name), fs_type

In this function, the return value appends the file name (minus its extension) to the last index of the components slice. Because this slice shares its underlying array with the Components field of the *accessors.OSPath passed in, the modification leaks back into the original OSPath.

While this mutation is not inherently problematic in isolation, it becomes an issue when the same *accessors.OSPath is reused across multiple calls that have different expectations about the path’s suffix.

Concrete Example

In the azure_upload Call function, the same arg.File (*accessors.OSPath) is used for both opening and statting the file:

// Before OpenWithOSPath:
// &accessors.OSPath{Components:[]string{
//   "downloads",
//   "C.3c4f2d6cfc5d7219",
//   "F.D5NVNO28H7U54",
//   "DESKTOP-HLOF40N-C.3c4f2d6cfc5d7219-F.D5NVNO28H7U54.zip",
// }}

file, err := accessor.OpenWithOSPath(arg.File)

After OpenWithOSPath (which calls FromGenericComponentList), the extension has been stripped from the final component:

// After OpenWithOSPath:
// &accessors.OSPath{Components:[]string{
//   "downloads",
//   "C.3c4f2d6cfc5d7219",
//   "F.D5NVNO28H7U54",
//   "DESKTOP-HLOF40N-C.3c4f2d6cfc5d7219-F.D5NVNO28H7U54",
// }}

Later, the same arg.File is passed to accessor.LstatWithOSPath, which expects the full filename (including the extension). Because the suffix has been removed, the stat call fails.

Proposed Fix

One possible solution is to clone the components slice in getTypeFromComponents before returning the modified version, ensuring the original slice (and its backing array) remain untouched:

fs_type, name := api.GetFileStorePathTypeFromExtension(last_component)

clone := slices.Clone(components[:len(components)-1])
return append(clone, name), fs_type

This avoids mutating shared state at the cost of a small allocation per call.

Open Questions

I’m happy to submit a pull request with this change, but I wanted to check first in case there’s a preferred or more idiomatic approach that avoids the extra allocation or addresses the issue at a higher level. It’s very possible I’m missing some nuance in how these paths are intended to be handled.

Please let me know your thoughts, thank you!

[scudette]

It should be fine to make a copy of the slice. The path spec abstraction is supposed to be immutable.

Thanks for finding this issue. Maybe we should quickly look for similar issues in the same code

[nullifysecurity]

Thanks Mike, I'll have a look for any other issues from code introduced in #4592.

Since the path spec abstraction is meant to be immutable, is it worth refactoring the accessor.OSPath struct to enforce immutability? For example, making the components field private and providing a GetComponents() or Components() function which returns a copy of the slice?

I just had a look at the references to OSPath.Components though and it looks like it'd be a fairly significant refactor.

Edit: I see that the _PathSpec interface already implements a similar functionality, however, just returns the private field and not a copy of the struct.

[scudette]

I'm not sure it's worth the extra cost of making the components copy each access. We just need to be careful to not change it directly.

[nullifysecurity]

Yeah, for sure, that's totally fair.

I've just had a quick look at the other code and can't see anything that sticks out to me. I'll write up a pull request for this fix. 😄