On 2019-08-15 12:56, Alan Bateman wrote:
On 15/08/2019 11:03, Claes Redestad wrote:
I think the approach is okay as URL::openConnection doesn't actually
open the connection to the resource and the creation of the
URLStreamdHandler shouldn't depend on permissions granted to the caller.
If a handler needs permissions when creating the URLConnection then it
should do so in a privileged block.
by resolving permissions for code source URLs lazily, we can reduce
early class loading during bootstrap, which improves footprint, startup
and reduces the typical bootstrap dependency graph.
I checked most of the handlers and the openConnection implementations
and couldn't find any path that isn't either free of permission checks
or doing permission sensitive steps in a privileged block already. Many
handlers are already potentially created lazily after a SM has already
been installed, so I don't think we need additional tests for this.
I think it would be a bit cleaner if
the supporting class would lazily add the permissions for a CodeSource
to the collection. That is, create it with a permissions collection and
code source rather than a URL to match
SecureClassLoader::getPermissions. You could potentially use it in
URLClassLoader getPermission(CodeSource) method too.
That'd be cool. The logic in URLClassLoader is mostly a super-set of the
logic I'm hoisting from BuiltinClassLoader here, but the logic in
URLClassLoader also has a security check acting on the ACC of the
classloader which would be hard to move to the supporting class, and it
seems that check need to happen eagerly.
I'll readjust the API to wrap the CodeSource rather than the URL, but I
think we should leave URLClassLoader alone for now.
In System.setSecurityManager then you might need
DefaultFileSystemProvider.theFileSystem() to ensure that the default
file system is fully initialized before setting the SM.
Right, doesn't make much of a difference since all providers currently
set up their singleton file system on creation, but using theFileSystem
better captures intent.
A minor nit this adds a spurious import BuiltinClassLoader. Also it can
import the sun.security.uti class to be consistent with the existing code.
Cleaned up imports, too: