From 7159ba3d73625d4fcfa497ed0f3d91c4309ccaa7 Mon Sep 17 00:00:00 2001 From: Jerry Duffy Date: Fri, 31 May 2024 11:45:45 -0400 Subject: [PATCH 1/2] add security excludes during normal class transformer creation --- .../agent/config/AgentConfigImpl.java | 32 +-------- .../config/ClassTransformerConfigImpl.java | 23 ++++-- .../src/main/resources/META-INF/excludes | 4 +- .../ClassTransformerConfigImplTest.java | 71 +++++++++++++------ 4 files changed, 70 insertions(+), 60 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java index 9120935538..bdb767c6f3 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java @@ -780,36 +780,8 @@ private BrowserMonitoringConfig initBrowserMonitoringConfig() { private ClassTransformerConfig initClassTransformerConfig(boolean liteMode) { boolean customTracingEnabled = getProperty(ENABLE_CUSTOM_TRACING, DEFAULT_ENABLE_CUSTOM_TRACING); Map props = nestedProps(CLASS_TRANSFORMER); - props = placeSecurityCollectorRelatedModification(props); - return ClassTransformerConfigImpl.createClassTransformerConfig(props, customTracingEnabled, liteMode); - } - - /** - * Security agent specific excludes needed to allow functioning with java.io.InputStream and OutputStream instrumentation. - */ - private Map placeSecurityCollectorRelatedModification(Map props) { - if (getProperty("security") != null) { - if (props == null) { - props = new HashMap<>(); - } else { - props = new HashMap<>(props); - } - Set securityExcludes = new HashSet<>(); - securityExcludes.add("java/util/zip/InflaterInputStream"); - securityExcludes.add("java/util/zip/ZipFile$ZipFileInputStream"); - securityExcludes.add("java/util/zip/ZipFile$ZipFileInflaterInputStream"); - securityExcludes.add("com/newrelic/api/agent/security/.*"); - securityExcludes.add("com/newrelic/agent/security/.*"); - - Object userProvidedExcludes = props.get(ClassTransformerConfigImpl.EXCLUDES); - if (userProvidedExcludes instanceof String) { - securityExcludes.add((String) userProvidedExcludes); - } else if (userProvidedExcludes instanceof Set) { - securityExcludes.addAll((Collection) userProvidedExcludes); - } - props.put(ClassTransformerConfigImpl.EXCLUDES, securityExcludes); - } - return props; + boolean addSecurityExcludes = getProperty("security") != null; + return ClassTransformerConfigImpl.createClassTransformerConfig(props, customTracingEnabled, liteMode, addSecurityExcludes); } private CircuitBreakerConfig initCircuitBreakerConfig() { diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/config/ClassTransformerConfigImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/config/ClassTransformerConfigImpl.java index db34fd20e4..6e356574cc 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/config/ClassTransformerConfigImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/config/ClassTransformerConfigImpl.java @@ -66,6 +66,10 @@ final class ClassTransformerConfigImpl extends BaseConfig implements ClassTransf private static final Set DEFAULT_DISABLED_WEAVE_PACKAGES = new HashSet<>(); private static final Set DEFAULT_CLASSLOADER_EXCLUDES = new HashSet<>(); + // Security agent specific excludes needed to allow functioning with java.io.InputStream and OutputStream instrumentation. + private static final Set SECURITY_AGENT_CLASS_EXCLUDES = new HashSet<>(Arrays.asList("java/util/zip/InflaterInputStream", "java/util/zip/ZipFile$ZipFileInputStream", + "java/util/zip/ZipFile$ZipFileInflaterInputStream", "com/newrelic/api/agent/security/.*", "com/newrelic/agent/security/.*")); + static { DEFAULT_DISABLED_WEAVE_PACKAGES.add("com.newrelic.instrumentation.servlet-user"); DEFAULT_DISABLED_WEAVE_PACKAGES.add("com.newrelic.instrumentation.spring-aop-2"); @@ -103,14 +107,14 @@ final class ClassTransformerConfigImpl extends BaseConfig implements ClassTransf private final boolean litemode; private final long autoAsyncLinkRateLimit; - public ClassTransformerConfigImpl(Map props, boolean customTracingEnabled, boolean litemode) { + public ClassTransformerConfigImpl(Map props, boolean customTracingEnabled, boolean litemode, boolean addSecurityExcludes) { super(props, SYSTEM_PROPERTY_ROOT); this.custom_tracing = customTracingEnabled; this.litemode = litemode; isEnabled = getProperty(ENABLED, DEFAULT_ENABLED); isDefaultInstrumentationEnabled = getDefaultInstrumentationEnabled(); isBuiltinExtensionEnabled = getBuiltinExensionEnabled(); - excludes = Collections.unmodifiableSet(new HashSet<>(getUniqueStrings(EXCLUDES))); + excludes = initializeClassExcludes(addSecurityExcludes); includes = Collections.unmodifiableSet(new HashSet<>(getUniqueStrings(INCLUDES))); classloaderExclusions = initializeClassloaderExcludes(); classloaderDelegationExcludes = initializeClassloaderDelegationExcludes(); @@ -131,7 +135,7 @@ public ClassTransformerConfigImpl(Map props, boolean customTraci } public ClassTransformerConfigImpl(Map props, boolean customTracingEnabled) { - this(props, customTracingEnabled, false); + this(props, customTracingEnabled, false, false); } private boolean getBuiltinExensionEnabled() { @@ -156,6 +160,15 @@ private boolean getDefaultInstrumentationEnabled() { } } + private Set initializeClassExcludes(boolean addSecurityExcludes) { + HashSet tempExcludes = new HashSet<>(getUniqueStrings(EXCLUDES)); + if (addSecurityExcludes) { + tempExcludes.addAll(SECURITY_AGENT_CLASS_EXCLUDES); + } + + return Collections.unmodifiableSet(new HashSet<>(tempExcludes)); + } + private Set initializeClassloaderExcludes() { Set classloadersToExclude = new HashSet<>(getUniqueStrings(CLASSLOADER_EXCLUDES)); @@ -326,11 +339,11 @@ public Collection getJdbcStatements() { return Arrays.asList(jdbcStatementsProp.split(",[\\s]*")); } - static ClassTransformerConfig createClassTransformerConfig(Map settings, boolean custom_tracing, boolean litemode) { + static ClassTransformerConfig createClassTransformerConfig(Map settings, boolean custom_tracing, boolean litemode, boolean addSecurityExcludes) { if (settings == null) { settings = Collections.emptyMap(); } - return new ClassTransformerConfigImpl(settings, custom_tracing, litemode); + return new ClassTransformerConfigImpl(settings, custom_tracing, litemode, addSecurityExcludes); } @Override diff --git a/newrelic-agent/src/main/resources/META-INF/excludes b/newrelic-agent/src/main/resources/META-INF/excludes index c416afe95d..617216eb1a 100644 --- a/newrelic-agent/src/main/resources/META-INF/excludes +++ b/newrelic-agent/src/main/resources/META-INF/excludes @@ -3,10 +3,10 @@ ^java/io/(BufferedOutputStream|ByteArrayOutputStream|DataOutputStream|FilterOutputStream|ObjectOutputStream|PipedOutputStream) ^java/lang/(NullOutputStream|ProcessPipeOutputStream) ^java/net/SocketOutputStream -//To resolve class circularity error +# To resolve class circularity error ^javax/crypto/BadPaddingException ^javax/crypto/SecretKey -//Remove ThreadLocal Weak Random Class Instrumentation +# Remove ThreadLocal Weak Random Class Instrumentation ^java/util/concurrent/ThreadLocalRandom ^(org/objectweb/asm/|javax/xml/|org/apache/juli/)(.*) # Don't instrument agent threads diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java index 6d7450f6e5..7055393554 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java @@ -45,11 +45,11 @@ public class ClassTransformerConfigImplTest { public void isEnabled() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, - true, false); + true, false, false); assertTrue(config.isEnabled()); classTransformerMap.put("enabled", false); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertFalse(config.isEnabled()); } @@ -59,20 +59,20 @@ public void isInstrumentationDefaultEnabled() throws Exception { Map instDefault = new HashMap<>(); Map builtinExt = new HashMap<>(); - ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertTrue(config.isDefaultInstrumentationEnabled()); Assert.assertTrue(config.isBuiltinExtensionEnabled()); instDefault.put("enabled", false); classTransformerMap.put(ClassTransformerConfigImpl.DEFAULT_INSTRUMENTATION, instDefault); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertFalse(config.isDefaultInstrumentationEnabled()); Assert.assertFalse(config.isBuiltinExtensionEnabled()); builtinExt.put("enabled", true); classTransformerMap.put(ClassTransformerConfigImpl.BUILTIN_EXTENSIONS, builtinExt); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertFalse(config.isDefaultInstrumentationEnabled()); Assert.assertTrue(config.isBuiltinExtensionEnabled()); @@ -82,9 +82,9 @@ public void isInstrumentationDefaultEnabled() throws Exception { public void isCustomTracingEnabled() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, - true, false); + true, false, false); assertTrue(config.isCustomTracingEnabled()); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, false, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, false, false, false); Assert.assertFalse(config.isCustomTracingEnabled()); } @@ -92,17 +92,17 @@ public void isCustomTracingEnabled() throws Exception { public void shutdownDelayInNanos() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, - true, false); + true, false, false); Assert.assertEquals(-1L, config.getShutdownDelayInNanos()); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); classTransformerMap.put("shutdown_delay", 60); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertEquals(60000000000L, config.getShutdownDelayInNanos()); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); classTransformerMap.put("shutdown_delay", 44.10); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Assert.assertEquals(44000000000L, config.getShutdownDelayInNanos()); } @@ -110,13 +110,13 @@ public void shutdownDelayInNanos() throws Exception { public void includes() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, - true, false); + true, false, false); Assert.assertEquals(0, config.getIncludes().size()); classTransformerMap.put( ClassTransformerConfigImpl.INCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper, test/ExcludeTest$ExcludeTestInner, org/jruby/rack/RackEnvironment"); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Set includes = config.getIncludes(); Assert.assertEquals(3, includes.size()); assertTrue(includes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); @@ -125,7 +125,7 @@ public void includes() throws Exception { classTransformerMap.put(ClassTransformerConfigImpl.INCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper"); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); includes = config.getIncludes(); Assert.assertEquals(1, includes.size()); assertTrue(includes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); @@ -154,7 +154,7 @@ public void classloaderExcludes() throws Exception { AgentConfig config = AgentConfigImpl.createAgentConfig(configMap); ClassTransformerConfig classTransformerConfig = config.getClassTransformerConfig(); - classTransformerConfig = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + classTransformerConfig = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Set exclusions = classTransformerConfig.getClassloaderExclusions(); Assert.assertEquals(8, exclusions.size()); // 6 included by default ClassTransformerConfigImpl#initializeClassloaderExcludes @@ -205,13 +205,13 @@ public void defaultClassloaderExcludes() throws ConfigurationException, ForceDis public void excludes() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, - true, false); + true, false, false); Assert.assertEquals(0, config.getExcludes().size()); classTransformerMap.put( ClassTransformerConfigImpl.EXCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper, test/ExcludeTest$ExcludeTestInner, org/jruby/rack/RackEnvironment"); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Set excludes = config.getExcludes(); Assert.assertEquals(3, excludes.size()); assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); @@ -220,12 +220,37 @@ public void excludes() throws Exception { classTransformerMap.put(ClassTransformerConfigImpl.EXCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper"); - config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); excludes = config.getExcludes(); Assert.assertEquals(1, excludes.size()); assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); } + @Test + public void excludes_withSecurityAgentClasses() throws Exception { + Map classTransformerMap = new HashMap<>(); + ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, + true, false, true); + Assert.assertEquals(5, config.getExcludes().size()); + + classTransformerMap.put( + ClassTransformerConfigImpl.EXCLUDES, + "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper, test/ExcludeTest$ExcludeTestInner, org/jruby/rack/RackEnvironment"); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, true); + Set excludes = config.getExcludes(); + Assert.assertEquals(8, excludes.size()); + assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); + assertTrue(excludes.contains("test/ExcludeTest$ExcludeTestInner")); + assertTrue(excludes.contains("org/jruby/rack/RackEnvironment")); + + classTransformerMap.put(ClassTransformerConfigImpl.EXCLUDES, + "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper"); + config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, true); + excludes = config.getExcludes(); + Assert.assertEquals(6, excludes.size()); + assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); + } + @Test public void noTraceAnnotationClassNames() { ClassTransformerConfig config = new ClassTransformerConfigImpl(Collections.EMPTY_MAP, true); @@ -381,7 +406,7 @@ public void weavePackageConfigInstrumentationDisable() { instDefault.put("enabled", false); props.put(ClassTransformerConfigImpl.DEFAULT_INSTRUMENTATION, instDefault); - config = ClassTransformerConfigImpl.createClassTransformerConfig(props, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(props, true, false, false); assertFalse(config.isWeavePackageEnabled(weaveConfig)); @@ -507,7 +532,7 @@ public void testPointcutDisabled() { public void testPointcutEnabled() { Map classTransformerMap = new HashMap<>(); - ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false); + ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, false); Map configSettings = new HashMap<>(); configSettings.put(AgentConfigImpl.CLASS_TRANSFORMER, classTransformerMap); @@ -565,12 +590,12 @@ public void testPoincutExplicitEnable() { public void weavePackageConfigInstrumentationDisableSystemPropertyOverride() { final String moduleName = "com.newrelic.instrumentation.mymodule-1.0"; Map configMap = new HashMap<>(); - ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(configMap, true, false); + ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(configMap, true, false, false); Assert.assertTrue(config.isDefaultInstrumentationEnabled()); configMap.put("instrumentation_default:enabled", false); configMap = buildConfigMap(configMap); - config = ClassTransformerConfigImpl.createClassTransformerConfig(configMap, true, false); + config = ClassTransformerConfigImpl.createClassTransformerConfig(configMap, true, false, false); Assert.assertFalse(config.isDefaultInstrumentationEnabled()); WeavePackageConfig weaveConfig = WeavePackageConfig.builder().name(moduleName).build(); From 4739ccf349a95fa29075ed3045c17884c5d0ff27 Mon Sep 17 00:00:00 2001 From: Jerry Duffy Date: Fri, 31 May 2024 13:21:54 -0400 Subject: [PATCH 2/2] Update unit test --- .../ClassTransformerConfigImplTest.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java index 7055393554..dd55ec08fe 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/config/ClassTransformerConfigImplTest.java @@ -231,17 +231,28 @@ public void excludes_withSecurityAgentClasses() throws Exception { Map classTransformerMap = new HashMap<>(); ClassTransformerConfig config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, true); - Assert.assertEquals(5, config.getExcludes().size()); + Set excludes = config.getExcludes(); + Assert.assertEquals(5, excludes.size()); + assertTrue(excludes.contains("java/util/zip/InflaterInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInflaterInputStream")); + assertTrue(excludes.contains("com/newrelic/api/agent/security/.*")); + assertTrue(excludes.contains("com/newrelic/agent/security/.*")); classTransformerMap.put( ClassTransformerConfigImpl.EXCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper, test/ExcludeTest$ExcludeTestInner, org/jruby/rack/RackEnvironment"); config = ClassTransformerConfigImpl.createClassTransformerConfig(classTransformerMap, true, false, true); - Set excludes = config.getExcludes(); + excludes = config.getExcludes(); Assert.assertEquals(8, excludes.size()); assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); assertTrue(excludes.contains("test/ExcludeTest$ExcludeTestInner")); assertTrue(excludes.contains("org/jruby/rack/RackEnvironment")); + assertTrue(excludes.contains("java/util/zip/InflaterInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInflaterInputStream")); + assertTrue(excludes.contains("com/newrelic/api/agent/security/.*")); + assertTrue(excludes.contains("com/newrelic/agent/security/.*")); classTransformerMap.put(ClassTransformerConfigImpl.EXCLUDES, "org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper"); @@ -249,6 +260,11 @@ public void excludes_withSecurityAgentClasses() throws Exception { excludes = config.getExcludes(); Assert.assertEquals(6, excludes.size()); assertTrue(excludes.contains("org/apache/tomcat/dbcp/dbcp/PoolableDataSource$PoolGuardConnectionWrapper")); + assertTrue(excludes.contains("java/util/zip/InflaterInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInputStream")); + assertTrue(excludes.contains("java/util/zip/ZipFile$ZipFileInflaterInputStream")); + assertTrue(excludes.contains("com/newrelic/api/agent/security/.*")); + assertTrue(excludes.contains("com/newrelic/agent/security/.*")); } @Test