Conditional application of InjectableArguments #137
Conversation
…InjectableArguments. Squashed equivalent of security-code-scan#135
|
Regarding tests. Is it when it doesn’t find any? I had similar issue. My workaround is:
|
|
@JarLob Eh, I can get tests to run in 2017 but not 2019 on master. Clean or not, doesn't really matter. As always, I'm tempted to blame my local environment. I don't see a failing test in the AppVeyor logs, and tests (in VS2017 at least) all pass? |
|
Tests are fine. It failed uploading artifacts. |
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
SecurityCodeScan/Analyzers/Taint/CodeEvaluation/CSharpCodeEvaluation.cs
Outdated
Show resolved
Hide resolved
|
Commenting just to say these all look like legitimate issues, appreciate the thorough review. I’m currently on vacation, so my computer time is a bit circumscribed for another week or so. If I find myself with some downtime I’ll take a whack at fixing these while I’m traveling, otherwise I’ll pick this back up the first week of September. |
|
NP, enjoy vacations! |
…s on constructors with default parameters
…d, and force all values in the conditional dictionaries to ints or bools
… actual behavior applies check
…vin-montrose/security-code-scan into behavior-condition-injectable
…locate, but for multi-term conditions will have to pass over arguments multiple times - pretty classic time-space tradeoff. More sophisticated options (like maybe putting the old one on the stack, or stashing bits in ExecutionState) feel like overkill as the common case is 0 or 1 conditionals
|
@JarLob I think I've addressed everything, so it's ready for another review. The only "marginal" fix is switching to a non-allocating |
… most probably the performance of the old object.ToString and new is/as string is comparable. At least it is more explicit now that only int, bool and string are supported.
|
Sorry for delay. Looks good! |
|
Excellent and appreciated. Is there an expected date by which new releases of the Nuget packages are expected? Now that this has landed, I'm just about ready to make Security Code Scan analyzers a part of regular Stack Overflow builds. All that's holding me back is that I'm currently using a local build of Security Code Scan's v2017 branch. |
|
I'll run it on some real projects tomorrow and make a new release. Thank you for your contributions! |
2 participants
This is #135, except against master.
I squashed the commits because there was a lot of noise, and cherry-picking all of them was kind of pointless.
As with #136, tests really do not like running on my box. Makes me want to look into unify master and vs2017 branches, as I suspect the issue is in VS.
This is a proposal (and thus a draft PR) to add a
Conditionto behaviors that allows control of whether a behavior'sInjectableArgumentsapply, akin to the Condition on attributes from #127 .Currently, at least as I understand, it is possible to conditionally control what taint is applied to a returned value (either absolutely, or via arguments).
InjectableArgumentsare unconditionally considered.The motivating use here are RedirectAction returning methods in the Stack Overflow codebase which default to disallowing external domains, but have a parameter that when set allows external targets. Calls are therefore vulnerable to an open redirect injection only when the parameter is set. A minimal example of this is in the included test.