Add HostAccess configuration that disables reflection

[jazdw]

Feature request

Is your feature request related to a problem? Please describe. There are several pre-built HostAccess configurations but none which allow you to access public methods but disable reflection.

Describe the solution you'd like. A HostAccess configuration that allows access to public methods but disables reflection. At it's most basic I think it could look like this -

HostAccess disableReflection = HostAccess.newBuilder()
        .allowPublicAccess(true)
        .allowAllImplementations(true)
        .allowArrayAccess(true)
        .allowListAccess(true)
        .denyAccess(ClassLoader.class)
        .denyAccess(Member.class) // includes Method, Field and Constructor
        .denyAccess(AnnotatedElement.class) // includes Class
        .denyAccess(Proxy.class)
        .denyAccess(Object.class, false) // wait(), notify(), getClass()
        .denyAccess(System.class) // setProperty(), getProperty(), gc(), exit()
        .build();

Note this is based off https://github.com/oracle/graal/blob/8fb8fc502411b3c84308ff5cda4d6c1f775f3920/truffle/src/com.oracle.truffle.api.test/src/com/oracle/truffle/api/test/polyglot/HostAccessTest.java#L120-L127

Another nice option would be the ability to allow/deny based on package.

As a side note, it would also be nice to be able to deny access to methods of class but still allow access to equals(). This may be a bug?

HostAccess disableReflection = HostAccess.newBuilder()
        .allowPublicAccess(true)
        .denyAccess(Object.class, false)
        .allowAccess(Object.class.getMethod("equals", Object.class)) // doesn't work, deny takes precedence
        .build();

Describe who do you think will benefit the most. GraalVM users

Describe alternatives you've considered. Manually build HostAccess using builder, I feel a default configuration would make this easier and potentially help us identify which classes to deny access to.

Additional context. Nashorn applies similar restrictions when a class filter is present - https://docs.oracle.com/en/java/javase/11/nashorn/nashorn-java-api.html#GUID-FBB32DB3-CF59-4016-B690-9581E1406594

Express whether you'd like to help contributing this feature I can if you would like.

[jazdw]

Related issues #987 and #1654. @chumer would probably be interested in this one.

[boris-spas]

Tracking internally as GR-24938

[chumer]

Sorry for the late response.

We had long discussions around this feature when it was originally designed. We decided to deliberately not add a flag to disable reflection. The reason is that we didn't want to encourage usage of such a mode. Just disabling reflection does not make the context "safe", especially if you expose classes of third-party libraries. Those third-party libraries might just expose an additional method that suddenly breaks the sandbox and allows the guest code to access reflection.

For example what if someone writes a class like this?

public static class MyClass {
   public static Object forNameAndInstantiate(String className) {
    return Class.forName(className).newInstance();
  }
}

The only way to safely expose Java methods to a sandboxed context is by manually vetting every single method. In other words you need to list them on an allowed-list explicitly. It is generally not recommended to expose methods of 3rd party code as this can open up a new breakage to the sandbox in just minor version change. That is why the default mode for polyglot contexts is HostAccess.EXPLICIT where you annotate every method that is allowed to be accessed.

Feel free to reopen if you want to discuss further.

[jazdw]

@chumer I can't re-open it, hopefully you read this

The reason is that we didn't want to encourage usage of such a mode. Just disabling reflection does not make the context "safe", especially if you expose classes of third-party libraries. Those third-party libraries might just expose an additional method that suddenly breaks the sandbox and allows the guest code to access reflection.

I want to use this for my own codebase, not a 3rd party library. I just don't want to have to manually annotate every class that I expose. I am aware that I need to vet the methods of these classes.

Honestly people are going to look for ways to do this regardless, it will result in people baking their own configurations which leave holes. If there was a pre-built configuration at least we could prevent the most obvious avenues for exploitation. It would also give the benefit that if some other loophole was found it could be added to the configuration and everyone would benefit when they upgrade.

At least can you consider the other two comments that I made -

• Package based allow/deny
• Deny access to class takes precedence over allow access to method within that class - there is no way to blacklist every method from a class except one or two

[chumer]

Honestly people are going to look for ways to do this regardless, it will result in people baking their own configurations which leave holes. If there was a pre-built configuration at least we could prevent the most obvious avenues for exploitation. It would also give the benefit that if some other loophole was found it could be added to the configuration and everyone would benefit when they upgrade.

That is a false sense of security. If you insist you can do it yourself with a custom configuration, but you are on your own with that. If we give people an API for disabling reflection they expect it to be reliable. I already posted a loophole example where we cannot do anything against.

