search

github / securitylab

Resources related to GitHub Security Lab
https://securitylab.github.com
MIT License
1.35k stars 242 forks source link

[JS]: added sqlite and TypeORM SQLI Sinks #790

Closed am0o0 closed 6 months ago

am0o0 commented 9 months ago

Query PR

https://github.com/github/codeql/pull/14302

Language

Javascript

CVE(s) ID list

CWE

CWE-089

Report

I've added SQL query builder sinks that should not get values from untrusted sources. unlike parameterized sinks that are safe and suggested by documentation usually, using user-controllable input directly to build database queries can be dangerous.

Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc).

Blog post link

No response

am0o0 commented 9 months ago

Hi @p- I think there are some points of the sqlite3 package that haven't been modeled yet, please let me know if still I can upgrade this package. Also, I had some additional taint steps for the sequelize package which was really helpful, can I move them from my DOS query to here?

p- commented 9 months ago

Hi @amammad 👋

I think there are some points of the sqlite3 package that haven't been modeled yet, please let me know if still I can upgrade this package.

Yes if you want to you can still do that and let me know afterwards 👍

p- commented 9 months ago

@jorgectf

Also, I had some additional taint steps for the sequelize package which was really helpful, can I move them from my DOS query to here?

WDYT?

jorgectf commented 9 months ago

Please go ahead with the move, I can wait for this submission to finish to begin the DOS one.

am0o0 commented 8 months ago

@p- I think adding sequelize additional taint steps that can help to have a bigger taint tracking scope is really helpful but it can have FPs which I think is not a good idea if I submit it here, I have it in my git stash now maybe in future I give it a chance, instead, I've added the better-sqlite3 package and I updated the pg too!

ghsecuritylab commented 8 months ago

Your submission is now in status Test run.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

ghsecuritylab commented 8 months ago

Your submission is now in status Results analysis.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

ghsecuritylab commented 8 months ago

Your submission is now in status Query review.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

am0o0 commented 8 months ago

@p- I found one CVE related to TypeORM. It led me to uncover new sinks that weren't documented directly I assume. There is one more sink about active records too which is a little bit hard to find with codeql, but I'm working on it, Do I have any chance to solve these two issues or should I coordinate that in PR? I'm sorry for taking your time too.

am0o0 commented 8 months ago

@p- The fix is ready, there is one more problem that I'm going to solve and that's the remote flow sources of nestjs which are related to CVE. So I'll submit another all-in-one if, after my initial assessments about nestjs support range in codeql, there is a lot of work to do otherwise I think it is good to add that here as it is related to CVE. I'm waiting to hear from you to let me push TypeORM updates to PRs too.

p- commented 8 months ago

Hey @amammad 👋 I understand, that happens sometimes. I think the best thing for now is if you mark the PR as draft again and let me now once it's ready so I can perform a new query run. Best if you leave a short comment on the PR too. (The Query review stage will not change for that as we're out of the process in this situation.)

am0o0 commented 8 months ago

Hi @p- I think I can't mark it as a draft. I can't see any Draft option in the PR.

p- commented 8 months ago

@amammad Ok then we'll leave it at that for now. I wrote a comment in the PR.

am0o0 commented 8 months ago

Hi @p- it seems that there are many works for implementing nestjs remote sources because there are no default controllers in module definitions. It seems they are using fastify somehow as a custom controller, so the FP will be a little high if I want to implement it in codeql quickly because the PR is open now and I don't want to make it in waiting status a lot. I'll do this in another PR in the future.

ghsecuritylab commented 7 months ago

Your submission is now in status Final decision.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

ghsecuritylab commented 6 months ago

Your submission is now in status Pay.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

xcorail commented 6 months ago

Created Hackerone report 2278922 for bounty 536606 : [790] [JS]: added sqlite and TypeORM SQLI Sinks

ghsecuritylab commented 6 months ago

Your submission is now in status Closed.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed