-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[Entitlements] Add URLConnection instrumentation for file protocol #123824
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
[Entitlements] Add URLConnection instrumentation for file protocol #123824
Conversation
|
|
||
| private void checkURLFileRead(Class<?> callerClass, URL url) { | ||
| try { | ||
| policyManager.checkFileRead(callerClass, Paths.get(url.toURI())); |
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 it be better/safer to use url.getFile() here instead?
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.
@rjernst I'd like your input here, wdyt? Is it OK as is, or is using url.getFile() better?
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.
The idea behind the current implementation is that if we are getting the URL from a FileURLConnection, so the URL must be a valid File URL already; and even if we are give an invalid File URL, the method we are instrumenting would eventually fail anyway, so we don't care about re-throwing the exception.
The idea behind using url.getFile() instead is that this does not throw; it simply returns a (possibly invalid) String, that we will use to create a Path and then pass to the PolicyManager/FileAccessTree (which will do a string-matching check, so we don't really care if it's valid or not). While cratering a Path we can still get a InvalidPathException, but that is much more unlikely (there are just a bunch of character that cannot be in a path in the various OSes).
An third alternative is to catch the syntax exception and re-throw a NotEntitledException, much like I want to do with the next PR (Jar) once we cover all the protocols we want to cover: in case we "see" a different protocol, or in general some unknown URL (not file, not network), we block it (see
Line 685 in a407e99
| private void checkEntitlementForUrl(Class<?> callerClass, URL that) { |
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.
I believe what you have (using Paths.get on the uri) is the correct way. It's what we do in other places, eg in JarHell.
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.
You might be right that trying to get the path out of the URL directly can work without needing to catch exceptions, though it should be getPath() instead (see the docs on getFile(), it can contain query stuff as well).
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.
Thanks Ryan. I'll keep Paths for now, and make a mental note to use getPath() if I spot anything even remotely suspicious.
…g-url-connection-3
…g-url-connection-3
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
This |
…g-url-connection-3
…g-url-connection-3
…ematte/elasticsearch into entitlements/missing-url-connection-3
💔 Backport failed
You can use sqren/backport to manually backport by running |
…astic#123861) Follows elastic#123824 Relates to ES-10994
…astic#123861) Follows elastic#123824 Relates to ES-10994
…astic#123861) Follows elastic#123824 Relates to ES-10994
…astic#123861) Follows elastic#123824 Relates to ES-10994
Based on #123503
Relates to ES-10994