Package based allow/deny

We can support denyPackage but not allowPackage. Note you can deny lookup of additional classes using class filters.

Deny access to class takes precedence over allow access to method within that class - there is no way to blacklist every method from a class except one or two

We want to encourage allow lists. We consider deny lists not safe.

As a side note, it would also be nice to be able to deny access to methods of class but still allow access to equals(). This may be a bug?

HostAccess disableReflection = HostAccess.newBuilder()
        .allowPublicAccess(true)
        .denyAccess(Object.class, false)
        .allowAccess(Object.class.getMethod("equals", Object.class)) // doesn't work, deny takes precedence
        .build();

Yes that is unexpected behavior, we can probably fix. Currently we don't further check if a class is excluded whether it is included explicitly. Look in HostAccess#allowsAccess for an intuition how this works. In the meantime can you just deny all methods of Object you want to exclude?

[jazdw]

That is a false sense of security. If you insist you can do it yourself with a custom configuration, but you are on your own with that. If we give people an API for disabling reflection they expect it to be reliable. I already posted a loophole example where we cannot do anything against.

OK, fair enough, you want to transfer the onus to the user. I think at least an example somewhere would be advantageous, with a warning of course.

We can support denyPackage but not allowPackage. We want to encourage allow lists. We consider deny lists not safe.

These statements seem contradictory. Typo? allowPackage would be great!

Note you can deny lookup of additional classes using class filters.

Yep, I know of the class filter, I have disabled lookup for all classes. I do not want to allow the lookup of any classes using Java.type(), I simply want to enable the access to public methods on any instances I explicitly insert into the script engine's context.

Yes that is unexpected behavior, we can probably fix. Currently we don't further check if a class is excluded whether it is included explicitly. Look in HostAccess#allowsAccess for an intuition how this works. In the meantime can you just deny all methods of Object you want to exclude?

OK thanks. Yeah I can definitely work around it, not a huge blocker for me.

[chumer]

These statements seem contradictory. Typo? allowPackage would be great!

Right this can be interpreted as contradiction. I don't think we want to add allowPackage for the same reason we don't want allowClass, they have too low granularity. Code bases are in flux, and typically the next maintainer does not remember that a class is exposed to the guest script, so he just adds a new public method. And accidentally this method will be fully exposed. So in order to make this safe we assume there is some vetting process in place. The vetting is typically done by either adding an annotation or adding a call to HostAccess.Builder#allowAccess.

I can see how this can be inconvenient if you have an established convention that a full package is exposed. However at the same time we would like to encourage explicit vetting. Is there any reason you cannot use the HostAccess.Export annotation?

[jazdw]

I can see how this can be inconvenient if you have an established convention that a full package is exposed. However at the same time we would like to encourage explicit vetting.

Yeah you have hit the nail on the head, we have a set of singleton instances that by convention we say are safe to use from the scripting context.

Is there any reason you cannot use the HostAccess.Export annotation?

• There are hundreds of methods, I don't like the idea of annotating/whitelisting each and every one of these
• The methods are pre-existing, we used them from Nashorn previously, they have been vetted
• Our application is modular, a lot of the classes/methods that we want to expose access to are in the core application where the annotation is not on the classpath (we add GraalJS via a module and access it via the JSR-233 interface). Note: when I say module I am not referring to JPMS modules

Currently it is possible to configure HostAccess to allow access to all public methods then selectively deny access to specific classes. I don't see how adding a way to selectively allow access only to certain classes'/packages' public methods is in any way less secure.

[chumer]

Our application is modular, a lot of the classes/methods that we want to expose access to are in the core application where the annotation is not on the classpath

For that purpose we have designed a way to specify your own annotation using HostAccess.Builder#allowAccessAnnotatedBy

There are hundreds of methods, I don't like the idea of annotating/whitelisting each and every one of these

Want to elaborate? Hundreds sounds reasonable. It is a one-time thing. And makes exposure to scripts explicit for future code changes.

I don't see how adding a way to selectively allow access only to certain classes'/packages' public methods is in any way less secure.

I think I have elaborated my point, with code bases that are in flux and people in the loop.

There are hundreds of methods, I don't like the idea of annotating/whitelisting each and every one of these

If you disagree strongly with our approach to this. Maybe a build time step where you can generate a list of methods exposed that are later registered? There shouldn't be any fundamental blockers to this. HostAccess.Builder is very flexible.

[matneu]

