Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.17][Security Solution][Endpoint] Fix artifact path file name checking utility #131085

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -558,4 +558,24 @@ describe('Executable filenames with wildcard PATHS', () => {
})
).toEqual(false);
});

it('should return FALSE when WINDOWS wildcards paths do not have a file name', () => {
expect(
hasSimpleExecutableName({
os: OperatingSystem.WINDOWS,
type: 'wildcard',
value: 'c:\\folder\\',
})
).toEqual(false);
});

it('should TRUE when WINDOWS wildcards paths `type` is not `wildcard`', () => {
expect(
hasSimpleExecutableName({
os: OperatingSystem.WINDOWS,
type: 'match',
value: 'c:\\folder\\one.exe',
})
).toEqual(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case with a very large path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I forgot about that. I'll add one next.

});
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,13 @@ export const getDuplicateFields = (entries: ConditionEntry[]) => {
.map((entry) => entry[0]);
};

/*
* regex to match executable names
* starts matching from the eol of the path
* file names with a single or multiple spaces (for spaced names)
* and hyphens and combinations of these that produce complex names
* such as:
* c:\home\lib\dmp.dmp
* c:\home\lib\my-binary-app-+/ some/ x/ dmp.dmp
* /home/lib/dmp.dmp
* /home/lib/my-binary-app+-\ some\ x\ dmp.dmp
/**
* checks if the filename of a given path (if any) is a simple executable (does NOT have the
* wildcards supported by endpoing (`*` and `?`))
* @param os
* @param type
* @param value
*/
const WIN_EXEC_PATH = /\\(\w+|\w*[\w+|-]+\/ +)+\w+[\w+|-]+\.*\w+$/i;
const UNIX_EXEC_PATH = /(\/|\w*[\w+|-]+\\ +)+\w+[\w+|-]+\.*\w*$/i;

export const hasSimpleExecutableName = ({
os,
type,
Expand All @@ -57,10 +50,18 @@ export const hasSimpleExecutableName = ({
type: TrustedAppEntryTypes;
value: string;
}): boolean => {
if (type === 'wildcard') {
return os === OperatingSystem.WINDOWS ? WIN_EXEC_PATH.test(value) : UNIX_EXEC_PATH.test(value);
if (type !== 'wildcard') {
return true;
}
return true;

const separator = os === OperatingSystem.WINDOWS ? '\\' : '/';
const lastString = value.split(separator).pop();

if (!lastString) {
return false;
}

return /[\*\?]/.test(lastString) === false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be return !/[\*\?]/.test(lastString), but I think the current one reads better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use https://github.com/uhop/node-re2 here instead of the built-in RegExp library to further protect ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kobelb - thanks for the comment. The change here (almost all of it) is actually a copy of the code in main - we had already altered this method in main for 8.2 and we know it works. Do you know if node-re2 can be used both in browser and server side? this utility method is used in both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kobelb - we removed the RegEx here entirely for this 7.17.4 patch which goes out soon. We'll look into node-re2 when we've got a bit more time.

Related change: 0dc084b

};

export const isPathValid = ({
Expand Down