diff --git a/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java b/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java index b9351b5bfad8..4681b3df0739 100644 --- a/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java +++ b/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java @@ -23,7 +23,11 @@ import java.io.File; import java.io.OutputStream; import java.time.Clock; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -140,9 +144,12 @@ static class QueryLogger implements QueryExecutionMonitor private final boolean logQueryParameters; private static final Pattern PASSWORD_PATTERN = Pattern.compile( + // call signature "(?:(?i)call)\\s+dbms(?:\\.security)?\\.change(?:User)?Password\\(" + - "(?:\\s*(?:'(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*,)?" + - "\\s*('(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*\\)" ); + // optional username parameter, in single, double quotes, or parametrized + "(?:\\s*(?:'(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*,)?" + + // password parameter, in single, double quotes, or parametrized + "\\s*('(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*\\)" ); QueryLogger( Clock clock, Log log, long thresholdMillis, boolean logQueryParameters ) { @@ -180,20 +187,19 @@ private String logEntry( long time, ExecutingQuery query ) String queryText = query.queryText(); String metaData = mapAsString( query.metaData() ); - String password = ""; - + Set passwordParams = new HashSet<>(); Matcher matcher = PASSWORD_PATTERN.matcher( queryText ); while ( matcher.find() ) { - password = matcher.group( 1 ).trim(); + String password = matcher.group( 1 ).trim(); if ( password.charAt( 0 ) == '$' ) { - password = password.substring( 1 ); + passwordParams.add( password.substring( 1 ) ); } else if ( password.charAt( 0 ) == '{' ) { - password = password.substring( 1, password.length() - 1 ); + passwordParams.add( password.substring( 1, password.length() - 1 ) ); } else { @@ -204,7 +210,7 @@ else if ( password.charAt( 0 ) == '{' ) if ( logQueryParameters ) { - String params = mapAsString( query.queryParameters(), password ); + String params = mapAsString( query.queryParameters(), passwordParams ); return format( "%d ms: %s - %s - %s - %s", time, sourceString, queryText, params, metaData ); } else @@ -215,10 +221,10 @@ else if ( password.charAt( 0 ) == '{' ) private static String mapAsString( Map params ) { - return mapAsString( params, "" ); + return mapAsString( params, Collections.emptySet() ); } - private static String mapAsString( Map params, String obfuscate ) + private static String mapAsString( Map params, Collection obfuscate ) { if ( params == null ) { @@ -234,7 +240,7 @@ private static String mapAsString( Map params, String obfuscate .append( entry.getKey() ) .append( ": " ); - if ( entry.getKey().equals( obfuscate ) ) + if ( obfuscate.contains( entry.getKey() ) ) { builder.append( "******" ); } diff --git a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java index 3387fef0343b..3cb0a9c5d98f 100644 --- a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java +++ b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java @@ -357,6 +357,7 @@ public void shouldNotLogPassword() throws Exception assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "CALL dbms.security.changePassword(******)") ) ; + assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); } @@ -389,6 +390,7 @@ public void shouldNotLogChangeUserPassword() throws Exception assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "CALL dbms.security.changeUserPassword('neo4j', ******)") ) ; + assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); } @@ -421,6 +423,7 @@ public void shouldNotLogPasswordEvenIfPasswordIsSilly() throws Exception assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "CALL dbms.security.changePassword(******)") ) ; + assertThat( logLines.get( 0 ),not( containsString( ".changePassword(\\'si\"lly\\" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); } @@ -457,6 +460,49 @@ public void shouldNotLogPasswordEvenIfYouDoTwoThingsAtTheSameTime() throws Excep assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "CALL dbms.security.changeUserPassword('neo4j',******) CALL dbms.security.changeUserPassword('smith',******)") ) ; + assertThat( logLines.get( 0 ),not( containsString( "other$silly" ) ) ); + assertThat( logLines.get( 0 ),not( containsString( ".changePassword(silly)" ) ) ); + assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); + } + + @Test + public void shouldNotLogPasswordEvenIfYouDoTwoThingsAtTheSameTimeWithSeveralParms() throws Exception + { + GraphDatabaseFacade database = (GraphDatabaseFacade) databaseBuilder + .setConfig( GraphDatabaseSettings.log_queries, Settings.TRUE ) + .setConfig( GraphDatabaseSettings.logs_directory, logsDirectory.getPath() ) + .setConfig( GraphDatabaseSettings.auth_enabled, Settings.TRUE ) + .newGraphDatabase(); + + EnterpriseAuthManager authManager = database.getDependencyResolver().resolveDependency( EnterpriseAuthManager.class ); + EnterpriseAuthAndUserManager userManager= database.getDependencyResolver().resolveDependency( + EnterpriseAuthAndUserManager.class ); + userManager.getUserManager().newUser( "smith", "himitsu", false ); + EnterpriseSecurityContext neo = authManager.login( AuthToken.newBasicAuthToken( "neo4j", "neo4j" ) ); + + String query = "CALL dbms.security.changeUserPassword('neo4j',$first) "+ + "CALL dbms.security.changeUserPassword('smith',$second) RETURN 1"; + try ( InternalTransaction tx = database + .beginTransaction( KernelTransaction.Type.explicit, neo ) ) + { + Map params = new HashMap( ); + params.put("first",".changePassword(silly)"); + params.put("second",".other$silly"); + Result res = database.execute( tx, query, params ); + res.close(); + tx.success(); + } + finally + { + database.shutdown(); + } + + List logLines = readAllLines( logFilename ); + assertEquals( 1, logLines.size() ); + assertThat( logLines.get( 0 ), + containsString( "{first: ******, second: ******}")); + assertThat( logLines.get( 0 ),not( containsString( "other$silly" ) ) ); + assertThat( logLines.get( 0 ),not( containsString( ".changePassword(silly)" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); } @@ -489,6 +535,7 @@ public void shouldNotLogPasswordInParams() throws Exception assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "{password: ******}")); + assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); } @@ -521,6 +568,7 @@ public void shouldNotLogPasswordInDeprecatedParams() throws Exception assertEquals( 1, logLines.size() ); assertThat( logLines.get( 0 ), containsString( "{password: ******}")); + assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); }