From 18d7f4afdcf98ffbda51b7d86f375f526d8b8038 Mon Sep 17 00:00:00 2001 From: Simon Svensson Date: Mon, 27 Feb 2017 12:02:13 +0100 Subject: [PATCH] Extended Procedure sandboxing to involve user defined functions. --- .../factory/GraphDatabaseSettings.java | 6 +- ....java => ComponentInjectionException.java} | 4 +- .../proc/CallableUserAggregationFunction.java | 2 +- .../api/proc/LoadFailAggregatedFunction.java | 38 ++++++ .../kernel/api/proc/LoadFailFunction.java | 39 ++++++ .../kernel/impl/proc/FieldInjections.java | 6 +- .../proc/ReflectiveProcedureCompiler.java | 120 ++++++++++++------ ...ReflectiveUserAggregationFunctionTest.java | 2 +- .../impl/proc/ReflectiveUserFunctionTest.java | 2 +- .../java/org/neo4j/procedure/ProcedureIT.java | 8 ++ 10 files changed, 174 insertions(+), 53 deletions(-) rename community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/{ProcedureInjectionException.java => ComponentInjectionException.java} (88%) create mode 100644 community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailAggregatedFunction.java create mode 100644 community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailFunction.java diff --git a/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java b/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java index 0f24ef7c00848..403006b5ec0b3 100644 --- a/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java +++ b/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java @@ -524,9 +524,9 @@ public enum LabelIndex public static final Setting auth_store = pathSetting( "unsupported.dbms.security.auth_store.location", NO_DEFAULT ); - @Description( "A list of procedures (comma separated) that are allowed full access to the database. " + - "The list may contain both fully-qualified procedure names, and partial names with the wildcard '*'. " + - "Note that this enables these procedures to bypass security. Use with caution." ) + @Description( "A list of procedures and user defined functions (comma separated) that are allowed full access to " + + "the database. The list may contain both fully-qualified procedure names, and partial names with the " + + "wildcard '*'. Note that this enables these procedures to bypass security. Use with caution." ) public static final Setting procedure_unrestricted = setting( "dbms.security.procedures.unrestricted", Settings.STRING, "" ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ProcedureInjectionException.java b/community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ComponentInjectionException.java similarity index 88% rename from community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ProcedureInjectionException.java rename to community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ComponentInjectionException.java index 9c9d74b4a9ca3..9d873a1f874c7 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ProcedureInjectionException.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/exceptions/ComponentInjectionException.java @@ -19,9 +19,9 @@ */ package org.neo4j.kernel.api.exceptions; -public class ProcedureInjectionException extends ProcedureException +public class ComponentInjectionException extends ProcedureException { - public ProcedureInjectionException( Status statusCode, String message, + public ComponentInjectionException( Status statusCode, String message, Object... parameters ) { super( statusCode, message, parameters ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/proc/CallableUserAggregationFunction.java b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/CallableUserAggregationFunction.java index 0247d3168102b..294f9f4f28277 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/proc/CallableUserAggregationFunction.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/CallableUserAggregationFunction.java @@ -49,6 +49,6 @@ public UserFunctionSignature signature() } @Override - public abstract Aggregator create( Context ctx); + public abstract Aggregator create( Context ctx) throws ProcedureException; } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailAggregatedFunction.java b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailAggregatedFunction.java new file mode 100644 index 0000000000000..082dd56e4df6e --- /dev/null +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailAggregatedFunction.java @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.api.proc; + +import org.neo4j.kernel.api.exceptions.ProcedureException; +import org.neo4j.kernel.api.exceptions.Status; + +public class LoadFailAggregatedFunction extends CallableUserAggregationFunction.BasicUserAggregationFunction +{ + public LoadFailAggregatedFunction( UserFunctionSignature signature ) + { + super( signature ); + } + + @Override + public Aggregator create( Context ctx ) throws ProcedureException + { + throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, + signature().description().orElse( "Failed to load " + signature().name().toString() ) ); + } +} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailFunction.java b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailFunction.java new file mode 100644 index 0000000000000..83384e432389f --- /dev/null +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/proc/LoadFailFunction.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.api.proc; + +import org.neo4j.collection.RawIterator; +import org.neo4j.kernel.api.exceptions.ProcedureException; +import org.neo4j.kernel.api.exceptions.Status; + +public class LoadFailFunction extends CallableUserFunction.BasicUserFunction +{ + public LoadFailFunction( UserFunctionSignature signature ) + { + super( signature ); + } + + @Override + public RawIterator apply( Context ctx, Object[] input ) throws ProcedureException + { + throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, + signature().description().orElse( "Failed to load " + signature().name().toString() ) ); + } +} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java index 9f6f899dc0c11..e376b14536916 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java @@ -26,7 +26,7 @@ import java.util.LinkedList; import java.util.List; -import org.neo4j.kernel.api.exceptions.ProcedureInjectionException; +import org.neo4j.kernel.api.exceptions.ComponentInjectionException; import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.procedure.Context; @@ -122,10 +122,10 @@ private FieldSetter createInjector( Class cls, Field field ) throws Procedure ComponentRegistry.Provider provider = components.providerFor( field.getType() ); if( provider == null ) { - throw new ProcedureInjectionException( Status.Procedure.ProcedureRegistrationFailed, + throw new ComponentInjectionException( Status.Procedure.ProcedureRegistrationFailed, "Unable to set up injection for procedure `%s`, the field `%s` " + "has type `%s` which is not a known injectable component.", - cls.getSimpleName(), field.getName(), field.getType()); + cls.getSimpleName(), field.getName(), field.getType()); } MethodHandle setter = MethodHandles.lookup().unreflectSetter( field ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureCompiler.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureCompiler.java index 495a89c873bb8..b52eddf07c563 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureCompiler.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureCompiler.java @@ -34,7 +34,7 @@ import java.util.stream.Stream; import org.neo4j.collection.RawIterator; -import org.neo4j.kernel.api.exceptions.ProcedureInjectionException; +import org.neo4j.kernel.api.exceptions.ComponentInjectionException; import org.neo4j.kernel.api.exceptions.KernelException; import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.exceptions.Status; @@ -43,6 +43,8 @@ import org.neo4j.kernel.api.proc.CallableUserFunction; import org.neo4j.kernel.api.proc.Context; import org.neo4j.kernel.api.proc.FieldSignature; +import org.neo4j.kernel.api.proc.LoadFailAggregatedFunction; +import org.neo4j.kernel.api.proc.LoadFailFunction; import org.neo4j.kernel.api.proc.LoadFailProcedure; import org.neo4j.kernel.api.proc.ProcedureSignature; import org.neo4j.kernel.api.proc.QualifiedName; @@ -231,32 +233,32 @@ private CallableProcedure compileProcedure( Class procDefinition, MethodHandl Optional deprecated = deprecated( method, procedure::deprecatedBy, "Use of @Procedure(deprecatedBy) without @Deprecated in " + procName ); - List setters; - setters = allFieldInjections.setters( procDefinition ); - ProcedureSignature signature; - + List setters = allFieldInjections.setters( procDefinition ); if ( !fullAccess && !config.fullAccessFor( procName.toString() ) ) { try { setters = safeFieldInjections.setters( procDefinition ); } - catch ( ProcedureInjectionException e ) + catch ( ComponentInjectionException e ) { description = Optional.of( procName.toString() + " is not available due to not having unrestricted access rights, check configuration." ); - log.warn( description.get()); - signature = new ProcedureSignature( procName, inputSignature, outputMapper.signature(), Mode.DEFAULT, - Optional.empty(), new String[0], description, warning ); + log.warn( description.get() ); + ProcedureSignature signature = + new ProcedureSignature( procName, inputSignature, outputMapper.signature(), Mode.DEFAULT, + Optional.empty(), new String[0], description, warning ); return new LoadFailProcedure( signature ); } } - signature = new ProcedureSignature( procName, inputSignature, outputMapper.signature(), mode, deprecated, - config.rolesFor( procName.toString() ), description, warning ); + + ProcedureSignature signature = + new ProcedureSignature( procName, inputSignature, outputMapper.signature(), mode, deprecated, + config.rolesFor( procName.toString() ), description, warning ); return new ReflectiveProcedure( signature, constructor, procedureMethod, outputMapper, setters ); } - private ReflectiveUserFunction compileFunction( Class procDefinition, MethodHandle constructor, Method method ) + private CallableUserFunction compileFunction( Class procDefinition, MethodHandle constructor, Method method ) throws ProcedureException, IllegalAccessException { String valueName = method.getAnnotation( UserFunction.class ).value(); @@ -274,14 +276,30 @@ private ReflectiveUserFunction compileFunction( Class procDefinition, MethodH Class returnType = method.getReturnType(); TypeMappers.NeoValueConverter valueConverter = typeMappers.converterFor( returnType ); MethodHandle procedureMethod = lookup.unreflect( method ); - List setters = safeFieldInjections.setters( procDefinition ); - Optional description = description( method ); UserFunction function = method.getAnnotation( UserFunction.class ); - Optional deprecated = deprecated( method, function::deprecatedBy, "Use of @UserFunction(deprecatedBy) without @Deprecated in " + procName ); + List setters = allFieldInjections.setters( procDefinition ); + if ( !config.fullAccessFor( procName.toString() ) ) + { + try + { + setters = safeFieldInjections.setters( procDefinition ); + } + catch ( ComponentInjectionException e ) + { + description = Optional.of( procName.toString() + + " is not available due to not having unrestricted access rights, check configuration." ); + log.warn( description.get() ); + UserFunctionSignature signature = + new UserFunctionSignature( procName, inputSignature, valueConverter.type(), deprecated, + config.rolesFor( procName.toString() ), description ); + return new LoadFailFunction( signature ); + } + } + UserFunctionSignature signature = new UserFunctionSignature( procName, inputSignature, valueConverter.type(), deprecated, config.rolesFor( procName.toString() ), description ); @@ -289,14 +307,14 @@ private ReflectiveUserFunction compileFunction( Class procDefinition, MethodH return new ReflectiveUserFunction( signature, constructor, procedureMethod, valueConverter, setters ); } - private ReflectiveUserAggregationFunction compileAggregationFunction( Class definition, MethodHandle constructor, Method method ) - throws ProcedureException, IllegalAccessException + private CallableUserAggregationFunction compileAggregationFunction( Class definition, MethodHandle constructor, + Method method ) throws ProcedureException, IllegalAccessException { String valueName = method.getAnnotation( UserAggregationFunction.class ).value(); String definedName = method.getAnnotation( UserAggregationFunction.class ).name(); QualifiedName funcName = extractName( definition, method, valueName, definedName ); - if (funcName.namespace() == null || funcName.namespace().length == 0) + if ( funcName.namespace() == null || funcName.namespace().length == 0 ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "It is not allowed to define functions in the root namespace please use a namespace, e.g. `@UserFunction(\"org.example.com.%s\")", @@ -309,18 +327,18 @@ private ReflectiveUserAggregationFunction compileAggregationFunction( Class d Class aggregator = method.getReturnType(); for ( Method m : aggregator.getDeclaredMethods() ) { - if (m.isAnnotationPresent( UserAggregationUpdate.class )) - { - if ( update != null ) - { - throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, - "Class '%s' contains multiple methods annotated with '@%s'.", aggregator.getSimpleName(), - UserAggregationUpdate.class.getSimpleName() ); - } - update = m; - - } - if (m.isAnnotationPresent( UserAggregationResult.class )) + if ( m.isAnnotationPresent( UserAggregationUpdate.class ) ) + { + if ( update != null ) + { + throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, + "Class '%s' contains multiple methods annotated with '@%s'.", aggregator.getSimpleName(), + UserAggregationUpdate.class.getSimpleName() ); + } + update = m; + + } + if ( m.isAnnotationPresent( UserAggregationResult.class ) ) { if ( result != null ) { @@ -331,39 +349,37 @@ private ReflectiveUserAggregationFunction compileAggregationFunction( Class d result = m; } } - if ( result == null || update == null) + if ( result == null || update == null ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "Class '%s' must contain methods annotated with both '@%s' as well as '@%s'.", - aggregator.getSimpleName(), - UserAggregationResult.class.getSimpleName(), + aggregator.getSimpleName(), UserAggregationResult.class.getSimpleName(), UserAggregationUpdate.class.getSimpleName() ); } - if (update.getReturnType() != void.class) + if ( update.getReturnType() != void.class ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "Update method '%s' in %s has type '%s' but must have return type 'void'.", update.getName(), aggregator.getSimpleName(), update.getReturnType().getSimpleName() ); } - if ( !Modifier.isPublic(method.getModifiers())) + if ( !Modifier.isPublic( method.getModifiers() ) ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, - "Aggregation method '%s' in %s must be public.", method.getName(), - definition.getSimpleName() ); + "Aggregation method '%s' in %s must be public.", method.getName(), definition.getSimpleName() ); } - if ( !Modifier.isPublic(aggregator.getModifiers())) + if ( !Modifier.isPublic( aggregator.getModifiers() ) ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "Aggregation class '%s' must be public.", aggregator.getSimpleName() ); } - if ( !Modifier.isPublic(update.getModifiers())) + if ( !Modifier.isPublic( update.getModifiers() ) ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "Aggregation update method '%s' in %s must be public.", method.getName(), aggregator.getSimpleName() ); } - if ( !Modifier.isPublic(result.getModifiers())) + if ( !Modifier.isPublic( result.getModifiers() ) ) { throw new ProcedureException( Status.Procedure.ProcedureRegistrationFailed, "Aggregation result method '%s' in %s must be public.", method.getName(), @@ -376,7 +392,6 @@ private ReflectiveUserAggregationFunction compileAggregationFunction( Class d MethodHandle creator = lookup.unreflect( method ); MethodHandle updateMethod = lookup.unreflect( update ); MethodHandle resultMethod = lookup.unreflect( result ); - List setters = safeFieldInjections.setters( definition ); Optional description = description( method ); UserAggregationFunction function = method.getAnnotation( UserAggregationFunction.class ); @@ -384,11 +399,32 @@ private ReflectiveUserAggregationFunction compileAggregationFunction( Class d Optional deprecated = deprecated( method, function::deprecatedBy, "Use of @UserAggregationFunction(deprecatedBy) without @Deprecated in " + funcName ); + List setters = allFieldInjections.setters( definition ); + if ( !config.fullAccessFor( funcName.toString() ) ) + { + try + { + setters = safeFieldInjections.setters( definition ); + } + catch ( ComponentInjectionException e ) + { + description = Optional.of( funcName.toString() + + " is not available due to not having unrestricted access rights, check configuration." ); + log.warn( description.get() ); + UserFunctionSignature signature = + new UserFunctionSignature( funcName, inputSignature, valueConverter.type(), deprecated, + config.rolesFor( funcName.toString() ), description ); + + return new LoadFailAggregatedFunction( signature ); + } + } + UserFunctionSignature signature = new UserFunctionSignature( funcName, inputSignature, valueConverter.type(), deprecated, config.rolesFor( funcName.toString() ), description ); - return new ReflectiveUserAggregationFunction( signature, constructor, creator, updateMethod, resultMethod, valueConverter, setters ); + return new ReflectiveUserAggregationFunction( signature, constructor, creator, updateMethod, resultMethod, + valueConverter, setters ); } private Optional deprecated( Method method, Supplier supplier, String warning ) diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserAggregationFunctionTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserAggregationFunctionTest.java index d2b9e7735dde7..152768b6cb8ca 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserAggregationFunctionTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserAggregationFunctionTest.java @@ -69,7 +69,7 @@ public class ReflectiveUserAggregationFunctionTest public void setUp() throws Exception { components = new ComponentRegistry(); - procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components, new ComponentRegistry(), + procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components, components, NullLog.getInstance(), ProcedureConfig.DEFAULT ); } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserFunctionTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserFunctionTest.java index 7741af4c959fc..8b97228a48e0f 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserFunctionTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveUserFunctionTest.java @@ -65,7 +65,7 @@ public class ReflectiveUserFunctionTest public void setUp() throws Exception { components = new ComponentRegistry(); - procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components, new ComponentRegistry(), + procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components, components, NullLog.getInstance(), ProcedureConfig.DEFAULT ); } diff --git a/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java b/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java index dde4bbb89fee8..60460bef71dc0 100644 --- a/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java +++ b/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java @@ -52,6 +52,7 @@ import org.neo4j.graphdb.Result; import org.neo4j.graphdb.Transaction; import org.neo4j.graphdb.TransactionFailureException; +import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.graphdb.security.AuthorizationViolationException; import org.neo4j.helpers.collection.Iterators; import org.neo4j.io.fs.FileUtils; @@ -1147,6 +1148,7 @@ public void setUp() throws IOException db = new TestGraphDatabaseFactory() .newImpermanentDatabaseBuilder() .setConfig( plugin_dir, plugins.getRoot().getAbsolutePath() ) + .setConfig( GraphDatabaseSettings.procedure_unrestricted, "*" ) .newGraphDatabase(); } @@ -1288,6 +1290,9 @@ public static class ClassWithProcedures @Context public ProcedureTransaction procedureTransaction; + @Context + public GraphDatabaseAPI graphDatabaseAPI; + @Procedure public Stream guardMe() { @@ -1643,6 +1648,9 @@ public void schemaTryingToWrite() public static class ClassWithFunctions { + @Context + public GraphDatabaseAPI graphDatabaseAPI; + @UserFunction() public String getNodeName( @Name( value = "node", defaultValue = "null") Node node ) {