-
Notifications
You must be signed in to change notification settings - Fork 2
Make CAP Log injection query more resilient and conservative #226
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
Conversation
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.
can we add a test case that shows the interprocedural case for the req params? nice work on that though, that totally makes sense to add!
additionally there are some current unit test changes, are those expected?
...ript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPLogInjectionQuery.qll
Outdated
Show resolved
Hide resolved
...ript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPLogInjectionQuery.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Kristen Newbury <knewbury01@github.com>
Co-authored-by: Kristen Newbury <knewbury01@github.com>
…advanced-security/codeql-sap-js into jeongsoolee09/improve-cap-log-injection
@knewbury01 Test cases are added in f2511a2. |
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.
lgtm!
What This PR Contributes
This PR contributes two things to the CAP log injection query: making it more resilient and more conservative.
Make the query more resilient
It is observed that the request parameter object can be tossed around interprocedurally until it is read downstream. For example:
The untrusted property
req.data
is read in a callee downstream. This code previously could not be detected since property reads are tracked only locally. Therefore, wrap the request parameter definition in a type tracker to track it inter-procedurally.Make the query more conservative
A CDS element declared in a
.cds
file can lack its type information for several reasons, notably because it borrows its type interface from one of the common aspects that comes with the NPM package@sap/cds
which CodeQL analysis can't get to. In this case, it makes sense to be conservative and decide that a request parameter is a barrier only if its matching type information can statically be found in its CDS declaration and it's eitherString
orLargeString
. That is, the query now conservatively decides that the request parameter data can be an injection vector if it can't be sure it's a string type, even if in fact it is not a string type.Future Works
None at the moment.