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

Conditional application of InjectableArguments #137

Merged

Conversation

@kevin-montrose
Copy link
Contributor

commented Aug 19, 2019

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 Condition to behaviors that allows control of whether a behavior's InjectableArguments apply, 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). InjectableArguments are 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.

…InjectableArguments. Squashed equivalent of #135
@JarLob

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Regarding tests. Is it when it doesn’t find any? I had similar issue. My workaround is:

  1. Running clean.cmd when switching branches.
  2. Using VS2019 - somehow it catches up the tests better.
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@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?

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Tests are fine. It failed uploading artifacts.

@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

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.

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

NP, enjoy vacations!

…alues in the conditional dictionaries to ints or bools
…lti-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
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@JarLob I think I've addressed everything, so it's ready for another review.

The only "marginal" fix is switching to a non-allocating BehaviorApplies. For the cases I've encountered (where there's one condition "clause") the new approach is just better, but this approach will do more passes over arguments than the old approach for conditions with multiple clauses. This feels fine to me, but I wanted to draw attention to it.

JarLob added 2 commits Sep 11, 2019
… 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.
@JarLob

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Sorry for delay. Looks good!

@JarLob JarLob marked this pull request as ready for review Sep 11, 2019
@JarLob
JarLob approved these changes Sep 11, 2019
@JarLob JarLob merged commit 1868095 into security-code-scan:master Sep 11, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

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.

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

I'll run it on some real projects tomorrow and make a new release. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.