Skip to content
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

SQL injection detection #12

Merged
merged 87 commits into from
Jul 25, 2024
Merged

SQL injection detection #12

merged 87 commits into from
Jul 25, 2024

Conversation

CaspianA1
Copy link
Collaborator

@CaspianA1 CaspianA1 commented Jul 10, 2024

Mostly done, but not quite; I just made a PR to run e2e tests and get feedback on my progress so far.

Here's a summary of my changes:

  1. Improved global variable mutation detection (can now detect weird variable swaps, like z = [y, y = z][0];, and modifications separated by a comment, like x = 23 + x, y.a = 24 + x;). This covers all assignment expression types, I believe, except for spread assignments, which I will need to implement at some point.
  2. Minor function name changes, README changes, stuff like that
  3. Lots of SQL injection detection code
  4. Add lots of test cases for this
  5. Some dependency tweaks, to make the chances of broken installs of this package yet smaller

Here's how I determine probable SQL injections:

  • If I detect a callsite where raw SQL queries can happen (e.g. ctxt.client.raw(myQuery), where the ORM client is Knex), figure out if the value is reducible to a string literal, or concatenations of string literals.
  • This means that the parameter can be a string literal, a variable that is only ever assigned to string literals, a concatenation of string literals, a variable that's assigned to concatenations of string literals with other such variables, etc...
  • Template expressions (e.g. `${foo}, ${bar}, ${baz}`) are also supported, as long as the values used to build it (foo, bar, and baz here) are also reducible to string literals. This does not include tagged template expressions, since those involve a function call, and my variable definition analysis stays within the scope of a single Transaction function.
  • Using any of a Transaction function's parameters for a raw SQL query is not allowed either, since you don't know who will call that transaction except in certain cases.
  • If the passed-in variable to the query is not local, I examine the whole scope of the program for any non-literal-reducible assignments. If I find any, then I raise an error.
  • If some variable passed into a callsite for raw SQL queries does not meet the aforementioned requirements, I flag it as a SQL injection, because that then means that you most likely took some datatype and cast it to a string to build your query, which is risky.

Here's what's left:

  • Fixing some versioning hassle
  • Add support for more callsites (beyond just Knex's raw function). Should be relatively easy.
  • Proper line reporting for errors (provide the line where the potential injection happened, not the line of the raw SQL call). Medium difficulty (not particularly hard, it'd just take an hour or two to implement).
  • Dealing with type aliasing

And things to think about after that:

  • Maybe split everything into more files (but that might be another PR)
  • Version bump

CaspianA1 added 30 commits July 8, 2024 12:00
- Change some function and type names
- Prepare for detecting SQL injection
…tection. Flesh another function out slightly more.
…rue before, I just never noticed this earlier)
…obust (although there are still some more cases to handle). Replace `getLocal` with `isLocal` again.
- Expand the test cases
- Now inferring when assignment is happening correctly (it was very half-assed before, and I had some differing logic on how to handle it - both incomplete ways! - in the global variable detection code and the SQL injection detection code)
- General improvements: supporting all assignment operator types, global variable detection can now handle one of the weird variable swap cases, and the stack frames are now sets again, instead of maps.
…ic worked out, it's quite elegant IMO.

- I now just need to clean up a bit of the code, support more callsites, disallow format strings, and test some other binary operators on strings too.
- Also replace all throws with a `panic` function that I made (following DRY!).
…s global (for easy tweaking in the future). Remove a comment, and add a TODO.
… detect SQL injection in a more clear way. More type renaming.
…ecorator set error checker mapping to evaluate different determinism checkers)
@CaspianA1 CaspianA1 merged commit 9c78404 into main Jul 25, 2024
1 check passed
@CaspianA1 CaspianA1 deleted the CaspianA1/sql_injection_detection branch July 25, 2024 00:41
CaspianA1 added a commit to dbos-inc/dbos-docs that referenced this pull request Jul 25, 2024
Going to merge this once
[this](dbos-inc/eslint-plugin#12) PR is merged

---------

Signed-off-by: Caspian <[email protected]>
Co-authored-by: Qian Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants