search

dotnet-security-guard / roslyn-security-guard

Roslyn analyzers that aim to help security audit on .NET applications.
https://dotnet-security-guard.github.io
GNU Lesser General Public License v3.0
208 stars 38 forks source link

OutputCache false positives fixes #68

Closed JarLob closed 7 years ago

h3xstream commented 7 years ago

Can you explain quickly what false positive(s) are eliminated by this change set?

JarLob commented 7 years ago

@h3xstream, It all started with the false positive [OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")] When duration in seconds is set to zero the caching is disabled. I have tried it in my sample Mvc application. I didn't investigate what is the subtle difference between an absence of the attribute and this combination. Looks like developers combine NoStore and Duration=0 to say never cache it.

But apparently there were more issues. If you revert the changes in OutputCacheAnnotationAnalyzer.cs, but keep new tests in OutputCacheAnnotationAnalyzerTest.cs you will see tests DetectAnnotation4, DetectAnnotation5, DetectAnnotation6, DetectAnnotation7. FalsePositive2, FalsePositive3, FalsePositive4, FalsePositive5, FalsePositive6 failing. Conflicting attributes were checked only in the same class, but they can be derived from parent class or method. Conflicting attributes both applied to a class were not checked. (False Negatives) OutputCache duration can be overridden in derived class or method. Attribute name comparison was by short name, not fully qualified name. An attribute in a different namespace would trigger the alert too. (False Positives)

JarLob commented 7 years ago

@h3xstream, is anything wrong with my pull request?

h3xstream commented 7 years ago

@JarLob Yup its fine. I just needed a moment to fully process the new code.