Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Taint analysis vizualization (adding additional locations to TaintAnalyzer) #276

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

Draft
wants to merge 8 commits into
base: vs2019
Choose a base branch
Loading
from

Conversation

dbalikhin
Copy link

It is a new experimental feature that can be extremely helpful while reviewing any vulnerability found with taint analysis (aka all injections and not a single line item).

image

image

End goal: To be able to properly populate the SARIF file with all additional locations to visualize the code flow.

How:
A new flag TaintFlowVisualizationEnabled was introduced. It is disabled by default and works only with AuditMode=off.

Implementation is a bit hacky; instead of modifying a taint flow analysis logic (Roslyn stuff), I do additional post-processing to recreate the data flow. This way, I won't decrease performance significantly, and analyzers still produce the same results.
Due to internal limitations, the solution may not be ideal in its current state.

If it is something that you find interesting, please review the PR and provide feedback. I tried not to change too much

Example of code flow - Code Scanning with Github - I want to produce something similar at the end; this one is the first step:
code flow

@JarLob
Copy link
Contributor

JarLob commented Dec 10, 2022

Interesting! I'll take a look. Thanks!

@JarLob
Copy link
Contributor

JarLob commented Dec 13, 2022

I think it will be a great addition to Security-Code-Scan(SCS). Regarding a "hacky way" it is questionable what is better - hacky, but external to the analysis engine itself, or internal. Although there is a potential for infinite recursion or CPU consuming path finding while the work was already done. I'll investigate how many changes would it take to add it into the engine itself.

One blocker I need to solve before moving forward is that I'm contemplating changing SCS licensing a bit. But licensing is tricky, so it takes time.

It came to my attention that some companies provide paid Dev Sec Ops services relying partially on SCS which is allowed under current LGPL 3.0 license. To make it clear SCS if free for personal usage and for companies that use it internally in CI. I also don't mind if security researchers or companies provide commercial audit services using SCS as a tool. However including SCS into commercial Dev Sec Ops solutions without contributing back or sponsoring the project is simply exploitation of OSS.

SCS started as a fork of LGPL licensed https://github.com/dotnet-security-guard/roslyn-security-guard which was abandoned at that time. If I wanted to change the license of SCS I would probably need to collect approvals of all past contributors. However nobody except me has contributed to the Roslyn subfolder which was under Apache License and recently switched to MIT. I see it as an opportunity to release it under a different license that would prevent the abuse.

My next steps plan is:

  1. Make a VS2022 branch.
  2. License the Roslyn folder maybe under AGPL.
  3. Update Roslyn SDK to VS2022.
  4. Setup a GitHub action for the license agreement for new contributors.
  5. Make the changes for .NET 7 and release new version.
  6. Drop .net 3.1 & 5.0

Please let me know if you have any concerns.

@dbalikhin
Copy link
Author

Sounds good to me but with a couple of suggestions / observations:

  1. Most likely you will need a custom dual license. AGPL is not nuget friendly 😊
  2. Review Core 3.1 and 5.0 support, they are EOL but you might want to support for a bit longer
  3. Not sure about AuditMode and how many users are using it. For simplicity I would get rid of AuditMode. Technically it should provide more findings with maybe confidence, but in reality it was just code duplication for me.

@JarLob
Copy link
Contributor

JarLob commented Dec 13, 2022

  1. Review Core 3.1 and 5.0 support, they are EOL but you might want to support for a bit longer

Thank you for reminding it. I'll drop the support.

  1. Not sure about AuditMode and how many users are using it. For simplicity I would get rid of AuditMode. Technically it should provide more findings with maybe confidence, but in reality it was just code duplication for me.

The purpose of the AuditMode at first was to mark all the sinks without taint tracking. Some non taint analyzers use it like a flag for verbosity. Overall I think it is worth to keep the flag to have a grep on steroids for sinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.