security-code-scan / security-code-scan Goto Github PK

Currently ServicePointManager.ServerCertificateValidationCallback is checked

This code will generate two warnings instead of one

            string query = input;
            SqlCommand cmd1 = new SqlCommand(query);

            query = ""SELECT * FROM [User] WHERE user_id = 1"";
            SqlCommand cmd2 = new SqlCommand(query);

<%Page EnableViewStateMac = false

Are cleansing functions taken into consideration by the analyzer? Not just for open redirect but also xss and other vulnerabilities that need sanitizing.

This still shows a warning:

//if (!string.IsNullOrEmpty(model.ReturnUrl))
if (Url.IsLocalUrl(model.ReturnUrl))
{
    return Redirect(model.ReturnUrl);
}

The only way to get rid of the warning is to hard code the url string which isn't practical...
return Redirect("www.google.com");

Currently in tests nothing prevents GetAdditionalReferences from calling MetadataReference.CreateFromFile multiple times on the same file. This may be costly. The parent class DiagnosticVerifier may track what PortableExecutableReference for what file were already created.

https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/network/servicepointmanager-element-network-settings
https://msdn.microsoft.com/en-us/library/system.net.servicepointmanager.encryptionpolicy(v=vs.110).aspx

Hi,

We are looking at using Security-Code-Scan to provide static code analysis with security in mind, however running a scan on some legacy VB.net code I found an issue which stops the TaintAnalyzer from going through a method.

