-
-
Notifications
You must be signed in to change notification settings - Fork 676
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
ascanrules: SQLi: add checking for redirection after form input #3284
base: main
Are you sure you want to change the base?
Conversation
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. |
@koneko096 are you going to address the earlier feedback? |
Sure @kingthorin, will continue again this week |
@koneko096 do you plan to finish this? |
decc869
to
89c1c33
Compare
...anrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java
Outdated
Show resolved
Hide resolved
89c1c33
to
e42cb43
Compare
cc93987
to
81877de
Compare
Hi @kingthorin, I had updated following earlier feedbacks. Please help to review. Thank you |
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
aedf34a
to
d2a26e3
Compare
d2a26e3
to
e6ec9e1
Compare
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.
Haven’t tested against anything else but seems okay to me.
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 <laser.survivor@gmail.com>
5fc820f
to
9dc141d
Compare
hey guys @kingthorin @thc202 any follow up on this MR? |
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.
Seems okay overall to me. It should be rebased to pickup the latest base branch changes.
String confirmExpressionLocationHeader = | ||
msg.getResponseHeader().getHeader(HttpHeader.LOCATION); |
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.
Could extract a convenience method for this accepting HttpMessage and returning String.
Then leverage it throughout.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could extract to a convenience method to avoid duplication.
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