-
Notifications
You must be signed in to change notification settings - Fork 100
Sqli starter #1391
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
base: master
Are you sure you want to change the base?
Sqli starter #1391
Conversation
| "SQLiMySQLBodyEM", | ||
| 100 | ||
| ) { args -> | ||
| setOption(args, "security", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will need an experimental EMConfig option for SQLi, which should be off by default (until we run experiments)
| <dependency> | ||
| <groupId>mysql</groupId> | ||
| <artifactId>mysql-connector-java</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are those dependencies here? they point of having separated modules for E2E for different databases was to avoid possible conflicts and configuration when starting spring (as it scans the classpath)
| fun getVulnerableForSSRF() : Boolean = getResultValue(VULNERABLE_SSRF)?.toBoolean() ?: false | ||
|
|
||
| fun setResponseTime(responseTime: Long) = addResultValue(RESPONSE_TIME, responseTime.toString()) | ||
| fun getResponseTime(): Long = getResultValue(RESPONSE_TIME)?.toLong() ?: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case of errors, this is not going to be set (same as status code). so maybe better to return a nullable, ie, Long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still returning Long instead of Long?
| return true | ||
| } | ||
|
|
||
| // Simple SQLi payloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** */, so it gets into the documentation of the variable
| { t, res -> | ||
| rcr.setResponseTime(t) | ||
| }, | ||
| {createInvocation(a, chainState, cookies, tokens).invoke()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the createInvocation() should be not counted. rather save it to variable call, and then measure time of call.invoke()
| actionResults: List<ActionResult>, | ||
| fv: FitnessValue | ||
| ) { | ||
| if (!config.isEnabledFaultCategory(DefinedFaultCategory.XSS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xss??? also need a check on SQLi, which would be off by default, as still experimental.
however, maybe we should avoid long checks like:
!config.xss || !config.isEnabledFaultCategory(DefinedFaultCategory.XSS)
where the check for config.xss (and new !config.sqli) should be done inside isEnabledFaultCategory. eg something like:
fun isEnabledFaultCategory(category: FaultCategory) : Boolean{
if(category==DefinedFaultCategory.XSS && !xss){
return false;
}
return category !in getDisabledOracleCodesList()
}
| // continue | ||
| // } | ||
|
|
||
| if(r.getResponseTime() < config.sqlInjectionMaxResponseTimeMs && !r.getTimedout()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is tricky, as there might be long executing tests regardless of SQLi, and still not timingout.
first, need to check if SQLi payload is in the test.
then, likely we need a new value in RestCallResult to mark the increase in time after the payload. recall what wrote in notes.txt (if unclear, we can have a meeting about it)
| val hasInvalidChars = leafGene.invalidChars.any { payload.contains(it) } | ||
| if(!hasInvalidChars){ | ||
| // append the SQLi payload value | ||
| leafGene.value = leafGene.value + payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the gene might have length constraints. still need to validate such constraints would not break the validity. furthermore, the StringGene might not be be in use due to tainted value. might need to call getPhenotype() to make sure that leafGene.value is indeed in use
| copy.modifySampleType(SampleType.SECURITY) | ||
| copy.ensureFlattenedStructure() | ||
|
|
||
| val evaluatedIndividual = fitness.computeWholeAchievedCoverageForPostProcessing(copy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we need check the execution time before and after the injection, to see if it was successful. but, then, reexecuting the tests would lose such info :( we need to think of a way to handle this properly (i don't have an easy solution right now)
|
|
||
| @Experimental | ||
| @Cfg("Maximum response time (in milliseconds) to consider a potential SQL Injection vulnerability.") | ||
| var sqlInjectionMaxResponseTimeMs = 2000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recall notes.txt, there are 2 values to handle (there called N and K)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need experimental config parameter for SQLi, false by default
| fun getVulnerableForSSRF() : Boolean = getResultValue(VULNERABLE_SSRF)?.toBoolean() ?: false | ||
|
|
||
| fun setResponseTime(responseTime: Long) = addResultValue(RESPONSE_TIME, responseTime.toString()) | ||
| fun getResponseTime(): Long = getResultValue(RESPONSE_TIME)?.toLong() ?: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still returning Long instead of Long?
| fun setVulnerableForSSRF(on: Boolean) = addResultValue(VULNERABLE_SSRF, on.toString()) | ||
| fun getVulnerableForSSRF() : Boolean = getResultValue(VULNERABLE_SSRF)?.toBoolean() ?: false | ||
|
|
||
| fun setResponseTime(responseTime: Long?) = addResultValue(RESPONSE_TIME, responseTime.toString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Long? as input? if it is null, this method should simply not be called. ie, input should be Long
| const val TCP_PROBLEM = "TCP_PROBLEM" | ||
| const val APPLIED_LINK = "APPLIED_LINK" | ||
| const val LOCATION = "LOCATION" | ||
| const val RESPONSE_TIME = "RESPONSE_TIME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the measure unit? minutes? if it is milliseconds, rename it into RESPONSE_TIME_MS (as well as getter/setter)
| return copy | ||
| } | ||
| } | ||
| var baseResponseTime: Long? = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a nullable variable is initialized with 0 instead of null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also info about responses should NOT be in the call, but rather in the response object
| val baseline = a.baseResponseTime | ||
| val afterPayload = r.getResponseTime() | ||
| val K = config.sqliBaselineMaxResponseTimeMs // K: maximum allowed baseline response time | ||
| val N = config.sqliInjectedSleepDuration * 1000 // N: expected delay introduced by the injected sleep payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't confusing that two related configs are using different time units? ie, seconds and millisecond. shouldn't both use milliseconds?
|
|
||
| if(!hasInvalidChars && !hasMaxLength){ | ||
| // append the SQLi payload value | ||
| leafGene.getPhenotype().setFromStringValue(leafGene.getPhenotype().getValueAsRawString() + String.format(payload, config.sqliInjectedSleepDuration)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setFromStringValue returns false if it fails to update
| val evaluatedIndividual = fitness.computeWholeAchievedCoverageForPostProcessing(copy) | ||
|
|
||
| if (evaluatedIndividual == null) { | ||
| log.warn("Failed to evaluate constructed individual in handleStackTraceCheck") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleStackTraceCheck???
| if(!anySuccess){ | ||
| continue | ||
| } | ||
| actionCopy.baseResponseTime = responseTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this approach is hacky, and would not work if test is re-evaluated for any reason. i think we need a singleton, in which we keep track, for each action, the min/mean/max exection time during the search (and NOT during security phase). then, when we need a baseline, we query the singleton.
|
|
||
| @Experimental | ||
| @Cfg("Injected sleep duration (in seconds) used inside the malicious payload to detect time-based vulnerabilities.") | ||
| var sqliInjectedSleepDuration = 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be consistent with time units
| |`saveMockedResponseAsSeparatedFile`| __Boolean__. Whether to save mocked responses as separated files. *Default value*: `false`.| | ||
| |`saveScheduleTaskInvocationAsSeparatedFile`| __Boolean__. Whether to save schedule task invocation as separated files. *Default value*: `false`.| | ||
| |`saveTargetHeuristicsPrefixes`| __String__. Prefix specifying which targets to record. Each target can be separated by a comma, such as 'Branch,Line,Success, etc'. It is only used when processFormat is TARGET_HEURISTIC. *Default value*: `Branch`.| | ||
| |`seedTestCases`| __Boolean__. Whether to seed EvoMaster with some initial test cases. These test cases will be used and evolved throughout the search process. *Default value*: `false`.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not forget about generated test cases (in Java/Kotlin/Python/Javascript) need to then log the execution time (eg startTime-endTime) and assert it is more than N. but best to do it in a new PR after this one is merged
No description provided.