Issue with locking down supressAccessChecks with Jackson Databind

[shorea]

Customer reported issue with the AWS SDK for Java (aws/aws-sdk-java#528) asking for ability to restrict the permission supressAccessChecks in a security manager. Tried to fix the issue on our end by making all classes/constructors/methods involved in serialization public but was still getting exceptions from the databind library, specifically ClassUtils. Did a little investigation and found that the code handling SecurityExceptions seems to be incorrect.

/* 14-Jan-2009, tatu: It seems safe and potentially beneficial to
 *   always to make it accessible (latter because it will force
 *   skipping checks we have no use for...), so let's always call it.
 */
//if (!ao.isAccessible()) {
try {
    ao.setAccessible(true);
} catch (SecurityException se) {
    /* 17-Apr-2009, tatu: Related to [JACKSON-101]: this can fail on
     *    platforms like EJB and Google App Engine); so let's
     *    only fail if we really needed it...
     */
    if (!ao.isAccessible()) {
        Class<?> declClass = member.getDeclaringClass();
        throw new IllegalArgumentException("Can not access "+member+" (from class "+declClass.getName()+"; failed to set access: "+se.getMessage());
    }
}

The method isAccessible checks if access checks have been suppressed, i.e. not locked down by a security manager, rather then if the method/constructor/field is actually accessible per it's modifiers. If we are unable to suppress access checks can we not just proceed and have deserialization fail when the method is invoked? Unless I'm mistaken this makes Jackson unusable with a SecurityManager in place.

I'll put together a pull request for this.

[shorea]

#993

[cowtowncoder]

I am not sure. I do prefer explicit, upfront exceptions over later-on ambiguous and misleading exceptions, which removal of check could cause.
At the same time it would also be wrong to add more failure modes for non-failing cases.

So I guess I'd like to understand your problem better, and/or suggest alternative handling.

From original bug report it would seem to me that use of Jackson from a static initializer block of a class is bit dangerous in itself. Could that be changed to occur in a place where there is less damage from this check failing? Or perhaps handling of possible Exceptions?

[cowtowncoder]

One thing I can and will do, for 2.7, is give method in question access to configuration, which will make it possible to use a MapperFeature to at least allow alternate behavior of not failing. But it may also make sense to allow something like only forcing access for non-public methods/fields, since although there is (or was -- I need to verify this) a good reason for calling the method on public methods/fields too (performance-wise it made a big difference as of JDK 1.5 at least), it won't do much good if SecurityManager disallows such use.

One more possibility could be to figure out dynamically whether SecurityManager would allow setAccessible call, if that can be determined easily. If not, perhaps it could even try calling this on one of Jackson's own classes, catch exception if one is thrown.

[cowtowncoder]

In the meantime, for version 2.6 and earlier, there is MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS (default: true); disabling of which should prevent all calls to setAccess(). With brief performance testing it would seem like there wasn't much difference with or without this call, using JDK 1.7. So it may make sense to disable this feature for ObjectMapper; this should prevent the problem if methods/fields used are are public, as well as classes they are contained in.