leonfin commented on PR #7941:
URL: https://github.com/apache/geode/pull/7941#issuecomment-3615147191
MINOR ISSUE #1: Missing Test Coverage
Location: Missing tests for SecureClassLoaderObjectInputStream
What's Missing:
1. Test for null filter validation (IllegalArgumentException expected)
2. Test for resolveClass() fallback to context ClassLoader
3. Test for resolveProxyClass() with non-public interfaces
4. Integration test with actual serialization/deserialization flow
Impact: Medium - Core functionality of SecureClassLoaderObjectInputStream
is not unit tested
MINOR ISSUE #2: Missing Integration Test
Location: No end-to-end test for GemfireHttpSession.getAttribute() with
malicious payload
What's Missing: Test that simulates:
1. Storing a session attribute with different ClassLoader
2. Attempting to deserialize a blocked class
3. Verifying SecurityException is caught and null is returned
4. Verifying security log contains the blocked attempt
Impact: Medium - The actual integration point is not tested
🔍 Security Review
Threat Model Coverage
| Attack Vector | Protected | Implementation
|
|----------------------------------|-----------|---------------------------------------------|
| Commons Collections gadgets | ✅ | BLOCKED_CLASSES +
BLOCKED_PATTERNS |
| TemplatesImpl bytecode injection | ✅ | Explicitly blocked
|
| Spring Framework exploits | ✅ |
org.springframework.beans.factory.* blocked |
| JDK RMI exploits | ✅ | java.rmi.*, sun.rmi.*
blocked |
| Groovy MethodClosure | ✅ |
org.codehaus.groovy.runtime.* blocked |
| C3P0 JNDI injection | ✅ | com.mchange.v2.c3p0.*
blocked |
| Stack overflow (DoS) | ✅ | MAX_DEPTH = 50
|
| Memory exhaustion (DoS) | ✅ | MAX_REFERENCES = 10K,
MAX_BYTES = 10MB |
Defense-in-Depth Layers
1. ✅ Layer 1: Blacklist known dangerous classes (BLOCKED_CLASSES)
2. ✅ Layer 2: Blacklist dangerous package patterns (BLOCKED_PATTERNS)
3. ✅ Layer 3: Whitelist only known-safe classes (ALLOWED_CLASSES)
4. ✅ Layer 4: Whitelist safe package patterns (ALLOWED_PATTERNS)
5. ✅ Layer 5: Resource limits (depth, references, arrays, bytes)
6. ✅ Layer 6: Security logging for audit trail
7. ✅ Layer 7: Fail-safe exception handling (return null vs throw)
Nice-to-Have
1. 💡 Consider adding performance benchmark test to validate <1ms overhead
claim
2. 💡 Consider documenting upgrade path for users with custom session
attributes
3. 💡 Consider adding configuration property to extend whitelist at runtime
(for custom DTOs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