The simplest example I have boiled it down to is the ability in VB.net (though this seems impossible in C#) to declare a loop variable outside of the loop.

e.g.
the code:

 Sub Main(args As String())
    For Each argItem As String In args
        Console.WriteLine(argItem)
    Next
End Sub

appears to be analyzed fine, however moving the declaration of argItem out of the loop as in:

Sub Main(args As String())
    Dim argItem As String

    For Each argItem In args
        Console.WriteLine(argItem)
    Next
End Sub

causes the error:
Severity Code Description Project File Line Suppression State
Warning AD0001 Analyzer 'SecurityCodeScan.Analyzers.Taint.TaintAnalyzer' threw an exception of type 'System.Exception' with message 'Unhandled exception while visiting method Sub Main(args As String())
argItem As String

    For Each argItem In args
        Console.WriteLine(argItem)
    Next
End Sub : Unable to cast object of type 'Microsoft.CodeAnalysis.VisualBasic.Syntax.IdentifierNameSyntax' to type 'Microsoft.CodeAnalysis.VisualBasic.Syntax.VariableDeclaratorSyntax'.'.	SecurityCodeScanBreaker		1	Active

Warn if always returns true.
Detect SslStream constructor call when the delegated is not created explicitly like:

            SslStream sslStream = new SslStream(
                client.GetStream(),
                false,
                ValidateServerCertificate,
                null
            );

https://msdn.microsoft.com/en-us/library/system.net.security.remotecertificatevalidationcallback(v=vs.110).aspx

There are few tests commented out with

// todo: add C# 7.0 support

in the code. Switching to a newer Roslyn analyzers nuget possibly is needed.

FSPickler
Sweet.Jayson

https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/network/servicepointmanager-element-network-settings
https://msdn.microsoft.com/en-us/library/system.net.configuration.servicepointmanagerelement.checkcertificatename(v=vs.110).aspx

If you are using ASP.NET 4.0 or higher, you have the option of extending or replacing the Request Validation logic by providing your own class that descends from System.Web.Util.RequestValidator. By implementing this class, you can determine when validation occurs and what type of request data to perform validation on.
https://www.owasp.org/index.php/ASP.NET_Request_Validation

class WeakHashing
{
    static string Sha256Name { get { return ""System.Security.Cryptography.SHA256""; } }
    static void Foo(string name)
    {
        var sha = HashAlgorithm.Create(Sha256Name);
    }
}

Taint analyzer should be used.

Currently hardcoded in the taint analyzer:

                        case "System.String.Empty":
                        case "System.IntPtr.Zero":
                        case "System.IO.Path.AltDirectorySeparatorChar":
                        case "System.IO.Path.DirectorySeparatorChar":
                        case "System.IO.Path.InvalidPathChars":
                        case "System.IO.Path.PathSeparator":
                        case "System.IO.Path.VolumeSeparatorChar":

Looks like the optimization based on node name:

            var objectCreation = nodeHelper.GetNameNode(ctx.Node);
            if(!objectCreation.ToString().Contains("JavaScriptSerializer"))
                return;

misses some corner cases:

using System.Web.Script.Serialization;
using JSS = System.Web.Script.Serialization.JavaScriptSerializer;

namespace VulnerableApp
{
    class Test
    {
        private JSS serializer = new JSS(new SimpleTypeResolver());
    }
}

It is just an example, all code should be reviewed.
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-directive

Add DSA, DSASignatureFormatter, HMACMD5, TripleDES, RIPEMD160 and HMACRIPEMD160.

Currently currently has to be done manually https://security-code-scan.github.io/#AnalyzingConfigFiles

Please can you clarify the following statement:

"Works on Visual Studio 2015 or higher. Free Visual Studio Community and paid Professional and Enterprise editions are supported."

That appear on https://security-code-scan.github.io/

I, and colleague have Visual Studio 2015 professional. Do we need to pay any licencing for this extension, or is it completely free to use?

I downloaded and install your extension from https://marketplace.visualstudio.com/items?itemName=JaroslavLobacevski.SecurityCodeScan after hearing about it on https://marketplace.visualstudio.com/items?itemName=PhilippeArteau.RoslynSecurityGuard
What I've done so far:

• I reopened VS2017 and my my solution in
• I blindly added AdditionalFileItemNames>$(AdditionalFileItemNames);Content</AdditionalFileItemNames> to my .csproj
• I checked [x] Editor > C# (or Basic) > Advanced > “Enable full solution analysis”
• I did a rebuild (noticed nothing, I I know my app has XSS and CSRF issues since I've only partially covered my public class ServicesController : ApiController [HttpPost]s with [AntiForgeryToken]
• I rebooted VS2017 and did another build, still nothing in Error List.

Did I miss something?

where IgnorableServerCertificateErrors is IList<ChainValidationResult> like:

var myWebSocket = new MessageWebSocket();
myWebSocket.Information.IgnorableServerCertificateErrors.Add(ChainValidationResult.Untrusted);

It is not specific to MessageWebSocket as exists in HttpBaseProtocolFilter too.

Currently some issues are reported in aspx or config files, but there is no way to suppress the warning. Maybe a specific html/xml comment can be introduced to suppress warnings in the next line.

certificateValidationMode and customCertificateValidatorType: https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/wcf/authentication-of-clientcertificate-element
https://msdn.microsoft.com/en-us/library/system.servicemodel.security.x509servicecertificateauthentication.certificatevalidationmode(v=vs.110).aspx and https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/windows-identity-foundation/certificatevalidation

https://msdn.microsoft.com/en-us/library/system.servicemodel.security.x509servicecertificateauthentication.customcertificatevalidator(v=vs.110).aspx
https://docs.microsoft.com/en-us/dotnet/api/system.identitymodel.selectors.x509certificatevalidator?view=netframework-4.7.2

A continuation of #15

There may be incompatible changes in configuration schema between versions. To support that:

1. A configuration file should has a property Version. It is better to have it as a property instead of filename, because configuration files may be per projects.
2. The version number should be separate from assembly version because there may be no configuration schema changes between different SCS versions.
3. When SCS is updated it should attempt to (an investigation is needed what is possible):
   • update the config in AppData in VS extension case. Since the location is shared between different installations (vs2015, vs2017 and nugets) the upgrade should create a new file in the folder (version number in the file name).
   • update the config in project when NuGet package is updated or during analysis in VS extension case (Investigate if it is even possible to have two versions of SCS running from NuGet and from VS extension at the same time. If it is, then maybe VS extension should convert the config in memory on the fly only).
4. If upgrade has failed or the version number is newer than supported SCS should show a fake analysis warning (SCS_Error1?) when the analysis is kicked off.
   • SCS_Warning for a suggestion to upgrade when doing memory only upgrade?
   • Since a user may change the number manually or the file maybe corrupted SCS_Error is needed if an exception happened during config loading.

For some deserializers like XmlSerializer and DataContractSerializer both type and serialized data have to be tainted to make it exploitable. Currently it gives false positives if only one is:

var deserializer = new XmlSerializer(typeof(MyType));
var my  = deserializer.Deserialize(input) as MyType;

It doesn't mean the MyType cannot be used as a gadget, the class should be investigated further.

• Keep current behavior for Auditing mode
• Show the warning only if both arguments are tainted

Do we need the check? Debug.WriteLine will be definitely not noticed...

There should be no warning because it serializes.

        static void Serialize(Type type, object data)
        {
            XmlSerializer xs = new XmlSerializer(type);
            StreamWriter writer = File.CreateText("");
            xs.Serialize(writer, data);
            writer.Flush();
            writer.Close();     
        }

https://github.com/ViktorijaAdo/security-code-scan/tree/unsafe_deserialization

love to see this available for core 2.x projects

1. The check if input value is used in output is incorrect, triggers also:

using System.Web.Mvc;

namespace VulnerableApp
{
    public class TestController : Controller
    {
        [HttpGet]
        public string Get(int sensibleData)
        {
            var x = sensibleData;
            return "value";
        }
    }
}

2. Input variables that cannot be used to inject javascript like int also trigger the warning.
3. EncodingMethods array is not used. A variable is considered encoded if used as an argument to any function.

Should be rewritten similar to CookieAnalyzer.

Although it doesn't clone private members...

        public T DeepClone<T>(T source)
        {
            var serializeSettings = new JsonSerializerSettings {TypeNameHandling = TypeNameHandling.All};
            var serialized = JsonConvert.SerializeObject(source, serializeSettings);
            return JsonConvert.DeserializeObject<T>(serialized, serializeSettings);
        }

Needs to use taint analysis or something similar to XXE analyzers.

Starting with ASP.NET 4.5 you can disable request validation at the individual server control level by setting ValidateRequestMode to "Disabled".
<asp:TextBox ID="txtASPNet" ValidateRequestMode="Disabled" runat="server" />

like:

            PasswordValidator pwdv = new PasswordValidator
            {
                RequireNonLetterOrDigit = true,
                RequireDigit = true,
            };

            pwdv.RequiredLength = " + (Constants.PasswordValidatorRequiredLength - 1) + @";
...
            pwdv.RequiredLength = " + (Constants.PasswordValidatorRequiredLength + 1) + @";

I have some ADO dataset generated code, where I see many instances of the following:

SCS0007 XML parsing vulnerable to XXE

The default setting for VS is supposed to suppress code analysis errors on generated code but SCS does not seem to do so. Is there a workaround? For example, is it possible to suppress analysis on the entire class via the global suppression file?

First, thanks for the effort building a free library with some security related checks 👍

I want to get an overview of all security warnings in a project. For prototyping this I've created a VSTS build for the WebGoat.NET sample project. The analyzer is picked up fine and I get the 22 expected errors in the detailed log:

When taking a look at the Build overview, I'm missing however half of the warnings:

I'm not sure whether this is a configuration issue on my side or something wrong in VSTS. Do you have any experience with this?

This should not give a waring:

class Test
{
    public static readonly string Safe = ""Safe"";

    static void TestMethod()
    {
        new SqlCommand(Safe);
    }
}

Or at least allow overriding the built in settings with settings from external file. If SCS is installed as extension it should be in the extension folder. In nuget case - per project file.

• Combine different settings yml files into one.
• Load and merge rules: Built in, global, project.

Microsoft has recently released a nice doc on TLS/SSL best practices here:

https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls

It recommends setting a number of things and also discourages the use of others. These could easily be turned into analyzers.

• Add the configuration setting
• Enable reporting questionable warnings

        static void Test()
        {
            var uri = new UriBuilder();
            uri.Password = "t0ps3cr3t";
        }

gives a warning, but not:

        static void Test()
        {
            string password = "t0ps3cr3t";
        }