Conditional application of InjectableArguments #135
Conversation
…tion so it looks at the logical position of an argument instead of it's lexical one.
… checking a bit easier
…sion method with a ref-like parameter
|
@JarLob I'll move this to be against |
| if (!(kv.Key is int) && !(kv.Key is string str && int.TryParse(str, out _))) | ||
| throw new Exception("Condition key must be an argument index"); | ||
|
|
||
| var argIx = int.Parse(""+kv.Key); |
There was a problem hiding this comment.
It is odd to convert the key to string and back in case it is int. I would rather have a switch on is.
| var argIx = int.Parse(""+kv.Key); | ||
| if(argIx < 0) | ||
| throw new Exception("Condition key must be an argument index >= 0"); | ||
|
|
| [DataRow("Injectable", "\"\", userProvided", false)] | ||
| [DataRow("Injectable", "userProvided, \"\"", true)] | ||
| [DataRow("Injectable", "\"\", \"\"", false)] | ||
| [DataRow("Injectable", "userProvided, userProvided", true)] |
There was a problem hiding this comment.
You destroyed my beautiful formatting...
| if (e.InnerException != null) | ||
| Logger.Log($"{e.InnerException.Message}"); | ||
| Logger.Log($"\n{e.StackTrace}", false); | ||
|
|
There was a problem hiding this comment.
I would rather add the empty line before Logger.Log($"\n{e.StackTrace}", false);
| : null; | ||
|
|
||
| var behaviorApplies = behavior != null && BehaviorApplies(behavior.AppliesUnderCondition, methodSymbol, argList?.Arguments, state); | ||
|
|
| if (destIx >= vals.Length) | ||
| return false; | ||
|
|
||
| vals[destIx] = val.Value?.ToString(); |
There was a problem hiding this comment.
Again for performance it is better to avoid string allocation. Could the object be used instead?
| if (val.HasValue) | ||
| { | ||
| if (destIx >= vals.Length) | ||
| return false; |
There was a problem hiding this comment.
When does it happen? With params? Could you add a test? If this is impossible situation I would rather throw.
|
|
||
| foreach (var kv in condition) | ||
| { | ||
| var ix = int.Parse((string)kv.Key); |
There was a problem hiding this comment.
Again, remember about optimized internal configuration structure. This can be done only once during config loading.
| var ix = int.Parse((string)kv.Key); | ||
|
|
||
| if (ix >= vals.Length) | ||
| return false; |
There was a problem hiding this comment.
Invalid configuration? I would throw and let the user know his config is wrong.
| if (codeVal == null) | ||
| return false; | ||
|
|
||
| if (!codeVal.Equals(expectedVal.ToString())) |
… actually switch on the types for expected values; addresses security-code-scan#135 (comment) & security-code-scan#135 (comment)
…l lookup has the same effect; addresses security-code-scan#135 (comment) & security-code-scan#135 (comment)
…InjectableArguments. Squashed equivalent of security-code-scan#135
|
Closing this in favor of #137, which is against master. |
2 participants
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.
This also pulls in the commits from #134 , since that was needed to test successfully against the full Stack Overflow code base.