-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[7.17][Security Solution][Endpoint] Fix artifact path
file name checking utility
#131085
Conversation
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
return false; | ||
} | ||
|
||
return /[\*\?]/.test(lastString) === false; |
There was a problem hiding this comment.
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.
return false; | ||
} | ||
|
||
return /[\*\?]/.test(lastString) === false; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value: 'c:\\folder\\one.exe', | ||
}) | ||
).toEqual(true); | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see how it looks with the re2 package suggested but other than that it LGTM!
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
Checked it out and tried it. Added several long paths with and without Ex:
LGTM! EDIT: I also verified that regular, expected wildcard scenarios still work in Trusted Apps with mimikatz. |
Summary
RegExp
of thehasSimpleExecutableName()
utility that was causing an infinite loop when used from Artifact Manifest manager and in turn preventing access to the entire Kibana system.About the bug:
With long paths defined with the
matches
operator defined for a Trusted App, BUT having no wildcards in the path, the artifact builder task does not complete and causes a Kibana TLS handshake timeout.Checklist