@jazdw the HostAccess feature is actually designed to help embedders such as you to create secure applications by applying the principle of least privilege. Hence the deliberately restrictive model that requires very conscious decisions to expose host code to guest applications.

[jazdw]

For that purpose we have designed a way to specify your own annotation using HostAccess.Builder#allowAccessAnnotatedBy

Ah I wasn't aware of that one. Thanks.

Want to elaborate? Hundreds sounds reasonable. It is a one-time thing. And makes exposure to scripts explicit for future code changes.

Yeah but again, there is an understanding that any public methods in these classes are accessible via script. I don't want to pollute the codebase with hundreds/thousands of annotations: at the end of the day your developers need to be conscious of what they are doing.

I think I have elaborated my point, with code bases that are in flux and people in the loop.

Yeah look I understand why you might want to use method level annotations, but why not let the end user choose what policy they want to implement? Use cases vary. You have a way to allow access to all public methods of all classes so you must acknowledge this. At least a package/class level whitelisting is an improvement over full public without going down to method level granularity.

If you disagree strongly with our approach to this. Maybe a build time step where you can generate a list of methods exposed that are later registered? There shouldn't be any fundamental blockers to this.

Yeah this is possible but seems a bit like jumping through hoops.

HostAccess.Builder is very flexible.

This I would disagree with, its very much all or nothing.

[chumer]

You have a way to allow access to all public methods of all classes so you must acknowledge this. At least a package/class level whitelisting is an improvement over full public without going down to method level granularity.

If you enable public access

/**
         * Allows unrestricted access to all public constructors, methods or fields of public
         * classes. Note that this policy allows unrestricted access to reflection. It is highly
         * discouraged from using this option in environments where the guest application is not
         * fully trusted.
         *
         * @since 19.0
         */

That is indeed equivalent to doing HostAccess.ALL. Any other wildcard mode, like package inclusion, or class inclusion would suffer from the exactly same problem and we would need to put the same disclaimer, that it effectively provides access to everything.

Yeah look I understand why you might want to use method level annotations, but why not let the end user choose what policy they want to implement?

I don't like features that encourage users to do the wrong thing. Adding a class or adding a method is not an explicit agreement to exposing something to a script. You clearly care about what is exposed to the scripts so why not make it explicit? I mean it has a lot of other implications, like compatibility too.

[jazdw]

That is indeed equivalent to doing HostAccess.ALL. Any other wildcard mode, like package inclusion, or class inclusion would suffer from the exactly same problem and we would need to put the same disclaimer, that it effectively provides access to everything.

I have absolutely no qualms whatsoever with putting the same notice on any method which allows you to whitelist a class or package.

I don't like features that encourage users to do the wrong thing. Adding a class or adding a method is not an explicit agreement to exposing something to a script.

I mean this is the issue, you consider it to be the wrong thing. I do not. Just give me the option, I am not asking you to change your default security model, I just want the flexibility to make my own choices.

Here's another thought: we are able to provide a classFilter Predicate, why not have the same option for allowing access to public methods?

You clearly care about what is exposed to the scripts so why not make it explicit? I mean it has a lot of other implications, like compatibility too.

Different scenarios dictate different levels of acceptable risk. I trust our developers and processes enough to say that this is an acceptable level of risk. Compatibility we deal with separately (release notes, no breaking changes in minor patches etc) though that is a good point.

Anyway, at this point I am starting to feel like this is flogging a dead horse. I have stated my case.

[chumer]

Here's another thought: we are able to provide a classFilter Predicate, why not have the same option for allowing access to public methods?

Hm... We could do HostAccess.Builder#allowPublicAccess((className) -> ...). That would not require an additional method and at least would not make the situation worse. What do you think @matneu ?

[matneu]

We could do HostAccess.Builder#allowPublicAccess((className) -> ...). That would not require an additional method and at least would not make the situation worse.

Sounds good to me!

[chumer]

Alright. I will try to sneak this feature into 20.3. Unfortunately the window for 20.2 closed today.

[jazdw]

Alright. I will try to sneak this feature into 20.3. Unfortunately the window for 20.2 closed today.

Thanks for taking it on. Do you think the predicate will take a class name as a string or a Class object? I would imagine that at the moment the check is performed you have the instance of the object and know its Class.

[chumer]

Class could work here, yes. Would provide the most flexibility.

[o-nix]

Hey guys, just stumbled upon this, had to apply the .denyAccess(Object.class, false) partial solution. Please say there are still plans to add HostAccess.Builder#allowPublicAccess((className) -> ...), thanks in advance! 🤞