ascanrules: SQLi: add checking for redirection after form input #3284
Conversation
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
|
ideally the changes should be accompanied with a test(s) that verifies them. IMO it would be better apply the header check to all injections not just OR, this also applies to any redirection (likely in any form) not just logins. |
thc202
reviewed
Oct 29, 2021
|
@koneko096 are you going to address the earlier feedback? |
|
Sure @kingthorin, will continue again this week |
|
@koneko096 do you plan to finish this? |
koneko096
force-pushed
the
sqli-by-redirection
branch
from
decc869 to
89c1c33
Compare
kingthorin
reviewed
Apr 16, 2022
...anrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java
Outdated
Show resolved
Hide resolved
koneko096
force-pushed
the
sqli-by-redirection
branch
from
89c1c33 to
e42cb43
Compare
koneko096
changed the title
koneko096
force-pushed
the
sqli-by-redirection
branch
2 times, most recently
from
cc93987 to
81877de
Compare
|
Hi @kingthorin, I had updated following earlier feedbacks. Please help to review. Thank you |
kingthorin
reviewed
Apr 30, 2022
addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java
Outdated
Show resolved
Hide resolved
addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java
Outdated
Show resolved
Hide resolved
kingthorin
added
the
waiting-for:author
koneko096
force-pushed
the
sqli-by-redirection
branch
2 times, most recently
from
aedf34a to
d2a26e3
Compare
koneko096
force-pushed
the
sqli-by-redirection
branch
from
d2a26e3 to
e6ec9e1
Compare
kingthorin
approved these changes
Dec 27, 2022
kingthorin
removed
the
waiting-for:author
koneko096
force-pushed
the
sqli-by-redirection
branch
2 times, most recently
from
2b96865 to
5fc820f
Compare
In case of redirection after authentication, the body may be empty and content comparison won't work. By adding new checking for location header, we may catch potential vulnerability especially in case of OR true injection. Signed-off-by: koneko096 <[email protected]>
koneko096
force-pushed
the
sqli-by-redirection
branch
from
5fc820f to
9dc141d
Compare
|
hey guys @kingthorin @thc202 any follow up on this MR? |
kingthorin
reviewed
Apr 9, 2024
Comment on lines
+1959
to
+1960
| String confirmExpressionLocationHeader = | ||
| msg.getResponseHeader().getHeader(HttpHeader.LOCATION); |
There was a problem hiding this comment.
Could extract a convenience method for this accepting HttpMessage and returning String.
Then leverage it throughout.
Comment on lines
+248
to
+254
| protected Response getResponse(String value) { | ||
| Response response = | ||
| newFixedLengthResponse( | ||
| Response.Status.REDIRECT, NanoHTTPD.MIME_HTML, ""); | ||
| response.addHeader("Location", "http://somewhere"); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Could extract to a convenience method to avoid duplication.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In case of redirection after form input, the body may be empty and content comparison won't work. By adding new checking for location header, we may catch potential vulnerability after boolean or arithmetic expression.
Fixes: #6883