-
Notifications
You must be signed in to change notification settings - Fork 355
IAST security controls: handling custom validation and sanitization methods #7997
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 32 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
0f9c22c
formatter and config POC
jandro996 a44168e
more changes
jandro996 3b19a16
Not working maybe need to rollback
jandro996 8083908
use IastSystem
jandro996 57134bd
First working version
jandro996 8e591d4
Add more stacktrace cases
jandro996 2c725b9
first review
jandro996 831fe64
Fix test and forbidden apis
jandro996 8602477
fix spotless
jandro996 325086f
fix spotless
jandro996 838d924
fix test
jandro996 f0f8496
fix test
jandro996 9844124
Add test and fix coverage
jandro996 744cdbb
remove security control enabled env var
jandro996 74a93b8
fix classVisitor
jandro996 82bb7df
change array for list
jandro996 7d54511
fix test
jandro996 8516871
Add unit test for IastSecurityControlTransformer
jandro996 2aa1c72
improve test
jandro996 21d7ed1
fix onUnexpectedException message
jandro996 e6225ec
fix codenarc
jandro996 867ab0f
change to forked tests
jandro996 800cc0c
fix spotless
jandro996 1c02218
fix coverage and tests
jandro996 963f7c8
fix secure marks in formatter
jandro996 69a3466
Add more tests
jandro996 dfef843
upgrade Opcodes.ASM9
jandro996 7c7e642
remove unnecessary change
jandro996 722e3f6
rename endpoint
jandro996 fff5baf
Add safeguard to security control class transform
jandro996 fab351a
Fix method adapter add logs and more test cases
jandro996 dee1faf
Fix jacoco
jandro996 7411d9b
changes suggested by manu
jandro996 045b376
changes suggested by manu
jandro996 49ad3d6
remove unnecessary check
jandro996 0ae0bdd
separate method visitors for adapters and input validators
jandro996 78e6d43
fix spotless
jandro996 c7290cb
Merge branch 'master' into alejandro.gonzalez/security-controls
jandro996 3eff5ce
Merge branch 'master' into alejandro.gonzalez/security-controls
jandro996 de8c198
Update dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securi…
jandro996 70186de
Merge branch 'master' into alejandro.gonzalez/security-controls
jandro996 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61 changes: 61 additions & 0 deletions
61
...t-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package com.datadog.iast.securitycontrol; | ||
|
|
||
| import datadog.trace.api.iast.securitycontrol.SecurityControl; | ||
| import java.lang.instrument.ClassFileTransformer; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import javax.annotation.Nullable; | ||
| import org.objectweb.asm.ClassReader; | ||
| import org.objectweb.asm.ClassVisitor; | ||
| import org.objectweb.asm.ClassWriter; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class IastSecurityControlTransformer implements ClassFileTransformer { | ||
|
|
||
| private static final Logger LOGGER = | ||
| LoggerFactory.getLogger(IastSecurityControlTransformer.class); | ||
|
|
||
| private final List<SecurityControl> securityControls; | ||
| private final Set<String> classFilter; | ||
|
|
||
| public IastSecurityControlTransformer(List<SecurityControl> securityControls) { | ||
| this.securityControls = securityControls; | ||
| this.classFilter = | ||
| securityControls.stream().map(SecurityControl::getClassName).collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public byte[] transform( | ||
| ClassLoader loader, | ||
| String className, | ||
| Class<?> classBeingRedefined, | ||
| java.security.ProtectionDomain protectionDomain, | ||
| byte[] classfileBuffer) { | ||
| if (!classFilter.contains(className)) { | ||
| return null; // Do not transform classes that are not in the classFilter | ||
| } | ||
| List<SecurityControl> match = getSecurityControl(className); | ||
| if (match == null || match.isEmpty()) { | ||
| return null; // Do not transform classes that do not have a security control | ||
| } | ||
| try { | ||
| ClassReader cr = new ClassReader(classfileBuffer); | ||
| ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); | ||
| ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); | ||
| cr.accept(cv, 0); | ||
|
jandro996 marked this conversation as resolved.
Outdated
|
||
| return cw.toByteArray(); | ||
| } catch (Throwable e) { | ||
| LOGGER.warn("Failed to transform class: {}", className, e); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private List<SecurityControl> getSecurityControl(final String className) { | ||
| return securityControls.stream() | ||
| .filter(sc -> sc.getClassName().equals(className)) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| } | ||
103 changes: 103 additions & 0 deletions
103
...ent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package com.datadog.iast.securitycontrol; | ||
|
|
||
| import datadog.trace.api.iast.securitycontrol.SecurityControl; | ||
| import datadog.trace.api.iast.securitycontrol.SecurityControlType; | ||
| import org.objectweb.asm.MethodVisitor; | ||
| import org.objectweb.asm.Opcodes; | ||
| import org.objectweb.asm.Type; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class SecurityControlMethodAdapter extends MethodVisitor { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(SecurityControlMethodAdapter.class); | ||
|
|
||
| public static final String HELPER = | ||
| "datadog/trace/api/iast/securitycontrol/SecurityControlHelper"; | ||
| public static final String METHOD = "setSecureMarks"; | ||
| public static final String DESCRIPTOR = "(Ljava/lang/Object;I)V"; | ||
| private final MethodVisitor mv; | ||
| private final SecurityControl securityControl; | ||
| private final String desc; | ||
|
|
||
| public SecurityControlMethodAdapter( | ||
| final MethodVisitor mv, final SecurityControl securityControl, final String desc) { | ||
| super(Opcodes.ASM9, mv); | ||
| this.mv = mv; | ||
| this.securityControl = securityControl; | ||
| this.desc = desc; | ||
| } | ||
|
|
||
| @Override | ||
| public void visitInsn(int opcode) { | ||
| // Check if the opcode is a return instruction | ||
| if (opcode >= Opcodes.IRETURN && opcode <= Opcodes.RETURN) { | ||
| if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { | ||
| processInputValidator(); | ||
| } else { // SecurityControlType.SANITIZER | ||
| processSanitizer(); | ||
| } | ||
| } | ||
|
|
||
| super.visitInsn(opcode); | ||
| } | ||
|
|
||
| private void processSanitizer() { | ||
| Type returnType = Type.getReturnType(desc); | ||
| if (isPrimitive(returnType)) { | ||
| // no need to check primitives as we are not tainting them | ||
| LOGGER.warn( | ||
| "Sanitizers should not be used on non-primitive return types. Return type {}. Security control: {}", | ||
|
jandro996 marked this conversation as resolved.
Outdated
|
||
| returnType.getClassName(), | ||
| securityControl); | ||
| return; | ||
| } | ||
| // Duplicate the return value on the stack | ||
| mv.visitInsn(Opcodes.DUP); | ||
| // Load the marks from securityControl onto the stack | ||
| mv.visitLdcInsn(securityControl.getMarks()); | ||
| // Insert the call to setSecureMarks with the return value and marks as parameters | ||
| mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); | ||
| } | ||
|
|
||
| private void processInputValidator() { | ||
| boolean allParameters = securityControl.getParametersToMark() == null; | ||
| Type[] types = Type.getArgumentTypes(desc); | ||
| int parametersCount = 0; | ||
| for (int i = 0; i < types.length; i++) { | ||
| Type type = types[i]; | ||
| boolean isPrimitive = isPrimitive(type); | ||
| if (allParameters) { | ||
| if (!isPrimitive) { | ||
| callInputValidation(parametersCount); | ||
| } | ||
| } else if (securityControl.getParametersToMark().contains(i)) { | ||
| if (isPrimitive) { | ||
| LOGGER.warn( | ||
|
smola marked this conversation as resolved.
Outdated
|
||
| "Input validators should not be used on primitive types. Parameter {} with type {} .Security control: {}", | ||
| i, | ||
| type.getClassName(), | ||
| securityControl); | ||
| } else { | ||
| callInputValidation(parametersCount); | ||
| } | ||
| } | ||
| parametersCount += types[i].getSize(); | ||
| } | ||
| } | ||
|
|
||
| private void callInputValidation(int i) { | ||
| // Duplicate the parameter value on the stack | ||
| mv.visitVarInsn(Opcodes.ALOAD, i); | ||
|
jandro996 marked this conversation as resolved.
Outdated
|
||
| // Load the marks from securityControl onto the stack | ||
| mv.visitLdcInsn(securityControl.getMarks()); | ||
| // Insert the call to setSecureMarks with the parameter value and marks as parameters | ||
| mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); | ||
| } | ||
|
|
||
| private static boolean isPrimitive(Type type) { | ||
| // Check if is a primitive type | ||
| int sort = type.getSort(); | ||
| return sort >= Type.BOOLEAN && sort <= Type.DOUBLE; | ||
| } | ||
| } | ||
71 changes: 71 additions & 0 deletions
71
...ast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package com.datadog.iast.securitycontrol; | ||
|
|
||
| import static org.objectweb.asm.Opcodes.ASM8; | ||
|
|
||
| import datadog.trace.api.iast.securitycontrol.SecurityControl; | ||
| import java.util.List; | ||
| import javax.annotation.Nullable; | ||
| import org.objectweb.asm.ClassVisitor; | ||
| import org.objectweb.asm.ClassWriter; | ||
| import org.objectweb.asm.MethodVisitor; | ||
| import org.objectweb.asm.Type; | ||
|
|
||
| public class SecurityControlMethodClassVisitor extends ClassVisitor { | ||
|
|
||
| private final List<SecurityControl> securityControls; | ||
|
|
||
| public SecurityControlMethodClassVisitor( | ||
| final ClassWriter cw, final List<SecurityControl> securityControls) { | ||
| super(ASM8, cw); | ||
| this.securityControls = securityControls; | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public MethodVisitor visitMethod( | ||
| int access, String name, String desc, String signature, String[] exceptions) { | ||
| MethodVisitor mv = super.visitMethod(access, name, desc, signature, exceptions); | ||
| if (mv == null) { | ||
| return null; | ||
| } | ||
| SecurityControl match = null; | ||
| for (SecurityControl securityControl : securityControls) { | ||
| if (shouldBeAdapted(securityControl, name, desc)) { | ||
| match = securityControl; | ||
| break; | ||
| } | ||
| } | ||
| if (match != null) { | ||
| return new SecurityControlMethodAdapter(mv, match, desc); | ||
| } | ||
| return mv; | ||
| } | ||
|
|
||
| public void visitEnd() { | ||
| cv.visitEnd(); | ||
| } | ||
|
|
||
| private boolean shouldBeAdapted(SecurityControl securityControl, String name, String desc) { | ||
|
|
||
| if (!securityControl.getMethod().equals(name)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (securityControl.getParameterTypes() == null) { | ||
| return true; | ||
| } | ||
|
|
||
| Type[] types = Type.getArgumentTypes(desc); | ||
| if (types.length != securityControl.getParameterTypes().size()) { | ||
| return false; | ||
| } | ||
|
|
||
| for (int i = 0; i < types.length; i++) { | ||
| if (!types[i].getClassName().equals(securityControl.getParameterTypes().get(i))) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.