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

Rules update #8

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Rules update #8

wants to merge 5 commits into from

Conversation

gand3lf
Copy link

@gand3lf gand3lf commented Jul 9, 2024

Hi Marco, congratulations for this repository!

With this pull request, I propose a little update about these rules:

  • command-injection: exec* checks
  • double-free: support to loops
  • incorrect-use-of-free: return without free
  • incorrect-use-of-strncat: added the use of strnlen

And a new rule:

  • insecure-random-seed: situations about the use of dangerous seeds.

I hope to have time in future to contribute also on the other rules. ✌️

@0xdea
Copy link
Owner

0xdea commented Jul 19, 2024

Hi Riccardo, thank you for your interest in this project!

I'm going to need some time to properly review your PR before accepting it. Just a few remarks after a cursory look:

  • command-injection: exec* checks - unless I'm missing something, these are not command injections, but argument injections at best
  • double-free: support to loops - this could be a valid PR, I've got to look into it more closely to ensure that it doesn't cause too many false positives... My original rule is definitely suboptimal, but I found it tricky to improve it while keeping it reliable enough
  • incorrect-use-of-free: return without free - this detects a memory leak; as a choice, my ruleset doesn't detect those as their security impact is not particularly interesting
  • incorrect-use-of-strncat: added the use of strnlen - this looks like a valid improvement
  • insecure-random-seed: situations about the use of dangerous seeds - this new rule looks interesting, but so far I don't have any taint rules in my ruleset, so it would need some testing to get used to it

Thanks again for the contribution 👍

@gand3lf
Copy link
Author

gand3lf commented Sep 6, 2024

Hi Marco, I completely agree with your comments. Please let me know if you would like me to modify anything.

Regarding the taint mode, I strongly suggest considering it, since Semgrep Pro (taint mode with interfile and interprocedure) offers interfile and interprocedure analysis at no extra cost (for single users).

Cheers!

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.

2 participants