ZupIT/horusec

"SQL Injection Entity Framework" rule false positive

Closed this issue · 2 comments

What happened:
This rule gives a false positive when we use the ExecuteSqlCommand command like this:

Database.ExecuteSqlCommand("DELETE FROM Table1 WHERE Id= @p0", id);

What you expected to happen:
According to the EF6 method documentation (https://github.com/dotnet/ef6/blob/main/src/EntityFramework/Database.cs#L568) this is a safe way to avoid SQL Injection. It is also possible to verify in the EF6's source code that when using the overload above the arguments will be converted into SqlParameter: https://github.com/dotnet/ef6/blob/df31fde50966a117180459313024d95c0a004162/src/EntityFramework/Core/Objects/ ObjectContext.cs#L5034

We know that it is possible to suppress false positives, but in the case of our project there are many points where this ExecuteSqlCommand signature is used, so it is not feasible to suppress them individually.

How to reproduce it (as minimally and precisely as possible):

Database.ExecuteSqlCommand("DELETE FROM Table1 WHERE Id= @p0", id);

Anything else we need to know?:
From what I could see, the problem with this rule is in this regex:

regexp.MustCompile(`(Database\.ExecuteSqlCommand)(([^S]|S[^q]|Sq[^l]|Sql[^P]|SqlP[^a]|SqlPa[^r]|SqlPar[^a]|SqlPara[^m]|SqlParam[^e]|SqlParame[^t]|SqlParamet[^e]|SqlParamete[^r])*)(\);)`),

Perhaps the regex should consider this other signature as valid.

BTW, thanks for Horusec, a great tool!

Environment:

  • Horusec version (use horusec version): v2.0.0
  • Operating System: Windows 10
  • Network plugin / Tool and version (if this is a network-related / tool bug): N/A
  • Others: N/A

thank you so much for the feedback @giacomelli it helps us a so much !

It really is a possibility to improve our assertiveness, our team will prioritize as soon as possible!

hello @giacomelli

We tried several options to reduce the amount of false positives in the structure we have today, but without success.

so our approach will be to use the semantic analysis as it is written in our ROADMAP.md, so we will have a little more intelligence in the analysis.

I'll close this issue for now, any doubts can call us again :)