From 2e4482372c7bf8480bfe2e387115ea4e5f043fb7 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Mon, 24 Jun 2019 17:07:05 +0800 Subject: [PATCH 01/12] Do not hard code encryption key. --- .../Security/DoNotHardCodeEncryptionKey.cs | 28 ++ .../SourceTriggeredTaintedDataAnalyzerBase.cs | 12 + .../SystemSecurityCryptographyResources.resx | 9 + ...SystemSecurityCryptographyResources.cs.xlf | 15 + ...SystemSecurityCryptographyResources.de.xlf | 15 + ...SystemSecurityCryptographyResources.es.xlf | 15 + ...SystemSecurityCryptographyResources.fr.xlf | 15 + ...SystemSecurityCryptographyResources.it.xlf | 15 + ...SystemSecurityCryptographyResources.ja.xlf | 15 + ...SystemSecurityCryptographyResources.ko.xlf | 15 + ...SystemSecurityCryptographyResources.pl.xlf | 15 + ...temSecurityCryptographyResources.pt-BR.xlf | 15 + ...SystemSecurityCryptographyResources.ru.xlf | 15 + ...SystemSecurityCryptographyResources.tr.xlf | 15 + ...mSecurityCryptographyResources.zh-Hans.xlf | 15 + ...mSecurityCryptographyResources.zh-Hant.xlf | 15 + .../DoNotHardCodeEncryptionKeyTests.cs | 273 ++++++++++++++++++ src/Utilities/Compiler/WellKnownTypeNames.cs | 2 + .../FlowAnalysis.Utilities.projitems | 2 + .../HardCodeEncryptionKeySinks.cs | 35 +++ .../HardCodeEncryptionKeySources.cs | 39 +++ .../PooledHashSetExtensions.cs | 7 +- .../Analysis/TaintedDataAnalysis/SinkKind.cs | 1 + .../TaintedDataAnalysis/SourceInfo.cs | 16 +- ...ataAnalysis.TaintedDataOperationVisitor.cs | 15 +- .../TaintedDataAnalysis/TaintedDataConfig.cs | 7 + .../TaintedDataSymbolMapExtensions.cs | 19 ++ 27 files changed, 653 insertions(+), 7 deletions(-) create mode 100644 src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs create mode 100644 src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs create mode 100644 src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs create mode 100644 src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs new file mode 100644 index 0000000000..4362fc8042 --- /dev/null +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Analyzer.Utilities; +using Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.NetCore.Analyzers.Security.Helpers; + +namespace Microsoft.NetCore.Analyzers.Security +{ + [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] + public class DoNotHardCodeEncryptionKey : SourceTriggeredTaintedDataAnalyzerBase + { + internal static DiagnosticDescriptor Rule = SecurityHelpers.CreateDiagnosticDescriptor( + "CA5390", + typeof(SystemSecurityCryptographyResources), + nameof(SystemSecurityCryptographyResources.DoNotHardCodeEncryptionKey), + nameof(SystemSecurityCryptographyResources.DoNotHardCodeEncryptionKeyMessage), + DiagnosticHelpers.EnabledByDefaultIfNotBuildingVSIX, + helpLinkUri: null, + descriptionResourceStringName: nameof(SystemSecurityCryptographyResources.DoNotHardCodeEncryptionKeyDescription), + customTags: WellKnownDiagnosticTagsExtensions.DataflowAndTelemetry); + + protected override SinkKind SinkKind { get { return SinkKind.HardCodeEncryptionKey; } } + + protected override DiagnosticDescriptor TaintedDataEnteringSinkDescriptor { get { return Rule; } } + } +} diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 491427cacd..01d5277e13 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -84,6 +84,18 @@ public override void Initialize(AnalysisContext context) }, OperationKind.Invocation); + operationBlockStartContext.RegisterOperationAction( + operationAnalysisContext => + { + var arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; + if (arrayInitializerOperation.ElementValues.All(s => s.ConstantValue.HasValue) && + sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) + { + rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + } + }, + OperationKind.ArrayInitializer); + operationBlockStartContext.RegisterOperationBlockEndAction( operationBlockAnalysisContext => { diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx b/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx index 86f2dd9994..d3f933be16 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx @@ -192,6 +192,15 @@ {0} disables TLS 1.2 and enables SSLv3 + + Do Not Hard Code Encryption Key + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf index b308abbc37..2970c16395 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf @@ -142,6 +142,21 @@ {0} zakazuje TLS 1.2 a povoluje SSLv3. + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Standardně je úložiště certifikátů důvěryhodných kořenových certifikačních autorit nakonfigurované na sadu veřejných CA, které splnily požadavky programu Microsoft Root Certificate Program. Vzhledem k tomu, že všechny důvěryhodné kořenové CA můžou vystavovat certifikáty pro libovolnou doménu, útočník si může vybrat slabou nebo zranitelnou CA, kterou si sami nainstalujete na cíl útoku – a jedna ohrožená, škodlivá nebo zranitelná CA ohrožuje zabezpečení celého systému. A co je horší, tyto útoky se dají vcelku snadno skrýt. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf index ab00d120a2..3883f36d54 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf @@ -142,6 +142,21 @@ {0} deaktiviert TLS 1.2 und aktiviert SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Standardmäßig ist der Zertifikatspeicher der vertrauenswürdigen Stammzertifizierungsstellen mit einem Satz öffentlicher Zertifizierungsstellen konfiguriert, die die Anforderungen des Microsoft-Programms für Stammzertifikate erfüllen. Da alle vertrauenswürdigen Stammzertifizierungsstellen Zertifikate für eine beliebige Domäne ausstellen können, kann ein Angreifer eine schwache oder zwingende Zertifizierungsstelle, die Sie selbst installieren, als Ziel für einen Angriff auswählen. Auf diese Weise kann eine einzelne unsichere, schädliche oder zwingende Zertifizierungsstelle die Sicherheit des gesamten Systems untergraben. Hinzu kommt, dass diese Angriffe leicht unbemerkt bleiben können. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf index 005483e247..00dbf17a24 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf @@ -142,6 +142,21 @@ {0} deshabilita TLS 1.2 y habilita SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. De forma predeterminada, el almacén de certificados de entidades de certificación raíz de confianza está configurado con un conjunto de entidades de certificación públicas que cumplen los requisitos del programa de certificados raíz de Microsoft. Dado que todas las entidades de certificación raíz de confianza pueden emitir certificados para cualquier dominio, un atacante puede elegir una entidad de certificación coaccionable o débil que instale por su cuenta para destinar un ataque, y una única entidad de certificación vulnerable, malintencionada o convertible socava la seguridad de todo el sistema. Para empeorar las cosas, estos ataques pueden pasar desapercibidos con bastante facilidad. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf index da9ac4e7f6..e48c571473 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf @@ -142,6 +142,21 @@ {0} désactive TLS 1.2 et active SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Par défaut, le magasin de certificats Autorités de certification racines de confiance est configuré avec un ensemble d'autorités de certification publiques qui répond aux exigences du programme de certificat racine Microsoft. Comme toutes les autorités de certification racines de confiance peuvent émettre des certificats pour n'importe quel domaine, un attaquant peut choisir une autorité de certification faible ou coercible que vous installez par vous-même, or une seule autorité de certification vulnérable, malveillante ou coercible compromet la sécurité de l'ensemble du système. Pour compliquer la situation, ces attaques passent facilement inaperçues. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf index 8b6bba2f97..7fc3b7679c 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf @@ -142,6 +142,21 @@ {0} disabilita TLS 1.2 e abilita SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Per impostazione predefinita, l'archivio certificati di Autorità di certificazione radice disponibile nell'elenco locale è configurato con un set di CA pubbliche che soddisfa i requisiti del programma Microsoft Root Certificate. Dal momento che tutte le CA radice attendibili possono rilasciare certificati per qualsiasi dominio, un utente malintenzionato può selezionare come destinazione di attacco una CA debole o coercibile installata dall'utente. Una CA vulnerabile, dannosa o coercibile compromette la sicurezza dell'intero sistema, senza contare che questi attacchi possono passare inosservati abbastanza facilmente. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf index 4ec206d694..c0b005dd61 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf @@ -142,6 +142,21 @@ {0} は、TLS 1.2 を無効にして、SSLv3 を有効にします + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. 既定では、信頼されたルート証明機関の証明書ストアは、Microsoft ルート証明書プログラムの要件を満たす一連の公的 CA で構成されています。すべての信頼されたルート CA は任意のドメインの証明書を発行できますが、攻撃者はユーザー自身がインストールする脆弱な CA または制御可能な CA を攻撃対象として選択できます。脆弱な CA、悪意のあるCA、制御可能な CA が 1 つでもあると、システム全体のセキュリティが弱体化します。さらに悪いことに、こうした攻撃はまったく気付かずに、しかも簡単に行うことができます。 diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf index 9634d7e0a2..2db50f5e74 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf @@ -142,6 +142,21 @@ {0}이(가) TLS 1.2를 사용하지 않도록, SSLv3를 사용하도록 설정합니다. + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. 기본적으로 신뢰할 수 있는 루트 인증 기관 인증서 저장소는 Microsoft 루트 인증서 프로그램의 요구 사항을 준수하는 공용 CA 세트로 구성됩니다. 모든 신뢰할 수 있는 루트 CA는 모든 도메인에 대한 인증서를 발급할 수 있으므로 공격자는 사용자가 직접 설치한 약하거나 강제할 수 있는 CA를 선택하여 공격 대상으로 지정할 수 있으며, 취약하거나 악의적이거나 강제할 수 있는 CA 하나가 전체 시스템의 보안을 약화시킵니다. 설상가상으로 이러한 공격자는 상당히 쉽게 눈에 띄지 않고 넘어갈 수 있습니다. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf index 117694c59b..e6cc695e6b 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf @@ -142,6 +142,21 @@ Metoda {0} wyłącza protokół TLS 1.2 i włącza protokół SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Domyślnie magazyn certyfikatów zaufanych głównych urzędów certyfikacji jest skonfigurowany przy użyciu zbioru publicznych urzędów certyfikacji, który spełnia wymagania programu certyfikatów głównych firmy Microsoft. Ponieważ wszystkie zaufane główne urzędy certyfikacji mogą wystawiać certyfikaty dla dowolnej domeny, osoba atakująca może wybrać słaby lub wymuszony urząd certyfikacji zainstalowany przez Ciebie, aby umożliwić atak — a jeden narażony na ataki, złośliwy lub wymuszony urząd certyfikacji osłabia bezpieczeństwo całego systemu. Ponadto tego typu ataki mogą nawet zostać niezauważone. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf index a200aa6c85..e136f28893 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf @@ -142,6 +142,21 @@ O {0} desabilita o TLS 1.2 e habilita o SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Por padrão, o repositório de certificados de Autoridades de Certificação Confiáveis é configurado com um conjunto de CAs públicas que atendem aos requisitos do Microsoft Root Certificate Program. Como todas as ACs raiz confiáveis podem emitir certificados para qualquer domínio, um invasor pode escolher uma AC fraca ou coercível que você instala sozinho para direcionar um ataque – e uma única AC vulnerável, mal-intencionada ou coercível enfraquece a segurança de todo o sistema. Para piorar as coisas, esses ataques podem facilmente passar despercebidos. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf index 22340c16a9..aa7bfe2ce3 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf @@ -142,6 +142,21 @@ {0} отключает TLS 1.2 и включает SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. По умолчанию в хранилище сертификатов Доверенных корневых центров сертификации настроен набор общедоступных центров сертификации, удовлетворяющих требованиям программы корневых сертификатов Майкрософт. Поскольку все доверенные корневые ЦС могут создавать сертификаты для любого домена, злоумышленник может выбрать слабозащищенный или обязательный центр сертификации, который вы устанавливали для отражения атаки. Всего один слабозащищенный, вредоносный или обязательный ЦС может скомпрометировать всю систему. Что еще хуже, эти атаки могут пройти незамеченными. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf index 87b5f0485c..a6d2b3193c 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf @@ -142,6 +142,21 @@ {0}, TLS 1.2'yi devre dışı bırakır ve SSLv3'ü etkinleştirir + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. Varsayılan olarak, Güvenilen Kök Sertifika Yetkilileri sertifika deposu, Microsoft Kök Sertifika Programı gereksinimlerini karşılayan bir genel CA kümesiyle yapılandırılır. Tüm güvenilen kök CA'lar herhangi bir etki alanı için sertifika verebileceği için, bir saldırgan kendiniz yüklediğiniz savunmasız veya zorlanabilir bir CA'yı saldırı için seçebilir ve tek bir zayıf, kötü amaçlı ya da zorlanabilir CA tüm sistemin güvenliğini tehlikeye atabilir. Daha da kötüsü, bu saldırıların gözden kaçırılması oldukça kolaydır. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf index 347b78e1f3..5a6876ad31 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf @@ -142,6 +142,21 @@ {0} 禁用 TLS 1.2 并且启用 SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. 默认情况下，“受信任的根证书颁发机构”证书存储配置有一组符合 Microsoft 根证书程序的公共 CA。由于所有受信任的根 CA 都可为任意域颁发证书，因此攻击者可能会选择你自行安装的某个安全性较弱或可强迫的 CA 进行攻击；一个易受攻击、恶意或可强迫的 CA 就会降低整个系统的安全性。更糟糕的是，这些攻击者可能很难被察觉。 diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf index d03ecc1694..075552cfa9 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf @@ -142,6 +142,21 @@ {0} 停用 TLS 1.2 並啟用 SSLv3 + + Do Not Hard Code Encryption Key + Do Not Hard Code Encryption Key + + + + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + + + + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + + By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. 根據預設，信任的根憑證授權單位的憑證存放區，設定有一組符合 Microsoft 根憑證計劃需求的公開 CA。因為所有信任的根 CA，都可以為任何所有發出憑證，所以攻擊者可以挑選一個您自行安裝的弱式 CA 或強迫式 CA，發動攻擊 – 而只要一個弱式、惡意或強迫式 CA，就會侵害整個系統的安全性。而且，這些攻擊還很可能輕易地就被忽略，而讓事情變得更糟。 diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs new file mode 100644 index 0000000000..c02347a52c --- /dev/null +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs @@ -0,0 +1,273 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.NetCore.Analyzers.Security.UnitTests +{ + public class DoNotHardCodeEncryptionKeyTests : TaintedDataAnalyzerTestBase + { + public DoNotHardCodeEncryptionKeyTests(ITestOutputHelper output) + : base(output) + { + } + + protected override DiagnosticDescriptor Rule => DoNotHardCodeEncryptionKey.Rule; + + [Fact] + public void Test_HardcodedInString_CreateEncryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + string someHardCodedBase64String = ""AAAAAaazaoensuth""; + byte[] key = Convert.FromBase64String(someHardCodedBase64String); + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(key, someOtherBytesForIV); + } +}", + GetCSharpResultAt(12, 9, 10, 22, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[] Convert.FromBase64String(string s)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInMultilinesString_CreateEncryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + string someHardCodedBase64String = ""sssdsdsdsdsdsds"" + + ""sdasdsadasddsda"" + + ""sdasdsadasddsda"" ; + byte[] key = Convert.FromBase64String(someHardCodedBase64String); + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(key, someOtherBytesForIV); + + } +}", + GetCSharpResultAt(14, 9, 12, 22, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[] Convert.FromBase64String(string s)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateEncryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(11, 9, 9, 36, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateDecryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateDecryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(11, 9, 9, 36, "ICryptoTransform SymmetricAlgorithm.CreateDecryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_KeyProperty_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.Key = rgbKey; + } +}", + GetCSharpResultAt(11, 9, 9, 36, "byte[] SymmetricAlgorithm.Key", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateEncryptorFromDerivedClassOfSymmetricAlgorithm_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + Aes aes = Aes.Create(); + aes.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(11, 9, 9, 36, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateEncryptor_Multivalues_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + Random r = new Random(); + + if (r.Next(6) == 4) + { + rgbKey = new byte[] {4, 5, 6}; + } + + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(18, 9, 14, 33, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)"), + GetCSharpResultAt(18, 9, 9, 36, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateEncryptor_WithoutAssignment_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(new byte[] {1, 2, 3}, someOtherBytesForIV); + } +}", + GetCSharpResultAt(10, 9, 10, 41, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_MaybeHardcoded_CreateEncryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey) + { + Random r = new Random(); + + if (r.Next(6) == 4) + { + rgbKey = new byte[] {4, 5, 6}; + } + + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(17, 9, 13, 33, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey)")); + } + + [Fact] + public void Test_NotHardcoded_CreateEncryptor_NoDiagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey) + { + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}"); + } + + [Fact] + public void Test_HardcodedInbyteArray_CreateEncryptor_NoDiagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte b = 1; + byte[] rgbKey = new byte[] {b}; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}"); + } + + [Fact] + public void Test_HardcodedInArrayThenOverwrite_NoDiagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV, byte[] key) + { + byte[] rgbKey = new byte[] {1, 2, 3}; + rgbKey = key; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}"); + } + + protected override DiagnosticAnalyzer GetBasicDiagnosticAnalyzer() + { + return new DoNotHardCodeEncryptionKey(); + } + + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() + { + return new DoNotHardCodeEncryptionKey(); + } + } +} diff --git a/src/Utilities/Compiler/WellKnownTypeNames.cs b/src/Utilities/Compiler/WellKnownTypeNames.cs index 2653d044a3..752fe89e14 100644 --- a/src/Utilities/Compiler/WellKnownTypeNames.cs +++ b/src/Utilities/Compiler/WellKnownTypeNames.cs @@ -317,5 +317,7 @@ internal static class WellKnownTypeNames public const string SystemIOFileStream = "System.IO.FileStream"; public const string SystemIOPath = "System.IO.Path"; public const string SystemString = "System.String"; + public const string SystemConvert = "System.Convert"; + public const string SystemSecurityCryptographySymmetricAlgorithm = "System.Security.Cryptography.SymmetricAlgorithm"; } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems b/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems index fd2cce4851..f504ee231e 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems +++ b/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems @@ -137,6 +137,8 @@ + + diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs new file mode 100644 index 0000000000..2bf8724a91 --- /dev/null +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using Analyzer.Utilities.PooledObjects; + +namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis +{ + internal static class HardCodeEncryptionKeySinks + { + /// + /// s for tainted data process symmetric algorithm sinks. + /// + public static ImmutableHashSet SinkInfos { get; } + + static HardCodeEncryptionKeySinks() + { + var builder = PooledHashSet.GetInstance(); + + builder.AddSinkInfo( + WellKnownTypeNames.SystemSecurityCryptographySymmetricAlgorithm, + SinkKind.HardCodeEncryptionKey, + isInterface: false, + isAnyStringParameterInConstructorASink: false, + sinkProperties: new[] { + "Key", + }, + sinkMethodParameters: new[] { + ( "CreateEncryptor", new[] { "rgbKey" }), + ( "CreateDecryptor", new[] { "rgbKey" }), + }); + + SinkInfos = builder.ToImmutableAndFree(); + } + } +} diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs new file mode 100644 index 0000000000..abce198ffe --- /dev/null +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using Analyzer.Utilities.PooledObjects; + +namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis +{ + internal static class HardCodeEncryptionKeySources + { + /// + /// s for hardcoded key tainted data sources. + /// + public static ImmutableHashSet SourceInfos { get; } + + /// + /// Statically constructs. + /// + static HardCodeEncryptionKeySources() + { + var builder = PooledHashSet.GetInstance(); + + builder.AddSourceInfo( + WellKnownTypeNames.SystemConvert, + isInterface: false, + taintedProperties: null, + taintedMethods: new[] { + "FromBase64String", + }); + builder.AddSourceInfo( + WellKnownTypeNames.SystemByte, + isInterface: false, + taintedProperties: null, + taintedMethods: null, + taintArray: true); + + SourceInfos = builder.ToImmutableAndFree(); + } + } +} diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs index 91304e5da8..7154ce9653 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; +using System.Threading; using Analyzer.Utilities.PooledObjects; using Microsoft.CodeAnalysis; @@ -61,7 +62,8 @@ public static void AddSourceInfo( string fullTypeName, bool isInterface, string[] taintedProperties, - string[] taintedMethods) + string[] taintedMethods, + bool taintArray = false) { SourceInfo metadata = new SourceInfo( fullTypeName, @@ -70,7 +72,8 @@ public static void AddSourceInfo( ?? ImmutableHashSet.Empty, taintedMethods: taintedMethods?.ToImmutableHashSet(StringComparer.Ordinal) - ?? ImmutableHashSet.Empty); + ?? ImmutableHashSet.Empty, + taintArray: taintArray); builder.Add(metadata); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs index 23130017be..7eee58d7df 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs @@ -17,5 +17,6 @@ public enum SinkKind Xml, Xaml, ZipSlip, + HardCodeEncryptionKey, } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs index f475057985..bb7e45ff7f 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs @@ -17,12 +17,13 @@ internal class SourceInfo : ITaintedDataInfo, IEquatable /// /// Properties that generate tainted data. /// Methods that generate tainted data. - public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet taintedProperties, ImmutableHashSet taintedMethods) + public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet taintedProperties, ImmutableHashSet taintedMethods, bool taintArray) { FullTypeName = fullTypeName ?? throw new ArgumentNullException(nameof(fullTypeName)); IsInterface = isInterface; TaintedProperties = taintedProperties ?? throw new ArgumentNullException(nameof(taintedProperties)); TaintedMethods = taintedMethods ?? throw new ArgumentNullException(nameof(taintedMethods)); + TaintArray = taintArray; } /// @@ -45,12 +46,18 @@ public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet public ImmutableHashSet TaintedMethods { get; } + /// + /// Indicates array of this type generates tainted data. + /// + public bool TaintArray { get; } + public override int GetHashCode() { - return HashUtilities.Combine(this.TaintedProperties, + return HashUtilities.Combine(this.TaintArray.GetHashCode(), + HashUtilities.Combine(this.TaintedProperties, HashUtilities.Combine(this.TaintedMethods, HashUtilities.Combine(this.IsInterface.GetHashCode(), - StringComparer.Ordinal.GetHashCode(this.FullTypeName)))); + StringComparer.Ordinal.GetHashCode(this.FullTypeName))))); } public override bool Equals(object obj) @@ -64,7 +71,8 @@ public bool Equals(SourceInfo other) && this.FullTypeName == other.FullTypeName && this.IsInterface == other.IsInterface && this.TaintedProperties == other.TaintedProperties - && this.TaintedMethods == other.TaintedMethods; + && this.TaintedMethods == other.TaintedMethods + && this.TaintArray == other.TaintArray; } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 631920069c..1b1b6f2723 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -297,9 +297,22 @@ public override TaintedDataAbstractValue VisitArrayInitializer(IArrayInitializer taintedAbstractValues = taintedAbstractValues.Concat(baseAbstractValue); } + TaintedDataAbstractValue result = null; if (taintedAbstractValues.Any()) { - return TaintedDataAbstractValue.MergeTainted(taintedAbstractValues); + result = TaintedDataAbstractValue.MergeTainted(taintedAbstractValues); + } + + if (this.DataFlowAnalysisContext.SourceInfos.IsSourceArray(operation.Parent.Type as IArrayTypeSymbol) + && operation.ElementValues.All(s => s.ConstantValue.HasValue)) + { + var taintedDataAbstractValue = TaintedDataAbstractValue.CreateTainted(operation.Parent.Type, operation.Syntax, this.OwningSymbol); + result = result == null ? taintedDataAbstractValue : TaintedDataAbstractValue.MergeTainted(result, taintedDataAbstractValue); + } + + if (result != null) + { + return result; } else { diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs index 516cb312f7..3035e6fd82 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs @@ -197,6 +197,9 @@ private static ImmutableHashSet GetSourceInfos(SinkKind sinkKind) case SinkKind.ZipSlip: return ZipSlipSources.SourceInfos; + case SinkKind.HardCodeEncryptionKey: + return HardCodeEncryptionKeySources.SourceInfos; + default: Debug.Fail($"Unhandled SinkKind {sinkKind}"); return ImmutableHashSet.Empty; @@ -227,6 +230,7 @@ private static ImmutableHashSet GetSanitizerInfos(SinkKind sinkKi case SinkKind.Regex: case SinkKind.Redirect: case SinkKind.Xaml: + case SinkKind.HardCodeEncryptionKey: return ImmutableHashSet.Empty; case SinkKind.ZipSlip: @@ -279,6 +283,9 @@ private static ImmutableHashSet GetSinkInfos(SinkKind sinkKind) case SinkKind.ZipSlip: return ZipSlipSinks.SinkInfos; + case SinkKind.HardCodeEncryptionKey: + return HardCodeEncryptionKeySinks.SinkInfos; + default: Debug.Fail($"Unhandled SinkKind {sinkKind}"); return ImmutableHashSet.Empty; diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs index c618c0cd56..b2855fa83c 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs @@ -43,5 +43,24 @@ public static bool IsSourceProperty(this TaintedDataSymbolMap source return false; } + + /// + /// Determines if the given array is a tainted data source. + /// + /// + /// + /// + public static bool IsSourceArray(this TaintedDataSymbolMap sourceSymbolMap, IArrayTypeSymbol arrayTypeSymbol) + { + foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(arrayTypeSymbol.ElementType as INamedTypeSymbol)) + { + if (sourceInfo.TaintArray) + { + return true; + } + } + + return false; + } } } From 5ecea9ea16e4aa5c54ab3483a9fc997eaff7516a Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Tue, 2 Jul 2019 16:33:06 +0800 Subject: [PATCH 02/12] Modify according to the comments and add a argument check feature for SourceInfo. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 5 +- .../SystemSecurityCryptographyResources.resx | 4 +- ...SystemSecurityCryptographyResources.cs.xlf | 8 +-- ...SystemSecurityCryptographyResources.de.xlf | 8 +-- ...SystemSecurityCryptographyResources.es.xlf | 8 +-- ...SystemSecurityCryptographyResources.fr.xlf | 8 +-- ...SystemSecurityCryptographyResources.it.xlf | 8 +-- ...SystemSecurityCryptographyResources.ja.xlf | 8 +-- ...SystemSecurityCryptographyResources.ko.xlf | 8 +-- ...SystemSecurityCryptographyResources.pl.xlf | 8 +-- ...temSecurityCryptographyResources.pt-BR.xlf | 8 +-- ...SystemSecurityCryptographyResources.ru.xlf | 8 +-- ...SystemSecurityCryptographyResources.tr.xlf | 8 +-- ...mSecurityCryptographyResources.zh-Hans.xlf | 8 +-- ...mSecurityCryptographyResources.zh-Hant.xlf | 8 +-- .../DoNotHardCodeEncryptionKeyTests.cs | 59 +++++++++++++++---- .../HardCodeEncryptionKeySources.cs | 12 ++-- .../PooledHashSetExtensions.cs | 39 ++++++++++-- .../TaintedDataAnalysis/SourceInfo.cs | 18 +++--- ...ataAnalysis.TaintedDataOperationVisitor.cs | 7 ++- .../TaintedDataAnalysis.cs | 18 ++++++ .../TaintedDataAnalysisContext.cs | 7 ++- .../TaintedDataSymbolMapExtensions.cs | 38 +++++++++++- 23 files changed, 220 insertions(+), 91 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 01d5277e13..04ff9df989 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -87,9 +87,8 @@ public override void Initialize(AnalysisContext context) operationBlockStartContext.RegisterOperationAction( operationAnalysisContext => { - var arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; - if (arrayInitializerOperation.ElementValues.All(s => s.ConstantValue.HasValue) && - sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) + IArrayInitializerOperation arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; + if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) { rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); } diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx b/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx index bcbd697040..7d26ec4048 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx @@ -196,10 +196,10 @@ Do Not Hard Code Encryption Key - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' By default, the Trusted Root Certification Authorities certificate store is configured with a set of public CAs that has met the requirements of the Microsoft Root Certificate Program. Since all trusted root CAs can issue certificates for any domain, an attacker can pick a weak or coercible CA that you install by yourself to target for an attack – and a single vulnerable, malicious or coercible CA undermines the security of the entire system. To make matters worse, these attacks can go unnoticed quite easily. diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf index 056c64d03e..a4b640d23a 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.cs.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf index 8ca40cc71f..168e34925a 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.de.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf index f85efa2260..bbf7cadb58 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.es.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf index 0a31501d6a..bab9dc5f5d 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.fr.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf index dc1f0eccf7..d4a41bf7fc 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.it.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf index ac587fc721..f54e1976c5 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ja.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf index ab37ccc6c9..5c65d65c03 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ko.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf index 8f060bc0d9..8edd931551 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pl.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf index c337e50962..fa7e698ed5 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.pt-BR.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf index ebff146f27..b4a9121538 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.ru.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf index e6afc7e207..40ba721cb9 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.tr.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf index 3a6357fbfb..96ebf75e56 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hans.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf index edf0d44df1..6df51b973b 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/xlf/SystemSecurityCryptographyResources.zh-Hant.xlf @@ -158,13 +158,13 @@ - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. - The .Key property or rgbKey parameter of a SymmetricAlgorithm should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. + SymmetricAlgorithm's .Key property, or a method's rgbKey parameter, should never be a hardcoded value. - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' - Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hard-coded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' + Potential security vulnerability was found where '{0}' in method '{1}' may be tainted by hardcoded key from '{2}' in method '{3}' diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs index c02347a52c..a3b4b79cfa 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs @@ -17,7 +17,26 @@ public DoNotHardCodeEncryptionKeyTests(ITestOutputHelper output) protected override DiagnosticDescriptor Rule => DoNotHardCodeEncryptionKey.Rule; [Fact] - public void Test_HardcodedInString_CreateEncryptor_Diagnostic() + public void Test_HardcodedInString_CreateEncryptor_NeedValueContentAnalysis_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] key = Convert.FromBase64String(""AAAAAaazaoensuth""); + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(key, someOtherBytesForIV); + } +}", + GetCSharpResultAt(11, 9, 9, 22, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[] Convert.FromBase64String(string s)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_HardcodedInStringWithVariable_CreateEncryptor_Diagnostic() { VerifyCSharpWithDependencies(@" using System; @@ -97,6 +116,26 @@ public void TestMethod(byte[] someOtherBytesForIV) GetCSharpResultAt(11, 9, 9, 36, "ICryptoTransform SymmetricAlgorithm.CreateDecryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); } + [Fact] + public void Test_HardcodedInbyteArrayWithVariable_CreateEncryptor_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte b = 1; + byte[] rgbKey = new byte[] {b}; + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + } +}", + GetCSharpResultAt(12, 9, 10, 36, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + [Fact] public void Test_HardcodedInbyteArray_KeyProperty_Diagnostic() { @@ -223,7 +262,7 @@ public void TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey) } [Fact] - public void Test_HardcodedInbyteArray_CreateEncryptor_NoDiagnostic() + public void Test_HardcodedInArrayThenOverwrite_NoDiagnostic() { VerifyCSharpWithDependencies(@" using System; @@ -231,10 +270,10 @@ public void Test_HardcodedInbyteArray_CreateEncryptor_NoDiagnostic() class TestClass { - public void TestMethod(byte[] someOtherBytesForIV) + public void TestMethod(byte[] someOtherBytesForIV, byte[] key) { - byte b = 1; - byte[] rgbKey = new byte[] {b}; + byte[] rgbKey = new byte[] {1, 2, 3}; + rgbKey = key; SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); } @@ -242,7 +281,7 @@ public void TestMethod(byte[] someOtherBytesForIV) } [Fact] - public void Test_HardcodedInArrayThenOverwrite_NoDiagnostic() + public void Test_NotHardcodedInString_CreateEncryptor_NoDiagnostic() { VerifyCSharpWithDependencies(@" using System; @@ -250,12 +289,12 @@ public void Test_HardcodedInArrayThenOverwrite_NoDiagnostic() class TestClass { - public void TestMethod(byte[] someOtherBytesForIV, byte[] key) + public void TestMethod(byte[] someOtherBytesForIV) { - byte[] rgbKey = new byte[] {1, 2, 3}; - rgbKey = key; + byte[] key = Convert.FromBase64String(Console.ReadLine()); SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); - rijn.CreateEncryptor(rgbKey, someOtherBytesForIV); + rijn.CreateEncryptor(key, someOtherBytesForIV); + } }"); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs index abce198ffe..a57807ef66 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs @@ -2,6 +2,7 @@ using System.Collections.Immutable; using Analyzer.Utilities.PooledObjects; +using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -19,19 +20,22 @@ static HardCodeEncryptionKeySources() { var builder = PooledHashSet.GetInstance(); - builder.AddSourceInfo( + builder.AddSourceInfoWithArgumentChecks( WellKnownTypeNames.SystemConvert, isInterface: false, taintedProperties: null, - taintedMethods: new[] { - "FromBase64String", + taintedMethods: new (string, (string, ArgumentCheck)[])[] { + ("FromBase64String", + new (string, ArgumentCheck)[] { + ("s", (ImmutableHashSet potentialArgumentValues) => !potentialArgumentValues.IsEmpty), + }), }); builder.AddSourceInfo( WellKnownTypeNames.SystemByte, isInterface: false, taintedProperties: null, taintedMethods: null, - taintArray: true); + taintConstantArray: true); SourceInfos = builder.ToImmutableAndFree(); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs index 24ec30cd74..87c268763a 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs @@ -4,9 +4,8 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; -using System.Threading; using Analyzer.Utilities.PooledObjects; -using Microsoft.CodeAnalysis; +using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -63,7 +62,7 @@ public static void AddSourceInfo( bool isInterface, string[] taintedProperties, string[] taintedMethods, - bool taintArray = false) + bool taintConstantArray = false) { SourceInfo metadata = new SourceInfo( fullTypeName, @@ -71,9 +70,39 @@ public static void AddSourceInfo( taintedProperties: taintedProperties?.ToImmutableHashSet(StringComparer.Ordinal) ?? ImmutableHashSet.Empty, taintedMethods: - taintedMethods?.ToImmutableHashSet(StringComparer.Ordinal) + taintedMethods + ?.Select(o => new KeyValuePair>(o, ImmutableDictionary.Empty)) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary>.Empty, + taintConstantArray: taintConstantArray); + builder.Add(metadata); + } + + // Just to make hardcoding SourceInfos more convenient. + public static void AddSourceInfoWithArgumentChecks( + this PooledHashSet builder, + string fullTypeName, + bool isInterface, + string[] taintedProperties, + IEnumerable<(string Method, (string parameterName, ArgumentCheck argumentCheck)[] ParameterNameAndConditionChecks)> taintedMethods, + bool taintConstantArray = false) + { + SourceInfo metadata = new SourceInfo( + fullTypeName, + isInterface: isInterface, + taintedProperties: taintedProperties?.ToImmutableHashSet(StringComparer.Ordinal) ?? ImmutableHashSet.Empty, - taintArray: taintArray); + taintedMethods: + taintedMethods + ?.Select(o => new KeyValuePair>( + o.Method, + o.ParameterNameAndConditionChecks + ?.Select(o => new KeyValuePair(o.parameterName, o.argumentCheck)) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary.Empty)) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary>.Empty, + taintConstantArray: taintConstantArray); builder.Add(metadata); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs index bb7e45ff7f..d83b67e553 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs @@ -14,16 +14,16 @@ internal class SourceInfo : ITaintedDataInfo, IEquatable /// Constructs. /// /// Full type name of the...type (namespace + type). - /// /// Properties that generate tainted data. /// Methods that generate tainted data. - public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet taintedProperties, ImmutableHashSet taintedMethods, bool taintArray) + /// + public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet taintedProperties, ImmutableDictionary> taintedMethods, bool taintConstantArray) { FullTypeName = fullTypeName ?? throw new ArgumentNullException(nameof(fullTypeName)); IsInterface = isInterface; TaintedProperties = taintedProperties ?? throw new ArgumentNullException(nameof(taintedProperties)); TaintedMethods = taintedMethods ?? throw new ArgumentNullException(nameof(taintedMethods)); - TaintArray = taintArray; + TaintConstantArray = taintConstantArray; } /// @@ -41,19 +41,21 @@ public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet public ImmutableHashSet TaintedProperties { get; } + public delegate bool ArgumentCheck(ImmutableHashSet argumentValue); + /// /// Methods that generate tainted data. /// - public ImmutableHashSet TaintedMethods { get; } + public ImmutableDictionary> TaintedMethods { get; } /// - /// Indicates array of this type generates tainted data. + /// Indicates arrays initialized with constant values of this type generates tainted data. /// - public bool TaintArray { get; } + public bool TaintConstantArray { get; } public override int GetHashCode() { - return HashUtilities.Combine(this.TaintArray.GetHashCode(), + return HashUtilities.Combine(this.TaintConstantArray.GetHashCode(), HashUtilities.Combine(this.TaintedProperties, HashUtilities.Combine(this.TaintedMethods, HashUtilities.Combine(this.IsInterface.GetHashCode(), @@ -72,7 +74,7 @@ public bool Equals(SourceInfo other) && this.IsInterface == other.IsInterface && this.TaintedProperties == other.TaintedProperties && this.TaintedMethods == other.TaintedMethods - && this.TaintArray == other.TaintArray; + && this.TaintConstantArray == other.TaintConstantArray; } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 8c6b0f185a..aaa8a33d7e 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -224,11 +224,12 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo ProcessTaintedDataEnteringInvocationOrCreation(method, taintedArguments, originalOperation); } + IEnumerable<(string, ImmutableHashSet)> literalValueOfArguments = (originalOperation as IInvocationOperation).Arguments.Select(o => (o.Parameter.Name, GetValueContentAbstractValue(o.Value).LiteralValues)); if (this.IsSanitizingMethod(method)) { return TaintedDataAbstractValue.NotTainted; } - else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod(method)) + else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod(method, literalValueOfArguments)) { return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } @@ -313,9 +314,9 @@ public override TaintedDataAbstractValue VisitArrayInitializer(IArrayInitializer } if (this.DataFlowAnalysisContext.SourceInfos.IsSourceArray(operation.Parent.Type as IArrayTypeSymbol) - && operation.ElementValues.All(s => s.ConstantValue.HasValue)) + && operation.ElementValues.All(s => GetValueContentAbstractValue(s).IsLiteralState)) { - var taintedDataAbstractValue = TaintedDataAbstractValue.CreateTainted(operation.Parent.Type, operation.Syntax, this.OwningSymbol); + TaintedDataAbstractValue taintedDataAbstractValue = TaintedDataAbstractValue.CreateTainted(operation.Parent.Type, operation.Syntax, this.OwningSymbol); result = result == null ? taintedDataAbstractValue : TaintedDataAbstractValue.MergeTainted(result, taintedDataAbstractValue); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs index 1f120995ce..717bb1c0a3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs @@ -6,9 +6,12 @@ using Microsoft.CodeAnalysis.FlowAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { + using ValueContentAnalysisResult = DataFlowAnalysisResult; + internal partial class TaintedDataAnalysis : ForwardDataFlowAnalysis { private static readonly TaintedDataAnalysisDomain TaintedDataAnalysisDomainInstance = new TaintedDataAnalysisDomain(CoreTaintedDataAnalysisDataDomain.Instance); @@ -58,6 +61,20 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( return null; } + ValueContentAnalysisResult valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( + cfg, + containingMethod, + wellKnownTypeProvider, + interproceduralAnalysisConfig, + out var copyAnalysisResult, + out pointsToAnalysisResult, + pessimisticAnalysis: true, + performCopyAnalysis: false); + if (valueContentAnalysisResultOpt == null) + { + return null; + } + TaintedDataAnalysisContext analysisContext = TaintedDataAnalysisContext.Create( TaintedDataAbstractValueDomain.Default, wellKnownTypeProvider, @@ -66,6 +83,7 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( interproceduralAnalysisConfig, pessimisticAnalysis: false, pointsToAnalysisResult: pointsToAnalysisResult, + valueContentAnalysisResultOpt: valueContentAnalysisResultOpt, tryGetOrComputeAnalysisResult: TryGetOrComputeResultForAnalysisContext, taintedSourceInfos: taintedSourceInfos, taintedSanitizerInfos: taintedSanitizerInfos, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs index aa240d5418..a37708c186 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs @@ -15,6 +15,7 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { using InterproceduralTaintedDataAnalysisData = InterproceduralAnalysisData; + using ValueContentAnalysisResult = DataFlowAnalysisResult; internal sealed class TaintedDataAnalysisContext : AbstractDataFlowAnalysisContext { @@ -26,6 +27,7 @@ private TaintedDataAnalysisContext( InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, bool pessimisticAnalysis, PointsToAnalysisResult pointsToAnalysisResult, + ValueContentAnalysisResult valueContentAnalysisResultOpt, Func tryGetOrComputeAnalysisResult, ControlFlowGraph parentControlFlowGraph, InterproceduralTaintedDataAnalysisData interproceduralAnalysisDataOpt, @@ -43,7 +45,7 @@ private TaintedDataAnalysisContext( exceptionPathsAnalysis: false, copyAnalysisResultOpt: null, pointsToAnalysisResult, - valueContentAnalysisResultOpt: null, + valueContentAnalysisResultOpt: valueContentAnalysisResultOpt, tryGetOrComputeAnalysisResult, parentControlFlowGraph, interproceduralAnalysisDataOpt, @@ -64,6 +66,7 @@ public static TaintedDataAnalysisContext Create( InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, bool pessimisticAnalysis, PointsToAnalysisResult pointsToAnalysisResult, + ValueContentAnalysisResult valueContentAnalysisResultOpt, Func tryGetOrComputeAnalysisResult, TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, @@ -79,6 +82,7 @@ public static TaintedDataAnalysisContext Create( interproceduralAnalysisConfig, pessimisticAnalysis, pointsToAnalysisResult, + valueContentAnalysisResultOpt, tryGetOrComputeAnalysisResult, parentControlFlowGraph: null, interproceduralAnalysisDataOpt: null, @@ -107,6 +111,7 @@ public override TaintedDataAnalysisContext ForkForInterproceduralAnalysis( this.InterproceduralAnalysisConfiguration, this.PessimisticAnalysis, pointsToAnalysisResultOpt, + valueContentAnalysisResultOpt, this.TryGetOrComputeAnalysisResult, this.ControlFlowGraph, interproceduralAnalysisData, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs index b2855fa83c..82203c2a28 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs @@ -1,13 +1,17 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; using Microsoft.CodeAnalysis; +using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { internal static class TaintedDataSymbolMapExtensions { /// - /// Determines if the given method is a tainted data source. + /// Determines if the given method is a potential tainted data source. /// /// /// @@ -16,7 +20,7 @@ public static bool IsSourceMethod(this TaintedDataSymbolMap sourceSy { foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType)) { - if (sourceInfo.TaintedMethods.Contains(method.MetadataName)) + if (sourceInfo.TaintedMethods.Keys.Contains(method.MetadataName)) { return true; } @@ -25,6 +29,34 @@ public static bool IsSourceMethod(this TaintedDataSymbolMap sourceSy return false; } + /// + /// Determines if the given method is a tainted data source and all the arguments meet the requirement. + /// + /// + /// + /// + /// + public static bool IsSourceMethod(this TaintedDataSymbolMap sourceSymbolMap, IMethodSymbol method, IEnumerable<(string parameterName, ImmutableHashSet argumentValues)> argumentValues) + { + foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType)) + { + if (sourceInfo.TaintedMethods.TryGetValue(method.MetadataName, out var argumentChecks)) + { + foreach (KeyValuePair kvp in argumentChecks) + { + if (!kvp.Value(argumentValues.FirstOrDefault(o => o.parameterName == kvp.Key).argumentValues)) + { + return false; + } + } + + return true; + } + } + + return false; + } + /// /// Determines if the given property is a tainted data source. /// @@ -54,7 +86,7 @@ public static bool IsSourceArray(this TaintedDataSymbolMap sourceSym { foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(arrayTypeSymbol.ElementType as INamedTypeSymbol)) { - if (sourceInfo.TaintArray) + if (sourceInfo.TaintConstantArray) { return true; } From fe23db55717cabde64c108637bcc2f18b590d200 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Wed, 3 Jul 2019 11:09:22 +0800 Subject: [PATCH 03/12] Remove two Debug.Assert(). --- .../TaintedDataAnalysis.cs | 22 +++++-------------- .../TaintedDataAnalysisContext.cs | 18 ++++++++------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs index 717bb1c0a3..a09fc41ce0 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs @@ -48,29 +48,16 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( InterproceduralAnalysisConfiguration interproceduralAnalysisConfig) { WellKnownTypeProvider wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(compilation); - PointsToAnalysisResult pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( - cfg, - containingMethod, - wellKnownTypeProvider, - interproceduralAnalysisConfig, - interproceduralAnalysisPredicateOpt: null, - pessimisticAnalysis: true, - performCopyAnalysis: false); - if (pointsToAnalysisResult == null) - { - return null; - } - - ValueContentAnalysisResult valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( + ValueContentAnalysisResult valueContentAnalysisResult = ValueContentAnalysis.TryGetOrComputeResult( cfg, containingMethod, wellKnownTypeProvider, interproceduralAnalysisConfig, out var copyAnalysisResult, - out pointsToAnalysisResult, + out var pointsToAnalysisResult, pessimisticAnalysis: true, performCopyAnalysis: false); - if (valueContentAnalysisResultOpt == null) + if (valueContentAnalysisResult == null) { return null; } @@ -82,8 +69,9 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( containingMethod, interproceduralAnalysisConfig, pessimisticAnalysis: false, + copyAnalysisResultOpt: copyAnalysisResult, pointsToAnalysisResult: pointsToAnalysisResult, - valueContentAnalysisResultOpt: valueContentAnalysisResultOpt, + valueContentAnalysisResult: valueContentAnalysisResult, tryGetOrComputeAnalysisResult: TryGetOrComputeResultForAnalysisContext, taintedSourceInfos: taintedSourceInfos, taintedSanitizerInfos: taintedSanitizerInfos, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs index a37708c186..f3b5542cf9 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisContext.cs @@ -15,6 +15,7 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { using InterproceduralTaintedDataAnalysisData = InterproceduralAnalysisData; + using CopyAnalysisResult = DataFlowAnalysisResult; using ValueContentAnalysisResult = DataFlowAnalysisResult; internal sealed class TaintedDataAnalysisContext : AbstractDataFlowAnalysisContext @@ -26,8 +27,9 @@ private TaintedDataAnalysisContext( ISymbol owningSymbol, InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, bool pessimisticAnalysis, + CopyAnalysisResult copyAnalysisResultOpt, PointsToAnalysisResult pointsToAnalysisResult, - ValueContentAnalysisResult valueContentAnalysisResultOpt, + ValueContentAnalysisResult valueContentAnalysisResult, Func tryGetOrComputeAnalysisResult, ControlFlowGraph parentControlFlowGraph, InterproceduralTaintedDataAnalysisData interproceduralAnalysisDataOpt, @@ -43,9 +45,9 @@ private TaintedDataAnalysisContext( pessimisticAnalysis, predicateAnalysis: false, exceptionPathsAnalysis: false, - copyAnalysisResultOpt: null, + copyAnalysisResultOpt, pointsToAnalysisResult, - valueContentAnalysisResultOpt: valueContentAnalysisResultOpt, + valueContentAnalysisResult, tryGetOrComputeAnalysisResult, parentControlFlowGraph, interproceduralAnalysisDataOpt, @@ -65,8 +67,9 @@ public static TaintedDataAnalysisContext Create( ISymbol owningSymbol, InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, bool pessimisticAnalysis, + CopyAnalysisResult copyAnalysisResultOpt, PointsToAnalysisResult pointsToAnalysisResult, - ValueContentAnalysisResult valueContentAnalysisResultOpt, + ValueContentAnalysisResult valueContentAnalysisResult, Func tryGetOrComputeAnalysisResult, TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, @@ -81,8 +84,9 @@ public static TaintedDataAnalysisContext Create( owningSymbol, interproceduralAnalysisConfig, pessimisticAnalysis, + copyAnalysisResultOpt, pointsToAnalysisResult, - valueContentAnalysisResultOpt, + valueContentAnalysisResult, tryGetOrComputeAnalysisResult, parentControlFlowGraph: null, interproceduralAnalysisDataOpt: null, @@ -100,9 +104,6 @@ public override TaintedDataAnalysisContext ForkForInterproceduralAnalysis( DataFlowAnalysisResult valueContentAnalysisResultOpt, InterproceduralTaintedDataAnalysisData interproceduralAnalysisData) { - Debug.Assert(copyAnalysisResultOpt == null); // Just because we're not passing this argument along. - Debug.Assert(valueContentAnalysisResultOpt == null); - return new TaintedDataAnalysisContext( this.ValueDomain, this.WellKnownTypeProvider, @@ -110,6 +111,7 @@ public override TaintedDataAnalysisContext ForkForInterproceduralAnalysis( invokedMethod, this.InterproceduralAnalysisConfiguration, this.PessimisticAnalysis, + copyAnalysisResultOpt, pointsToAnalysisResultOpt, valueContentAnalysisResultOpt, this.TryGetOrComputeAnalysisResult, From 692efd357cb285fdeac0367d7b43b578f5acb230 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Wed, 3 Jul 2019 11:31:43 +0800 Subject: [PATCH 04/12] Regiter ArrayInitializer operation action only when there is a least one SourceInfo whose TaintConstantArray is true. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 21 +++++++++++-------- .../TaintedDataAnalysis/TaintedDataConfig.cs | 5 +++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 04ff9df989..e35616828c 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -84,16 +84,19 @@ public override void Initialize(AnalysisContext context) }, OperationKind.Invocation); - operationBlockStartContext.RegisterOperationAction( - operationAnalysisContext => - { - IArrayInitializerOperation arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; - if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) + if (taintedDataConfig.HasTaintArraySource(SinkKind)) + { + operationBlockStartContext.RegisterOperationAction( + operationAnalysisContext => { - rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); - } - }, - OperationKind.ArrayInitializer); + IArrayInitializerOperation arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; + if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) + { + rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + } + }, + OperationKind.ArrayInitializer); + } operationBlockStartContext.RegisterOperationBlockEndAction( operationBlockAnalysisContext => diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs index 3035e6fd82..86dade6007 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs @@ -160,6 +160,11 @@ public TaintedDataSymbolMap GetSinkSymbolMap(SinkKind sinkKind) return this.GetFromMap(sinkKind, this.SinkSymbolMap); } + public bool HasTaintArraySource(SinkKind sinkKind) + { + return GetSourceInfos(sinkKind).Any(o => o.TaintConstantArray); + } + private TaintedDataSymbolMap GetFromMap(SinkKind sinkKind, Dictionary>> map) where T : ITaintedDataInfo { From 055c8b18a5614437c5b1dbd241e0f3bc93dea0c1 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Thu, 4 Jul 2019 17:15:45 +0800 Subject: [PATCH 05/12] Change the form of argument check of tainted methods. --- .../Security/DoNotHardCodeEncryptionKey.cs | 2 +- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 83 ++++++++++++++++--- .../FlowAnalysis.Utilities.projitems | 4 +- ...inks.cs => HardcodedEncryptionKeySinks.cs} | 6 +- ...es.cs => HardcodedEncryptionKeySources.cs} | 22 +++-- .../PooledHashSetExtensions.cs | 42 +++++----- .../Analysis/TaintedDataAnalysis/SinkKind.cs | 2 +- .../TaintedDataAnalysis/SourceInfo.cs | 37 +++++++-- ...ataAnalysis.TaintedDataOperationVisitor.cs | 31 ++++++- .../TaintedDataAnalysis.cs | 53 ++++++++---- .../TaintedDataAnalysis/TaintedDataConfig.cs | 10 +-- .../TaintedDataSymbolMapExtensions.cs | 56 +++++-------- 12 files changed, 238 insertions(+), 110 deletions(-) rename src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/{HardCodeEncryptionKeySinks.cs => HardcodedEncryptionKeySinks.cs} (88%) rename src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/{HardCodeEncryptionKeySources.cs => HardcodedEncryptionKeySources.cs} (57%) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs index 4362fc8042..2c640f2214 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/DoNotHardCodeEncryptionKey.cs @@ -21,7 +21,7 @@ public class DoNotHardCodeEncryptionKey : SourceTriggeredTaintedDataAnalyzerBase descriptionResourceStringName: nameof(SystemSecurityCryptographyResources.DoNotHardCodeEncryptionKeyDescription), customTags: WellKnownDiagnosticTagsExtensions.DataflowAndTelemetry); - protected override SinkKind SinkKind { get { return SinkKind.HardCodeEncryptionKey; } } + protected override SinkKind SinkKind { get { return SinkKind.HardcodedEncryptionKey; } } protected override DiagnosticDescriptor TaintedDataEnteringSinkDescriptor { get { return Rule; } } } diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index e35616828c..0dea084b08 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -7,10 +7,15 @@ using Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; using Microsoft.CodeAnalysis.Operations; namespace Microsoft.NetCore.Analyzers.Security { + using ValueContentAnalysisResult = DataFlowAnalysisResult; + /// /// Base class to aid in implementing tainted data analyzers. /// @@ -59,16 +64,16 @@ public override void Initialize(AnalysisContext context) operationBlockStartContext => { ISymbol owningSymbol = operationBlockStartContext.OwningSymbol; - - HashSet rootOperationsNeedingAnalysis = new HashSet(); + Dictionary rootOperationsNeedingAnalysis = new Dictionary(); operationBlockStartContext.RegisterOperationAction( operationAnalysisContext => { IPropertyReferenceOperation propertyReferenceOperation = (IPropertyReferenceOperation)operationAnalysisContext.Operation; - if (sourceInfoSymbolMap.IsSourceProperty(propertyReferenceOperation.Property)) + IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); + if (sourceInfoSymbolMap.IsSourceProperty(propertyReferenceOperation.Property) && !rootOperationsNeedingAnalysis.ContainsKey(rootOperation)) { - rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + rootOperationsNeedingAnalysis[rootOperation] = false; } }, OperationKind.PropertyReference); @@ -77,9 +82,65 @@ public override void Initialize(AnalysisContext context) operationAnalysisContext => { IInvocationOperation invocationOperation = (IInvocationOperation)operationAnalysisContext.Operation; - if (sourceInfoSymbolMap.IsSourceMethod(invocationOperation.TargetMethod)) + bool needValueContentAnalysis = false; + if (sourceInfoSymbolMap.IsSourceMethod(invocationOperation.TargetMethod, out var evaluateWithPointsToAnalysis, out var evaluateWithValueContentAnslysis)) { - rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); + PointsToAnalysisResult pointsToAnalysisResult; + ValueContentAnalysisResult valueContentAnalysisResultOpt; + if (evaluateWithPointsToAnalysis.Count != 0) + { + pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( + rootOperation.GetEnclosingControlFlowGraph(), + owningSymbol, + WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), + InterproceduralAnalysisConfiguration.Create( + operationAnalysisContext.Options, + SupportedDiagnostics, + defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, + cancellationToken: operationAnalysisContext.CancellationToken), + interproceduralAnalysisPredicateOpt: null); + if (pointsToAnalysisResult == null + || !evaluateWithPointsToAnalysis.Any(s => s( + invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) + { + return; + } + } + else if (evaluateWithValueContentAnslysis.Count != 0) + { + valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( + rootOperation.GetEnclosingControlFlowGraph(), + owningSymbol, + WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), + InterproceduralAnalysisConfiguration.Create( + operationAnalysisContext.Options, + SupportedDiagnostics, + defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, + cancellationToken: operationAnalysisContext.CancellationToken), + out var copyAnalysisResult, + out pointsToAnalysisResult); + if (valueContentAnalysisResultOpt == null + || !evaluateWithValueContentAnslysis.Any(s => s( + invocationOperation.Arguments.Select( + o => pointsToAnalysisResult[o.Kind, o.Syntax]), + invocationOperation.Arguments.Select( + o => valueContentAnalysisResultOpt[o.Kind, o.Syntax])))) + { + return; + } + + needValueContentAnalysis = true; + } + else + { + return; + } + + if (needValueContentAnalysis || !rootOperationsNeedingAnalysis.ContainsKey(rootOperation)) + { + rootOperationsNeedingAnalysis[rootOperation] = needValueContentAnalysis; + } } }, OperationKind.Invocation); @@ -92,7 +153,8 @@ public override void Initialize(AnalysisContext context) IArrayInitializerOperation arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) { - rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + + rootOperationsNeedingAnalysis[operationAnalysisContext.Operation.GetRoot()] = true; } }, OperationKind.ArrayInitializer); @@ -106,10 +168,10 @@ public override void Initialize(AnalysisContext context) return; } - foreach (IOperation rootOperation in rootOperationsNeedingAnalysis) + foreach (KeyValuePair kvp in rootOperationsNeedingAnalysis) { TaintedDataAnalysisResult taintedDataAnalysisResult = TaintedDataAnalysis.TryGetOrComputeResult( - rootOperation.GetEnclosingControlFlowGraph(), + kvp.Key.GetEnclosingControlFlowGraph(), operationBlockAnalysisContext.Compilation, operationBlockAnalysisContext.OwningSymbol, operationBlockAnalysisContext.Options, @@ -117,7 +179,8 @@ public override void Initialize(AnalysisContext context) sourceInfoSymbolMap, taintedDataConfig.GetSanitizerSymbolMap(this.SinkKind), sinkInfoSymbolMap, - operationBlockAnalysisContext.CancellationToken); + operationBlockAnalysisContext.CancellationToken, + kvp.Value); if (taintedDataAnalysisResult == null) { return; diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems b/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems index 0bcebf56e6..9ff55076d4 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems +++ b/src/Utilities/FlowAnalysis/FlowAnalysis.Utilities.projitems @@ -137,8 +137,8 @@ - - + + diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySinks.cs similarity index 88% rename from src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs rename to src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySinks.cs index 2bf8724a91..572a354b35 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySinks.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySinks.cs @@ -5,20 +5,20 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { - internal static class HardCodeEncryptionKeySinks + internal static class HardcodedEncryptionKeySinks { /// /// s for tainted data process symmetric algorithm sinks. /// public static ImmutableHashSet SinkInfos { get; } - static HardCodeEncryptionKeySinks() + static HardcodedEncryptionKeySinks() { var builder = PooledHashSet.GetInstance(); builder.AddSinkInfo( WellKnownTypeNames.SystemSecurityCryptographySymmetricAlgorithm, - SinkKind.HardCodeEncryptionKey, + SinkKind.HardcodedEncryptionKey, isInterface: false, isAnyStringParameterInConstructorASink: false, sinkProperties: new[] { diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySources.cs similarity index 57% rename from src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs rename to src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySources.cs index a57807ef66..9b7257328b 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardCodeEncryptionKeySources.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/HardcodedEncryptionKeySources.cs @@ -1,12 +1,16 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using Analyzer.Utilities.PooledObjects; -using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { - internal static class HardCodeEncryptionKeySources + internal static class HardcodedEncryptionKeySources { /// /// s for hardcoded key tainted data sources. @@ -16,25 +20,25 @@ internal static class HardCodeEncryptionKeySources /// /// Statically constructs. /// - static HardCodeEncryptionKeySources() + static HardcodedEncryptionKeySources() { var builder = PooledHashSet.GetInstance(); - builder.AddSourceInfoWithArgumentChecks( + builder.AddSourceInfo( WellKnownTypeNames.SystemConvert, isInterface: false, taintedProperties: null, - taintedMethods: new (string, (string, ArgumentCheck)[])[] { + taintedMethodsNeedPointsToAnalysis: null, + taintedMethodsNeedsValueContentAnalysis: new (string, IsInvocationTaintedWithValueContentAnalysis)[]{ ("FromBase64String", - new (string, ArgumentCheck)[] { - ("s", (ImmutableHashSet potentialArgumentValues) => !potentialArgumentValues.IsEmpty), - }), + (IEnumerable argumentPonitsTos, IEnumerable argumentValueContents) => argumentValueContents.All(o => o.IsLiteralState)), }); builder.AddSourceInfo( WellKnownTypeNames.SystemByte, isInterface: false, taintedProperties: null, - taintedMethods: null, + taintedMethodsNeedPointsToAnalysis: null, + taintedMethodsNeedsValueContentAnalysis: null, taintConstantArray: true); SourceInfos = builder.ToImmutableAndFree(); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs index 87c268763a..eecb602948 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs @@ -5,7 +5,7 @@ using System.Collections.Immutable; using System.Linq; using Analyzer.Utilities.PooledObjects; -using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -61,30 +61,32 @@ public static void AddSourceInfo( string fullTypeName, bool isInterface, string[] taintedProperties, - string[] taintedMethods, - bool taintConstantArray = false) + IEnumerable taintedMethods) { SourceInfo metadata = new SourceInfo( fullTypeName, isInterface: isInterface, taintedProperties: taintedProperties?.ToImmutableHashSet(StringComparer.Ordinal) ?? ImmutableHashSet.Empty, - taintedMethods: + taintedMethodsNeedPointsToAnalysis: taintedMethods - ?.Select(o => new KeyValuePair>(o, ImmutableDictionary.Empty)) - ?.ToImmutableDictionary(StringComparer.Ordinal) - ?? ImmutableDictionary>.Empty, - taintConstantArray: taintConstantArray); + ?.Select(o => new KeyValuePair(o, (IEnumerable argumentValueContents) => { return true; })) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary.Empty, + taintedMethodsNeedsValueContentAnalysis: + ImmutableDictionary.Empty, + taintConstantArray: false); builder.Add(metadata); } // Just to make hardcoding SourceInfos more convenient. - public static void AddSourceInfoWithArgumentChecks( + public static void AddSourceInfo( this PooledHashSet builder, string fullTypeName, bool isInterface, string[] taintedProperties, - IEnumerable<(string Method, (string parameterName, ArgumentCheck argumentCheck)[] ParameterNameAndConditionChecks)> taintedMethods, + IEnumerable<(string Method, IsInvocationTaintedWithPointsToAnalysis)> taintedMethodsNeedPointsToAnalysis, + IEnumerable<(string Method, IsInvocationTaintedWithValueContentAnalysis)> taintedMethodsNeedsValueContentAnalysis, bool taintConstantArray = false) { SourceInfo metadata = new SourceInfo( @@ -92,16 +94,16 @@ public static void AddSourceInfoWithArgumentChecks( isInterface: isInterface, taintedProperties: taintedProperties?.ToImmutableHashSet(StringComparer.Ordinal) ?? ImmutableHashSet.Empty, - taintedMethods: - taintedMethods - ?.Select(o => new KeyValuePair>( - o.Method, - o.ParameterNameAndConditionChecks - ?.Select(o => new KeyValuePair(o.parameterName, o.argumentCheck)) - ?.ToImmutableDictionary(StringComparer.Ordinal) - ?? ImmutableDictionary.Empty)) - ?.ToImmutableDictionary(StringComparer.Ordinal) - ?? ImmutableDictionary>.Empty, + taintedMethodsNeedPointsToAnalysis: + taintedMethodsNeedPointsToAnalysis + ?.Select(o => new KeyValuePair(o.Method, o.Item2)) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary.Empty, + taintedMethodsNeedsValueContentAnalysis: + taintedMethodsNeedsValueContentAnalysis + ?.Select(o => new KeyValuePair(o.Method, o.Item2)) + ?.ToImmutableDictionary(StringComparer.Ordinal) + ?? ImmutableDictionary.Empty, taintConstantArray: taintConstantArray); builder.Add(metadata); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs index 7eee58d7df..a498c5efc2 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkKind.cs @@ -17,6 +17,6 @@ public enum SinkKind Xml, Xaml, ZipSlip, - HardCodeEncryptionKey, + HardcodedEncryptionKey, } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs index d83b67e553..be1d56b74c 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs @@ -1,10 +1,16 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.Collections.Immutable; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { + public delegate bool IsInvocationTaintedWithPointsToAnalysis(IEnumerable argumentPonitsTos); + public delegate bool IsInvocationTaintedWithValueContentAnalysis(IEnumerable argumentPonitsTos, IEnumerable argumentValueContents); + /// /// Info for tainted data sources, which generate tainted data. /// @@ -15,14 +21,22 @@ internal class SourceInfo : ITaintedDataInfo, IEquatable /// /// Full type name of the...type (namespace + type). /// Properties that generate tainted data. - /// Methods that generate tainted data. + /// Methods that generate tainted data and whose arguments don't need extra value content analysis. + /// Methods that generate tainted data and whose arguments need extra value content analysis and points to analysis. /// - public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet taintedProperties, ImmutableDictionary> taintedMethods, bool taintConstantArray) + public SourceInfo( + string fullTypeName, + bool isInterface, + ImmutableHashSet taintedProperties, + ImmutableDictionary taintedMethodsNeedPointsToAnalysis, + ImmutableDictionary taintedMethodsNeedsValueContentAnalysis, + bool taintConstantArray) { FullTypeName = fullTypeName ?? throw new ArgumentNullException(nameof(fullTypeName)); IsInterface = isInterface; TaintedProperties = taintedProperties ?? throw new ArgumentNullException(nameof(taintedProperties)); - TaintedMethods = taintedMethods ?? throw new ArgumentNullException(nameof(taintedMethods)); + TaintedMethodsNeedPointsToAnalysis = taintedMethodsNeedPointsToAnalysis ?? throw new ArgumentNullException(nameof(taintedMethodsNeedPointsToAnalysis)); + TaintedMethodsNeedsValueContentAnalysis = taintedMethodsNeedsValueContentAnalysis ?? throw new ArgumentNullException(nameof(taintedMethodsNeedsValueContentAnalysis)); TaintConstantArray = taintConstantArray; } @@ -41,12 +55,15 @@ public SourceInfo(string fullTypeName, bool isInterface, ImmutableHashSet public ImmutableHashSet TaintedProperties { get; } - public delegate bool ArgumentCheck(ImmutableHashSet argumentValue); + /// + /// Methods that generate tainted data and whose arguments don't need extra value content analysis. + /// + public ImmutableDictionary TaintedMethodsNeedPointsToAnalysis { get; } /// - /// Methods that generate tainted data. + /// Methods that generate tainted data and whose arguments need extra value content analysis and points to analysis. /// - public ImmutableDictionary> TaintedMethods { get; } + public ImmutableDictionary TaintedMethodsNeedsValueContentAnalysis { get; } /// /// Indicates arrays initialized with constant values of this type generates tainted data. @@ -57,9 +74,10 @@ public override int GetHashCode() { return HashUtilities.Combine(this.TaintConstantArray.GetHashCode(), HashUtilities.Combine(this.TaintedProperties, - HashUtilities.Combine(this.TaintedMethods, + HashUtilities.Combine(this.TaintedMethodsNeedPointsToAnalysis, + HashUtilities.Combine(this.TaintedMethodsNeedsValueContentAnalysis, HashUtilities.Combine(this.IsInterface.GetHashCode(), - StringComparer.Ordinal.GetHashCode(this.FullTypeName))))); + StringComparer.Ordinal.GetHashCode(this.FullTypeName)))))); } public override bool Equals(object obj) @@ -73,7 +91,8 @@ public bool Equals(SourceInfo other) && this.FullTypeName == other.FullTypeName && this.IsInterface == other.IsInterface && this.TaintedProperties == other.TaintedProperties - && this.TaintedMethods == other.TaintedMethods + && this.TaintedMethodsNeedPointsToAnalysis == other.TaintedMethodsNeedPointsToAnalysis + && this.TaintedMethodsNeedsValueContentAnalysis == other.TaintedMethodsNeedsValueContentAnalysis && this.TaintConstantArray == other.TaintConstantArray; } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index aaa8a33d7e..6e7c80278a 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -9,6 +9,8 @@ using Analyzer.Utilities.PooledObjects; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; using Microsoft.CodeAnalysis.Operations; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis @@ -224,13 +226,38 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo ProcessTaintedDataEnteringInvocationOrCreation(method, taintedArguments, originalOperation); } - IEnumerable<(string, ImmutableHashSet)> literalValueOfArguments = (originalOperation as IInvocationOperation).Arguments.Select(o => (o.Parameter.Name, GetValueContentAbstractValue(o.Value).LiteralValues)); if (this.IsSanitizingMethod(method)) { return TaintedDataAbstractValue.NotTainted; } - else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod(method, literalValueOfArguments)) + else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod(method, out var evaluateWithPointsToAnalysis, out var evaluateWithValueContentAnslysis)) { + ImmutableArray argumentOperation = (originalOperation as IInvocationOperation).Arguments; + if (evaluateWithPointsToAnalysis.Count != 0) + { + IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); + if (pointsToAnalysisResultOpt == null + || !evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResultOpt))) + { + return TaintedDataAbstractValue.NotTainted; + } + } + else if (evaluateWithValueContentAnslysis.Count != 0) + { + IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); + IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); + if (valueContentAnalysisResultOpt == null + || pointsToAnalysisResultOpt == null + || !evaluateWithValueContentAnslysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) + { + return TaintedDataAbstractValue.NotTainted; + } + } + else + { + return TaintedDataAbstractValue.NotTainted; + } + return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs index a09fc41ce0..330b114328 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs @@ -5,12 +5,14 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.FlowAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { using ValueContentAnalysisResult = DataFlowAnalysisResult; + using CopyAnalysisResult = DataFlowAnalysisResult; internal partial class TaintedDataAnalysis : ForwardDataFlowAnalysis { @@ -30,12 +32,13 @@ internal static TaintedDataAnalysisResult TryGetOrComputeResult( TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, TaintedDataSymbolMap taintedSinkInfos, - CancellationToken cancellationToken) + CancellationToken cancellationToken, + bool needValueContentAnalysis = false) { var interproceduralAnalysisConfig = InterproceduralAnalysisConfiguration.Create( analyzerOptions, rule, InterproceduralAnalysisKind.ContextSensitive, cancellationToken); return TryGetOrComputeResult(cfg, compilation, containingMethod, taintedSourceInfos, - taintedSanitizerInfos, taintedSinkInfos, interproceduralAnalysisConfig); + taintedSanitizerInfos, taintedSinkInfos, interproceduralAnalysisConfig, needValueContentAnalysis); } private static TaintedDataAnalysisResult TryGetOrComputeResult( @@ -45,21 +48,43 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, TaintedDataSymbolMap taintedSinkInfos, - InterproceduralAnalysisConfiguration interproceduralAnalysisConfig) + InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, + bool needValueContentAnalysis) { WellKnownTypeProvider wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(compilation); - ValueContentAnalysisResult valueContentAnalysisResult = ValueContentAnalysis.TryGetOrComputeResult( - cfg, - containingMethod, - wellKnownTypeProvider, - interproceduralAnalysisConfig, - out var copyAnalysisResult, - out var pointsToAnalysisResult, - pessimisticAnalysis: true, - performCopyAnalysis: false); - if (valueContentAnalysisResult == null) + ValueContentAnalysisResult valueContentAnalysisResult = null; + CopyAnalysisResult copyAnalysisResult = null; + PointsToAnalysisResult pointsToAnalysisResult = null; + if (needValueContentAnalysis) { - return null; + valueContentAnalysisResult = ValueContentAnalysis.TryGetOrComputeResult( + cfg, + containingMethod, + wellKnownTypeProvider, + interproceduralAnalysisConfig, + out copyAnalysisResult, + out pointsToAnalysisResult, + pessimisticAnalysis: true, + performCopyAnalysis: false); + if (valueContentAnalysisResult == null) + { + return null; + } + } + else + { + pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( + cfg, + containingMethod, + wellKnownTypeProvider, + interproceduralAnalysisConfig, + interproceduralAnalysisPredicateOpt: null, + pessimisticAnalysis: true, + performCopyAnalysis: false); + if (pointsToAnalysisResult == null) + { + return null; + } } TaintedDataAnalysisContext analysisContext = TaintedDataAnalysisContext.Create( diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs index 86dade6007..43f90264e3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataConfig.cs @@ -202,8 +202,8 @@ private static ImmutableHashSet GetSourceInfos(SinkKind sinkKind) case SinkKind.ZipSlip: return ZipSlipSources.SourceInfos; - case SinkKind.HardCodeEncryptionKey: - return HardCodeEncryptionKeySources.SourceInfos; + case SinkKind.HardcodedEncryptionKey: + return HardcodedEncryptionKeySources.SourceInfos; default: Debug.Fail($"Unhandled SinkKind {sinkKind}"); @@ -235,7 +235,7 @@ private static ImmutableHashSet GetSanitizerInfos(SinkKind sinkKi case SinkKind.Regex: case SinkKind.Redirect: case SinkKind.Xaml: - case SinkKind.HardCodeEncryptionKey: + case SinkKind.HardcodedEncryptionKey: return ImmutableHashSet.Empty; case SinkKind.ZipSlip: @@ -288,8 +288,8 @@ private static ImmutableHashSet GetSinkInfos(SinkKind sinkKind) case SinkKind.ZipSlip: return ZipSlipSinks.SinkInfos; - case SinkKind.HardCodeEncryptionKey: - return HardCodeEncryptionKeySinks.SinkInfos; + case SinkKind.HardcodedEncryptionKey: + return HardcodedEncryptionKeySinks.SinkInfos; default: Debug.Fail($"Unhandled SinkKind {sinkKind}"); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs index 82203c2a28..0b9a9a1e9c 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs @@ -1,60 +1,48 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; -using System.Collections.Immutable; -using System.Linq; using Microsoft.CodeAnalysis; -using static Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis.SourceInfo; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { internal static class TaintedDataSymbolMapExtensions { /// - /// Determines if the given method is a potential tainted data source. + /// Determines if the given method is a potential tainted data source and get the related argument check methods. /// /// /// + /// + /// /// - public static bool IsSourceMethod(this TaintedDataSymbolMap sourceSymbolMap, IMethodSymbol method) + public static bool IsSourceMethod( + this TaintedDataSymbolMap sourceSymbolMap, + IMethodSymbol method, + out HashSet evaluateWithPointsToAnalysis, + out HashSet evaluateWithValueContentAnslysis) { + evaluateWithPointsToAnalysis = new HashSet(); + evaluateWithValueContentAnslysis = new HashSet(); ; foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType)) { - if (sourceInfo.TaintedMethods.Keys.Contains(method.MetadataName)) + if (sourceInfo.TaintedMethodsNeedPointsToAnalysis.TryGetValue(method.MetadataName, out var pointsToAnalysisMethod)) { - return true; + evaluateWithPointsToAnalysis.Add(pointsToAnalysisMethod); } - } - - return false; - } - - /// - /// Determines if the given method is a tainted data source and all the arguments meet the requirement. - /// - /// - /// - /// - /// - public static bool IsSourceMethod(this TaintedDataSymbolMap sourceSymbolMap, IMethodSymbol method, IEnumerable<(string parameterName, ImmutableHashSet argumentValues)> argumentValues) - { - foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType)) - { - if (sourceInfo.TaintedMethods.TryGetValue(method.MetadataName, out var argumentChecks)) + else if (sourceInfo.TaintedMethodsNeedsValueContentAnalysis.TryGetValue(method.MetadataName, out var valueContentAnalysisMethod)) { - foreach (KeyValuePair kvp in argumentChecks) - { - if (!kvp.Value(argumentValues.FirstOrDefault(o => o.parameterName == kvp.Key).argumentValues)) - { - return false; - } - } - - return true; + evaluateWithValueContentAnslysis.Add(valueContentAnalysisMethod); } } - return false; + if (evaluateWithPointsToAnalysis.Count == 0 && evaluateWithValueContentAnslysis.Count == 0) + { + return false; + } + else + { + return true; + } } /// From 84a34d9139d4ce531b49ce4041fed706d8b75d37 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Fri, 5 Jul 2019 11:35:54 +0800 Subject: [PATCH 06/12] Change the type of rootOperationsNeedingAnalysis into ConcurrentDictionary. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 0dea084b08..4c45a497d5 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Concurrent; using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; @@ -64,7 +65,7 @@ public override void Initialize(AnalysisContext context) operationBlockStartContext => { ISymbol owningSymbol = operationBlockStartContext.OwningSymbol; - Dictionary rootOperationsNeedingAnalysis = new Dictionary(); + ConcurrentDictionary rootOperationsNeedingAnalysis = new ConcurrentDictionary(); operationBlockStartContext.RegisterOperationAction( operationAnalysisContext => @@ -73,7 +74,7 @@ public override void Initialize(AnalysisContext context) IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); if (sourceInfoSymbolMap.IsSourceProperty(propertyReferenceOperation.Property) && !rootOperationsNeedingAnalysis.ContainsKey(rootOperation)) { - rootOperationsNeedingAnalysis[rootOperation] = false; + rootOperationsNeedingAnalysis.AddOrUpdate(rootOperation, false, (k, v) => v == true ? true : false); } }, OperationKind.PropertyReference); @@ -137,10 +138,7 @@ public override void Initialize(AnalysisContext context) return; } - if (needValueContentAnalysis || !rootOperationsNeedingAnalysis.ContainsKey(rootOperation)) - { - rootOperationsNeedingAnalysis[rootOperation] = needValueContentAnalysis; - } + rootOperationsNeedingAnalysis.AddOrUpdate(rootOperation, needValueContentAnalysis, (k, v) => v == true ? true : needValueContentAnalysis); } }, OperationKind.Invocation); @@ -154,7 +152,7 @@ public override void Initialize(AnalysisContext context) if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) { - rootOperationsNeedingAnalysis[operationAnalysisContext.Operation.GetRoot()] = true; + rootOperationsNeedingAnalysis.AddOrUpdate(operationAnalysisContext.Operation.GetRoot(), true, (k, v) => true); } }, OperationKind.ArrayInitializer); From 7494695482db84de8c3c51a6c76f8499c8c57fb6 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Thu, 11 Jul 2019 15:16:55 +0800 Subject: [PATCH 07/12] Add a RequiresValueContentAnalysis method to ITaintedDataInfo and do ValueContentAnalysis if TaintedDataAnalysis might need it. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 145 ++++++++++-------- .../DoNotHardCodeEncryptionKeyTests.cs | 71 +++++++++ .../TaintedDataAnalysis/ITaintedDataInfo.cs | 2 + .../TaintedDataAnalysis/SanitizerInfo.cs | 5 + .../Analysis/TaintedDataAnalysis/SinkInfo.cs | 5 + .../TaintedDataAnalysis/SourceInfo.cs | 7 +- ...ataAnalysis.TaintedDataOperationVisitor.cs | 13 +- .../TaintedDataAnalysis.cs | 11 +- .../TaintedDataSymbolMap.cs | 7 + .../TaintedDataSymbolMapExtensions.cs | 34 ++-- .../ValueContentAnalysis.cs | 2 +- 11 files changed, 215 insertions(+), 87 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 4c45a497d5..ad1ed0876d 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -1,11 +1,10 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System.Collections.Concurrent; -using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using Analyzer.Utilities.Extensions; using Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis; +using Analyzer.Utilities.PooledObjects; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; @@ -65,16 +64,19 @@ public override void Initialize(AnalysisContext context) operationBlockStartContext => { ISymbol owningSymbol = operationBlockStartContext.OwningSymbol; - ConcurrentDictionary rootOperationsNeedingAnalysis = new ConcurrentDictionary(); + PooledHashSet rootOperationsNeedingAnalysis = PooledHashSet.GetInstance(); operationBlockStartContext.RegisterOperationAction( operationAnalysisContext => { IPropertyReferenceOperation propertyReferenceOperation = (IPropertyReferenceOperation)operationAnalysisContext.Operation; IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); - if (sourceInfoSymbolMap.IsSourceProperty(propertyReferenceOperation.Property) && !rootOperationsNeedingAnalysis.ContainsKey(rootOperation)) + if (sourceInfoSymbolMap.IsSourceProperty(propertyReferenceOperation.Property)) { - rootOperationsNeedingAnalysis.AddOrUpdate(rootOperation, false, (k, v) => v == true ? true : false); + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(rootOperation); + } } }, OperationKind.PropertyReference); @@ -83,13 +85,15 @@ public override void Initialize(AnalysisContext context) operationAnalysisContext => { IInvocationOperation invocationOperation = (IInvocationOperation)operationAnalysisContext.Operation; - bool needValueContentAnalysis = false; - if (sourceInfoSymbolMap.IsSourceMethod(invocationOperation.TargetMethod, out var evaluateWithPointsToAnalysis, out var evaluateWithValueContentAnslysis)) + if (sourceInfoSymbolMap.IsSourceMethod( + invocationOperation.TargetMethod, + out PooledHashSet evaluateWithPointsToAnalysis, + out PooledHashSet evaluateWithValueContentAnalysis)) { IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); PointsToAnalysisResult pointsToAnalysisResult; ValueContentAnalysisResult valueContentAnalysisResultOpt; - if (evaluateWithPointsToAnalysis.Count != 0) + if (evaluateWithPointsToAnalysis != null) { pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( rootOperation.GetEnclosingControlFlowGraph(), @@ -101,14 +105,21 @@ public override void Initialize(AnalysisContext context) defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, cancellationToken: operationAnalysisContext.CancellationToken), interproceduralAnalysisPredicateOpt: null); - if (pointsToAnalysisResult == null - || !evaluateWithPointsToAnalysis.Any(s => s( - invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) + if (pointsToAnalysisResult == null) { return; } + + if (evaluateWithPointsToAnalysis.Any(s => s( + invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) + { + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(rootOperation); + } + } } - else if (evaluateWithValueContentAnslysis.Count != 0) + else if (evaluateWithValueContentAnalysis != null) { valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( rootOperation.GetEnclosingControlFlowGraph(), @@ -121,24 +132,23 @@ public override void Initialize(AnalysisContext context) cancellationToken: operationAnalysisContext.CancellationToken), out var copyAnalysisResult, out pointsToAnalysisResult); - if (valueContentAnalysisResultOpt == null - || !evaluateWithValueContentAnslysis.Any(s => s( - invocationOperation.Arguments.Select( - o => pointsToAnalysisResult[o.Kind, o.Syntax]), - invocationOperation.Arguments.Select( - o => valueContentAnalysisResultOpt[o.Kind, o.Syntax])))) + if (valueContentAnalysisResultOpt == null) { return; } - needValueContentAnalysis = true; - } - else - { - return; + if (evaluateWithValueContentAnalysis.Any(s => s( + invocationOperation.Arguments.Select( + o => pointsToAnalysisResult[o.Kind, o.Syntax]), + invocationOperation.Arguments.Select( + o => valueContentAnalysisResultOpt[o.Kind, o.Syntax])))) + { + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(rootOperation); + } + } } - - rootOperationsNeedingAnalysis.AddOrUpdate(rootOperation, needValueContentAnalysis, (k, v) => v == true ? true : needValueContentAnalysis); } }, OperationKind.Invocation); @@ -149,10 +159,12 @@ public override void Initialize(AnalysisContext context) operationAnalysisContext => { IArrayInitializerOperation arrayInitializerOperation = (IArrayInitializerOperation)operationAnalysisContext.Operation; - if (sourceInfoSymbolMap.IsSourceArray(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) + if (sourceInfoSymbolMap.IsSourceConstantArrayOfType(arrayInitializerOperation.Parent.Type as IArrayTypeSymbol)) { - - rootOperationsNeedingAnalysis.AddOrUpdate(operationAnalysisContext.Operation.GetRoot(), true, (k, v) => true); + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(operationAnalysisContext.Operation.GetRoot()); + } } }, OperationKind.ArrayInitializer); @@ -161,53 +173,62 @@ public override void Initialize(AnalysisContext context) operationBlockStartContext.RegisterOperationBlockEndAction( operationBlockAnalysisContext => { - if (!rootOperationsNeedingAnalysis.Any()) - { - return; - } - - foreach (KeyValuePair kvp in rootOperationsNeedingAnalysis) + try { - TaintedDataAnalysisResult taintedDataAnalysisResult = TaintedDataAnalysis.TryGetOrComputeResult( - kvp.Key.GetEnclosingControlFlowGraph(), - operationBlockAnalysisContext.Compilation, - operationBlockAnalysisContext.OwningSymbol, - operationBlockAnalysisContext.Options, - TaintedDataEnteringSinkDescriptor, - sourceInfoSymbolMap, - taintedDataConfig.GetSanitizerSymbolMap(this.SinkKind), - sinkInfoSymbolMap, - operationBlockAnalysisContext.CancellationToken, - kvp.Value); - if (taintedDataAnalysisResult == null) + lock (rootOperationsNeedingAnalysis) { - return; - } - - foreach (TaintedDataSourceSink sourceSink in taintedDataAnalysisResult.TaintedDataSourceSinks) - { - if (!sourceSink.SinkKinds.Contains(this.SinkKind)) + if (!rootOperationsNeedingAnalysis.Any()) { - continue; + return; } - foreach (SymbolAccess sourceOrigin in sourceSink.SourceOrigins) + foreach (IOperation rootOperation in rootOperationsNeedingAnalysis) { - // Something like: - // CA3001: Potential SQL injection vulnerability was found where '{0}' in method '{1}' may be tainted by user-controlled data from '{2}' in method '{3}'. - Diagnostic diagnostic = Diagnostic.Create( - this.TaintedDataEnteringSinkDescriptor, - sourceSink.Sink.Location, - additionalLocations: new Location[] { sourceOrigin.Location }, - messageArgs: new object[] { + TaintedDataAnalysisResult taintedDataAnalysisResult = TaintedDataAnalysis.TryGetOrComputeResult( + rootOperation.GetEnclosingControlFlowGraph(), + operationBlockAnalysisContext.Compilation, + operationBlockAnalysisContext.OwningSymbol, + operationBlockAnalysisContext.Options, + TaintedDataEnteringSinkDescriptor, + sourceInfoSymbolMap, + taintedDataConfig.GetSanitizerSymbolMap(this.SinkKind), + sinkInfoSymbolMap, + operationBlockAnalysisContext.CancellationToken); + if (taintedDataAnalysisResult == null) + { + return; + } + + foreach (TaintedDataSourceSink sourceSink in taintedDataAnalysisResult.TaintedDataSourceSinks) + { + if (!sourceSink.SinkKinds.Contains(this.SinkKind)) + { + continue; + } + + foreach (SymbolAccess sourceOrigin in sourceSink.SourceOrigins) + { + // Something like: + // CA3001: Potential SQL injection vulnerability was found where '{0}' in method '{1}' may be tainted by user-controlled data from '{2}' in method '{3}'. + Diagnostic diagnostic = Diagnostic.Create( + this.TaintedDataEnteringSinkDescriptor, + sourceSink.Sink.Location, + additionalLocations: new Location[] { sourceOrigin.Location }, + messageArgs: new object[] { sourceSink.Sink.Symbol.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat), sourceSink.Sink.AccessingMethod.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat), sourceOrigin.Symbol.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat), sourceOrigin.AccessingMethod.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)}); - operationBlockAnalysisContext.ReportDiagnostic(diagnostic); + operationBlockAnalysisContext.ReportDiagnostic(diagnostic); + } + } } } } + finally + { + rootOperationsNeedingAnalysis.Free(); + } }); }); }); diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs index a3b4b79cfa..d3dcce2f72 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeEncryptionKeyTests.cs @@ -244,6 +244,54 @@ public void TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey) GetCSharpResultAt(17, 9, 13, 33, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey)", "byte[]", "void TestClass.TestMethod(byte[] someOtherBytesForIV, byte[] rgbKey)")); } + [Fact] + public void Test_PassTaintedSourceInfoAsParameter_SinkMethodParameters_Interprocedual_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] key = Convert.FromBase64String(""AAAAAaazaoensuth""); + CreateEncryptor(key, someOtherBytesForIV); + } + + public void CreateEncryptor(byte[] rgbKey, byte[] rgbIV) + { + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(rgbKey, rgbIV); + } +}", + GetCSharpResultAt(16, 9, 9, 22, "ICryptoTransform SymmetricAlgorithm.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "void TestClass.CreateEncryptor(byte[] rgbKey, byte[] rgbIV)", "byte[] Convert.FromBase64String(string s)", "void TestClass.TestMethod(byte[] someOtherBytesForIV)")); + } + + [Fact] + public void Test_PassTaintedSourceInfoAsParameter_SinkProperties_Interprocedual_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public void TestMethod() + { + byte[] key = Convert.FromBase64String(""AAAAAaazaoensuth""); + CreateEncryptor(key); + } + + public void CreateEncryptor(byte[] rgbKey) + { + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.Key = rgbKey; + } +}", + GetCSharpResultAt(16, 9, 9, 22, "byte[] SymmetricAlgorithm.Key", "void TestClass.CreateEncryptor(byte[] rgbKey)", "byte[] Convert.FromBase64String(string s)", "void TestClass.TestMethod()")); + } + [Fact] public void Test_NotHardcoded_CreateEncryptor_NoDiagnostic() { @@ -294,7 +342,30 @@ public void TestMethod(byte[] someOtherBytesForIV) byte[] key = Convert.FromBase64String(Console.ReadLine()); SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); rijn.CreateEncryptor(key, someOtherBytesForIV); + } +}"); + } + + // For now, it doesn't support checking return tainted source info. + [Fact] + public void Test_ReturnTaintedSourceInfo_Interprocedual_NoDiagnostic() + { + VerifyCSharpWithDependencies(@" +using System; +using System.Security.Cryptography; + +class TestClass +{ + public byte[] GetKey() + { + return Convert.FromBase64String(""AAAAAaazaoensuth""); + } + public void TestMethod(byte[] someOtherBytesForIV) + { + byte[] key = GetKey(); + SymmetricAlgorithm rijn = SymmetricAlgorithm.Create(); + rijn.CreateEncryptor(key, someOtherBytesForIV); } }"); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs index 40711ec9e4..56b9b6fb5d 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs @@ -18,5 +18,7 @@ internal interface ITaintedDataInfo /// Indicates that the type is an interface, rather than a concrete type. /// bool IsInterface { get; } + + bool RequiresValueContentAnalysis(); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs index a1b4c5e9fe..bbdc06ecc7 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs @@ -65,5 +65,10 @@ public bool Equals(SanitizerInfo other) && this.SanitizingMethods == other.SanitizingMethods && this.SanitizingInstanceMethods == other.SanitizingInstanceMethods; } + + public bool RequiresValueContentAnalysis() + { + return false; + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs index 1c6e1abb70..77defe0fc4 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs @@ -78,5 +78,10 @@ public bool Equals(SinkInfo other) && this.SinkProperties == other.SinkProperties && this.SinkMethodParameters == other.SinkMethodParameters; } + + public bool RequiresValueContentAnalysis() + { + return false; + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs index be1d56b74c..d16a131267 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs @@ -8,7 +8,7 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { - public delegate bool IsInvocationTaintedWithPointsToAnalysis(IEnumerable argumentPonitsTos); + public delegate bool IsInvocationTaintedWithPointsToAnalysis(IEnumerable argumentPointsTo); public delegate bool IsInvocationTaintedWithValueContentAnalysis(IEnumerable argumentPonitsTos, IEnumerable argumentValueContents); /// @@ -95,5 +95,10 @@ public bool Equals(SourceInfo other) && this.TaintedMethodsNeedsValueContentAnalysis == other.TaintedMethodsNeedsValueContentAnalysis && this.TaintConstantArray == other.TaintConstantArray; } + + public bool RequiresValueContentAnalysis() + { + return this.TaintedMethodsNeedsValueContentAnalysis != null; + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 6e7c80278a..1359d93db0 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -230,10 +230,13 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo { return TaintedDataAbstractValue.NotTainted; } - else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod(method, out var evaluateWithPointsToAnalysis, out var evaluateWithValueContentAnslysis)) + else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod( + method, + out PooledHashSet evaluateWithPointsToAnalysis, + out PooledHashSet evaluateWithValueContentAnalysis)) { ImmutableArray argumentOperation = (originalOperation as IInvocationOperation).Arguments; - if (evaluateWithPointsToAnalysis.Count != 0) + if (evaluateWithPointsToAnalysis != null) { IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); if (pointsToAnalysisResultOpt == null @@ -242,13 +245,13 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo return TaintedDataAbstractValue.NotTainted; } } - else if (evaluateWithValueContentAnslysis.Count != 0) + else if (evaluateWithValueContentAnalysis != null) { IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); if (valueContentAnalysisResultOpt == null || pointsToAnalysisResultOpt == null - || !evaluateWithValueContentAnslysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) + || !evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) { return TaintedDataAbstractValue.NotTainted; } @@ -340,7 +343,7 @@ public override TaintedDataAbstractValue VisitArrayInitializer(IArrayInitializer result = TaintedDataAbstractValue.MergeTainted(taintedAbstractValues); } - if (this.DataFlowAnalysisContext.SourceInfos.IsSourceArray(operation.Parent.Type as IArrayTypeSymbol) + if (this.DataFlowAnalysisContext.SourceInfos.IsSourceConstantArrayOfType(operation.Parent.Type as IArrayTypeSymbol) && operation.ElementValues.All(s => GetValueContentAbstractValue(s).IsLiteralState)) { TaintedDataAbstractValue taintedDataAbstractValue = TaintedDataAbstractValue.CreateTainted(operation.Parent.Type, operation.Syntax, this.OwningSymbol); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs index 330b114328..9f468c9aac 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Threading; +using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.FlowAnalysis; @@ -32,13 +33,12 @@ internal static TaintedDataAnalysisResult TryGetOrComputeResult( TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, TaintedDataSymbolMap taintedSinkInfos, - CancellationToken cancellationToken, - bool needValueContentAnalysis = false) + CancellationToken cancellationToken) { var interproceduralAnalysisConfig = InterproceduralAnalysisConfiguration.Create( analyzerOptions, rule, InterproceduralAnalysisKind.ContextSensitive, cancellationToken); return TryGetOrComputeResult(cfg, compilation, containingMethod, taintedSourceInfos, - taintedSanitizerInfos, taintedSinkInfos, interproceduralAnalysisConfig, needValueContentAnalysis); + taintedSanitizerInfos, taintedSinkInfos, interproceduralAnalysisConfig); } private static TaintedDataAnalysisResult TryGetOrComputeResult( @@ -48,14 +48,13 @@ private static TaintedDataAnalysisResult TryGetOrComputeResult( TaintedDataSymbolMap taintedSourceInfos, TaintedDataSymbolMap taintedSanitizerInfos, TaintedDataSymbolMap taintedSinkInfos, - InterproceduralAnalysisConfiguration interproceduralAnalysisConfig, - bool needValueContentAnalysis) + InterproceduralAnalysisConfiguration interproceduralAnalysisConfig) { WellKnownTypeProvider wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(compilation); ValueContentAnalysisResult valueContentAnalysisResult = null; CopyAnalysisResult copyAnalysisResult = null; PointsToAnalysisResult pointsToAnalysisResult = null; - if (needValueContentAnalysis) + if (taintedSourceInfos.RequiresValueContentAnalysis || taintedSanitizerInfos.RequiresValueContentAnalysis || taintedSinkInfos.RequiresValueContentAnalysis) { valueContentAnalysisResult = ValueContentAnalysis.TryGetOrComputeResult( cfg, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs index 6f8b5f2bd4..14df4a0461 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs @@ -43,6 +43,11 @@ public TaintedDataSymbolMap(WellKnownTypeProvider wellKnownTypeProvider, IEnumer { concreteInfosBuilder[namedTypeSymbol] = info; } + + if (info.RequiresValueContentAnalysis()) + { + RequiresValueContentAnalysis = true; + } } } @@ -65,6 +70,8 @@ public TaintedDataSymbolMap(WellKnownTypeProvider wellKnownTypeProvider, IEnumer /// public bool IsEmpty { get { return this.ConcreteInfos.IsEmpty && this.InterfaceInfos.IsEmpty; } } + public bool RequiresValueContentAnalysis { get; } + /// /// Gets an enumeration of infos for the given type. /// diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs index 0b9a9a1e9c..4ba4420b67 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs @@ -1,6 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System.Collections.Generic; +using Analyzer.Utilities.PooledObjects; using Microsoft.CodeAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis @@ -13,29 +13,39 @@ internal static class TaintedDataSymbolMapExtensions /// /// /// - /// + /// /// public static bool IsSourceMethod( this TaintedDataSymbolMap sourceSymbolMap, IMethodSymbol method, - out HashSet evaluateWithPointsToAnalysis, - out HashSet evaluateWithValueContentAnslysis) + out PooledHashSet evaluateWithPointsToAnalysis, + out PooledHashSet evaluateWithValueContentAnalysis) { - evaluateWithPointsToAnalysis = new HashSet(); - evaluateWithValueContentAnslysis = new HashSet(); ; + evaluateWithPointsToAnalysis = null; + evaluateWithValueContentAnalysis = null; foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType)) { - if (sourceInfo.TaintedMethodsNeedPointsToAnalysis.TryGetValue(method.MetadataName, out var pointsToAnalysisMethod)) + if (sourceInfo.TaintedMethodsNeedPointsToAnalysis.TryGetValue(method.MetadataName, out IsInvocationTaintedWithPointsToAnalysis pointsToAnalysisMethod)) { + if (evaluateWithPointsToAnalysis == null) + { + evaluateWithPointsToAnalysis = PooledHashSet.GetInstance(); + } + evaluateWithPointsToAnalysis.Add(pointsToAnalysisMethod); } - else if (sourceInfo.TaintedMethodsNeedsValueContentAnalysis.TryGetValue(method.MetadataName, out var valueContentAnalysisMethod)) + else if (sourceInfo.TaintedMethodsNeedsValueContentAnalysis.TryGetValue(method.MetadataName, out IsInvocationTaintedWithValueContentAnalysis valueContentAnalysisMethod)) { - evaluateWithValueContentAnslysis.Add(valueContentAnalysisMethod); + if (evaluateWithValueContentAnalysis == null) + { + evaluateWithValueContentAnalysis = PooledHashSet.GetInstance(); + } + + evaluateWithValueContentAnalysis.Add(valueContentAnalysisMethod); } } - if (evaluateWithPointsToAnalysis.Count == 0 && evaluateWithValueContentAnslysis.Count == 0) + if (evaluateWithPointsToAnalysis == null && evaluateWithValueContentAnalysis == null) { return false; } @@ -65,12 +75,12 @@ public static bool IsSourceProperty(this TaintedDataSymbolMap source } /// - /// Determines if the given array is a tainted data source. + /// Determines if the given array can be a tainted data source when its elements are constant all. /// /// /// /// - public static bool IsSourceArray(this TaintedDataSymbolMap sourceSymbolMap, IArrayTypeSymbol arrayTypeSymbol) + public static bool IsSourceConstantArrayOfType(this TaintedDataSymbolMap sourceSymbolMap, IArrayTypeSymbol arrayTypeSymbol) { foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(arrayTypeSymbol.ElementType as INamedTypeSymbol)) { diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs index ccbe53015e..dc0307b6a6 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs @@ -89,7 +89,7 @@ private static ValueContentAnalysisResult TryGetOrComputeResultForAnalysisContex { var operationVisitor = new ValueContentDataFlowOperationVisitor(analysisContext); var nullAnalysis = new ValueContentAnalysis(operationVisitor); - return nullAnalysis.TryGetOrComputeResultCore(analysisContext, cacheResult: false); + return nullAnalysis.TryGetOrComputeResultCore(analysisContext, cacheResult: true); } protected override ValueContentAnalysisResult ToResult(ValueContentAnalysisContext analysisContext, ValueContentAnalysisResult dataFlowAnalysisResult) From fbf9766f149ad249f3a59c69e050f5b3381cf5d3 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Mon, 15 Jul 2019 15:17:28 +0800 Subject: [PATCH 08/12] Wording and comment modification. --- .../TaintedDataAnalysis/ITaintedDataInfo.cs | 9 +++++---- .../Analysis/TaintedDataAnalysis/SanitizerInfo.cs | 11 ++++++----- .../Analysis/TaintedDataAnalysis/SinkInfo.cs | 13 ++++++------- .../Analysis/TaintedDataAnalysis/SourceInfo.cs | 10 +++++----- .../TaintedDataAnalysis/TaintedDataSymbolMap.cs | 6 +++++- .../TaintedDataSymbolMapExtensions.cs | 2 +- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs index 56b9b6fb5d..6bb46ca6f7 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/ITaintedDataInfo.cs @@ -1,6 +1,4 @@ -using System; -using System.Collections.Generic; -using System.Text; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -19,6 +17,9 @@ internal interface ITaintedDataInfo /// bool IsInterface { get; } - bool RequiresValueContentAnalysis(); + /// + /// Indicates that this info uses s. + /// + bool RequiresValueContentAnalysis { get; } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs index bbdc06ecc7..9e908b9baa 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SanitizerInfo.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Immutable; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -44,6 +45,11 @@ public SanitizerInfo(string fullTypeName, bool isInterface, bool isConstructorSa /// public ImmutableHashSet SanitizingInstanceMethods { get; } + /// + /// Indicates that this uses s. + /// + public bool RequiresValueContentAnalysis => false; + public override int GetHashCode() { return HashUtilities.Combine(this.SanitizingMethods, @@ -65,10 +71,5 @@ public bool Equals(SanitizerInfo other) && this.SanitizingMethods == other.SanitizingMethods && this.SanitizingInstanceMethods == other.SanitizingInstanceMethods; } - - public bool RequiresValueContentAnalysis() - { - return false; - } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs index 77defe0fc4..9c68270dc9 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SinkInfo.cs @@ -1,9 +1,8 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Collections.Immutable; -using System.Text; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -53,6 +52,11 @@ public SinkInfo(string fullTypeName, ImmutableHashSet sinkKinds, bool /// public ImmutableDictionary> SinkMethodParameters { get; } + /// + /// Indicates that this uses s. + /// + public bool RequiresValueContentAnalysis => false; + public override int GetHashCode() { return HashUtilities.Combine(this.SinkProperties, @@ -78,10 +82,5 @@ public bool Equals(SinkInfo other) && this.SinkProperties == other.SinkProperties && this.SinkMethodParameters == other.SinkMethodParameters; } - - public bool RequiresValueContentAnalysis() - { - return false; - } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs index d16a131267..a2651ecdc3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs @@ -70,6 +70,11 @@ public SourceInfo( /// public bool TaintConstantArray { get; } + /// + /// Indicates that this uses s. + /// + public bool RequiresValueContentAnalysis => this.TaintedMethodsNeedsValueContentAnalysis != null; + public override int GetHashCode() { return HashUtilities.Combine(this.TaintConstantArray.GetHashCode(), @@ -95,10 +100,5 @@ public bool Equals(SourceInfo other) && this.TaintedMethodsNeedsValueContentAnalysis == other.TaintedMethodsNeedsValueContentAnalysis && this.TaintConstantArray == other.TaintConstantArray; } - - public bool RequiresValueContentAnalysis() - { - return this.TaintedMethodsNeedsValueContentAnalysis != null; - } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs index 14df4a0461..d4ae710861 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs @@ -7,6 +7,7 @@ using Analyzer.Utilities.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -44,7 +45,7 @@ public TaintedDataSymbolMap(WellKnownTypeProvider wellKnownTypeProvider, IEnumer concreteInfosBuilder[namedTypeSymbol] = info; } - if (info.RequiresValueContentAnalysis()) + if (info.RequiresValueContentAnalysis) { RequiresValueContentAnalysis = true; } @@ -70,6 +71,9 @@ public TaintedDataSymbolMap(WellKnownTypeProvider wellKnownTypeProvider, IEnumer /// public bool IsEmpty { get { return this.ConcreteInfos.IsEmpty && this.InterfaceInfos.IsEmpty; } } + /// + /// Indicates that any in this uses s. + /// public bool RequiresValueContentAnalysis { get; } /// diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs index 4ba4420b67..aa7057c65a 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs @@ -75,7 +75,7 @@ public static bool IsSourceProperty(this TaintedDataSymbolMap source } /// - /// Determines if the given array can be a tainted data source when its elements are constant all. + /// Determines if the given array can be a tainted data source when its elements are all constant. /// /// /// From bad699d6578d3279765a7e237b8271acad81a821 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Tue, 16 Jul 2019 11:26:45 +0800 Subject: [PATCH 09/12] Add free operation to PooledHashSet. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 36 +++++++++++++------ ...ataAnalysis.TaintedDataOperationVisitor.cs | 28 +++++++++++---- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index ad1ed0876d..ce863531af 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -110,14 +110,21 @@ public override void Initialize(AnalysisContext context) return; } - if (evaluateWithPointsToAnalysis.Any(s => s( - invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) + try { - lock (rootOperationsNeedingAnalysis) + if (evaluateWithPointsToAnalysis.Any(s => s( + invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) { - rootOperationsNeedingAnalysis.Add(rootOperation); + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(rootOperation); + } } } + finally + { + evaluateWithPointsToAnalysis.Free(); + } } else if (evaluateWithValueContentAnalysis != null) { @@ -137,17 +144,24 @@ public override void Initialize(AnalysisContext context) return; } - if (evaluateWithValueContentAnalysis.Any(s => s( - invocationOperation.Arguments.Select( - o => pointsToAnalysisResult[o.Kind, o.Syntax]), - invocationOperation.Arguments.Select( - o => valueContentAnalysisResultOpt[o.Kind, o.Syntax])))) + try { - lock (rootOperationsNeedingAnalysis) + if (evaluateWithValueContentAnalysis.Any(s => s( + invocationOperation.Arguments.Select( + o => pointsToAnalysisResult[o.Kind, o.Syntax]), + invocationOperation.Arguments.Select( + o => valueContentAnalysisResultOpt[o.Kind, o.Syntax])))) { - rootOperationsNeedingAnalysis.Add(rootOperation); + lock (rootOperationsNeedingAnalysis) + { + rootOperationsNeedingAnalysis.Add(rootOperation); + } } } + finally + { + evaluateWithValueContentAnalysis.Free(); + } } } }, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 1359d93db0..8f57bc67f4 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -239,21 +239,35 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo if (evaluateWithPointsToAnalysis != null) { IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); - if (pointsToAnalysisResultOpt == null - || !evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResultOpt))) + try { - return TaintedDataAbstractValue.NotTainted; + if (pointsToAnalysisResultOpt == null + || !evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResultOpt))) + { + return TaintedDataAbstractValue.NotTainted; + } + } + finally + { + evaluateWithPointsToAnalysis.Free(); } } else if (evaluateWithValueContentAnalysis != null) { IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); - if (valueContentAnalysisResultOpt == null - || pointsToAnalysisResultOpt == null - || !evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) + try + { + if (valueContentAnalysisResultOpt == null + || pointsToAnalysisResultOpt == null + || !evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) + { + return TaintedDataAbstractValue.NotTainted; + } + } + finally { - return TaintedDataAbstractValue.NotTainted; + evaluateWithValueContentAnalysis.Free(); } } else From f487b6636515e156974d17154a66eabf93e8f572 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Thu, 18 Jul 2019 10:20:09 +0800 Subject: [PATCH 10/12] Move free operation position. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 103 +++++++++--------- .../PooledHashSetExtensions.cs | 8 +- ...ataAnalysis.TaintedDataOperationVisitor.cs | 65 +++++------ 3 files changed, 91 insertions(+), 85 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index ce863531af..8bd84d759b 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -85,33 +85,35 @@ public override void Initialize(AnalysisContext context) operationAnalysisContext => { IInvocationOperation invocationOperation = (IInvocationOperation)operationAnalysisContext.Operation; - if (sourceInfoSymbolMap.IsSourceMethod( - invocationOperation.TargetMethod, - out PooledHashSet evaluateWithPointsToAnalysis, - out PooledHashSet evaluateWithValueContentAnalysis)) + PooledHashSet evaluateWithPointsToAnalysis = null; + PooledHashSet evaluateWithValueContentAnalysis = null; + try { - IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); - PointsToAnalysisResult pointsToAnalysisResult; - ValueContentAnalysisResult valueContentAnalysisResultOpt; - if (evaluateWithPointsToAnalysis != null) + if (sourceInfoSymbolMap.IsSourceMethod( + invocationOperation.TargetMethod, + out evaluateWithPointsToAnalysis, + out evaluateWithValueContentAnalysis)) { - pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( - rootOperation.GetEnclosingControlFlowGraph(), - owningSymbol, - WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), - InterproceduralAnalysisConfiguration.Create( - operationAnalysisContext.Options, - SupportedDiagnostics, - defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, - cancellationToken: operationAnalysisContext.CancellationToken), - interproceduralAnalysisPredicateOpt: null); - if (pointsToAnalysisResult == null) + IOperation rootOperation = operationAnalysisContext.Operation.GetRoot(); + PointsToAnalysisResult pointsToAnalysisResult; + ValueContentAnalysisResult valueContentAnalysisResultOpt; + if (evaluateWithPointsToAnalysis != null) { - return; - } + pointsToAnalysisResult = PointsToAnalysis.TryGetOrComputeResult( + rootOperation.GetEnclosingControlFlowGraph(), + owningSymbol, + WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), + InterproceduralAnalysisConfiguration.Create( + operationAnalysisContext.Options, + SupportedDiagnostics, + defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, + cancellationToken: operationAnalysisContext.CancellationToken), + interproceduralAnalysisPredicateOpt: null); + if (pointsToAnalysisResult == null) + { + return; + } - try - { if (evaluateWithPointsToAnalysis.Any(s => s( invocationOperation.Arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax])))) { @@ -121,31 +123,24 @@ public override void Initialize(AnalysisContext context) } } } - finally - { - evaluateWithPointsToAnalysis.Free(); - } - } - else if (evaluateWithValueContentAnalysis != null) - { - valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( - rootOperation.GetEnclosingControlFlowGraph(), - owningSymbol, - WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), - InterproceduralAnalysisConfiguration.Create( - operationAnalysisContext.Options, - SupportedDiagnostics, - defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, - cancellationToken: operationAnalysisContext.CancellationToken), - out var copyAnalysisResult, - out pointsToAnalysisResult); - if (valueContentAnalysisResultOpt == null) + else if (evaluateWithValueContentAnalysis != null) { - return; - } + valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( + rootOperation.GetEnclosingControlFlowGraph(), + owningSymbol, + WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation), + InterproceduralAnalysisConfiguration.Create( + operationAnalysisContext.Options, + SupportedDiagnostics, + defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive, + cancellationToken: operationAnalysisContext.CancellationToken), + out var copyAnalysisResult, + out pointsToAnalysisResult); + if (valueContentAnalysisResultOpt == null) + { + return; + } - try - { if (evaluateWithValueContentAnalysis.Any(s => s( invocationOperation.Arguments.Select( o => pointsToAnalysisResult[o.Kind, o.Syntax]), @@ -158,10 +153,18 @@ public override void Initialize(AnalysisContext context) } } } - finally - { - evaluateWithValueContentAnalysis.Free(); - } + } + } + finally + { + if (evaluateWithPointsToAnalysis != null) + { + evaluateWithPointsToAnalysis.Free(); + } + + if (evaluateWithValueContentAnalysis != null) + { + evaluateWithValueContentAnalysis.Free(); } } }, diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs index eecb602948..f125243b26 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/PooledHashSetExtensions.cs @@ -85,8 +85,8 @@ public static void AddSourceInfo( string fullTypeName, bool isInterface, string[] taintedProperties, - IEnumerable<(string Method, IsInvocationTaintedWithPointsToAnalysis)> taintedMethodsNeedPointsToAnalysis, - IEnumerable<(string Method, IsInvocationTaintedWithValueContentAnalysis)> taintedMethodsNeedsValueContentAnalysis, + IEnumerable<(string Method, IsInvocationTaintedWithPointsToAnalysis pointsToCheck)> taintedMethodsNeedPointsToAnalysis, + IEnumerable<(string Method, IsInvocationTaintedWithValueContentAnalysis valueContentCheck)> taintedMethodsNeedsValueContentAnalysis, bool taintConstantArray = false) { SourceInfo metadata = new SourceInfo( @@ -96,12 +96,12 @@ public static void AddSourceInfo( ?? ImmutableHashSet.Empty, taintedMethodsNeedPointsToAnalysis: taintedMethodsNeedPointsToAnalysis - ?.Select(o => new KeyValuePair(o.Method, o.Item2)) + ?.Select(o => new KeyValuePair(o.Method, o.pointsToCheck)) ?.ToImmutableDictionary(StringComparer.Ordinal) ?? ImmutableDictionary.Empty, taintedMethodsNeedsValueContentAnalysis: taintedMethodsNeedsValueContentAnalysis - ?.Select(o => new KeyValuePair(o.Method, o.Item2)) + ?.Select(o => new KeyValuePair(o.Method, o.valueContentCheck)) ?.ToImmutableDictionary(StringComparer.Ordinal) ?? ImmutableDictionary.Empty, taintConstantArray: taintConstantArray); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 8f57bc67f4..50ed1d3b89 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -226,38 +226,33 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo ProcessTaintedDataEnteringInvocationOrCreation(method, taintedArguments, originalOperation); } - if (this.IsSanitizingMethod(method)) + PooledHashSet evaluateWithPointsToAnalysis = null; + PooledHashSet evaluateWithValueContentAnalysis = null; + try { - return TaintedDataAbstractValue.NotTainted; - } - else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod( - method, - out PooledHashSet evaluateWithPointsToAnalysis, - out PooledHashSet evaluateWithValueContentAnalysis)) - { - ImmutableArray argumentOperation = (originalOperation as IInvocationOperation).Arguments; - if (evaluateWithPointsToAnalysis != null) + if (this.IsSanitizingMethod(method)) + { + return TaintedDataAbstractValue.NotTainted; + } + else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod( + method, + out evaluateWithPointsToAnalysis, + out evaluateWithValueContentAnalysis)) { - IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); - try + ImmutableArray argumentOperation = (originalOperation as IInvocationOperation).Arguments; + if (evaluateWithPointsToAnalysis != null) { + IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); if (pointsToAnalysisResultOpt == null || !evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResultOpt))) { return TaintedDataAbstractValue.NotTainted; } } - finally - { - evaluateWithPointsToAnalysis.Free(); - } - } - else if (evaluateWithValueContentAnalysis != null) - { - IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); - IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); - try + else if (evaluateWithValueContentAnalysis != null) { + IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); + IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); if (valueContentAnalysisResultOpt == null || pointsToAnalysisResultOpt == null || !evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) @@ -265,26 +260,34 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo return TaintedDataAbstractValue.NotTainted; } } - finally + else { - evaluateWithValueContentAnalysis.Free(); + return TaintedDataAbstractValue.NotTainted; } + + return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } - else + else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) { + if (AnalysisEntityFactory.TryCreate(visitedInstance, out AnalysisEntity analysisEntity)) + { + this.CurrentAnalysisData.SetAbstractValue(analysisEntity, TaintedDataAbstractValue.NotTainted); + } + return TaintedDataAbstractValue.NotTainted; } - - return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } - else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) + finally { - if (AnalysisEntityFactory.TryCreate(visitedInstance, out AnalysisEntity analysisEntity)) + if (evaluateWithPointsToAnalysis != null) { - this.CurrentAnalysisData.SetAbstractValue(analysisEntity, TaintedDataAbstractValue.NotTainted); + evaluateWithPointsToAnalysis.Free(); } - return TaintedDataAbstractValue.NotTainted; + if (evaluateWithValueContentAnalysis != null) + { + evaluateWithValueContentAnalysis.Free(); + } } return baseVisit; From 730405ecf80ad066ff89daadab715581c6695449 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Mon, 22 Jul 2019 10:53:32 +0800 Subject: [PATCH 11/12] Update. --- .../SourceTriggeredTaintedDataAnalyzerBase.cs | 5 +++- ...ataAnalysis.TaintedDataOperationVisitor.cs | 29 ++++--------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs index 8bd84d759b..8558f6733e 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs @@ -120,10 +120,13 @@ public override void Initialize(AnalysisContext context) lock (rootOperationsNeedingAnalysis) { rootOperationsNeedingAnalysis.Add(rootOperation); + + return; } } } - else if (evaluateWithValueContentAnalysis != null) + + if (evaluateWithValueContentAnalysis != null) { valueContentAnalysisResultOpt = ValueContentAnalysis.TryGetOrComputeResult( rootOperation.GetEnclosingControlFlowGraph(), diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 50ed1d3b89..1a92daa5f3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -240,32 +240,15 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo out evaluateWithValueContentAnalysis)) { ImmutableArray argumentOperation = (originalOperation as IInvocationOperation).Arguments; - if (evaluateWithPointsToAnalysis != null) + IEnumerable pointsToAnalysisResult = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); + IEnumerable valueContentAnalysisResult = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); + if ((evaluateWithPointsToAnalysis != null && evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResult))) + || (evaluateWithValueContentAnalysis != null && evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResult, valueContentAnalysisResult)))) { - IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); - if (pointsToAnalysisResultOpt == null - || !evaluateWithPointsToAnalysis.Any(o => o(pointsToAnalysisResultOpt))) - { - return TaintedDataAbstractValue.NotTainted; - } - } - else if (evaluateWithValueContentAnalysis != null) - { - IEnumerable pointsToAnalysisResultOpt = argumentOperation.Select(o => GetPointsToAbstractValue(o.Value)); - IEnumerable valueContentAnalysisResultOpt = argumentOperation.Select(o => GetValueContentAbstractValue(o.Value)); - if (valueContentAnalysisResultOpt == null - || pointsToAnalysisResultOpt == null - || !evaluateWithValueContentAnalysis.Any(o => o(pointsToAnalysisResultOpt, valueContentAnalysisResultOpt))) - { - return TaintedDataAbstractValue.NotTainted; - } - } - else - { - return TaintedDataAbstractValue.NotTainted; + return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } - return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); + return TaintedDataAbstractValue.NotTainted; } else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) { From 0d2a8d7482c04a8e0bccb680e11a8af51c5d659d Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Tue, 23 Jul 2019 12:49:16 +0800 Subject: [PATCH 12/12] Move sanitizer check higher. --- ...DataAnalysis.TaintedDataOperationVisitor.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 1a92daa5f3..4158b516b3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -234,6 +234,15 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo { return TaintedDataAbstractValue.NotTainted; } + else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) + { + if (AnalysisEntityFactory.TryCreate(visitedInstance, out AnalysisEntity analysisEntity)) + { + this.CurrentAnalysisData.SetAbstractValue(analysisEntity, TaintedDataAbstractValue.NotTainted); + } + + return TaintedDataAbstractValue.NotTainted; + } else if (this.DataFlowAnalysisContext.SourceInfos.IsSourceMethod( method, out evaluateWithPointsToAnalysis, @@ -248,15 +257,6 @@ public override TaintedDataAbstractValue VisitInvocation_NonLambdaOrDelegateOrLo return TaintedDataAbstractValue.CreateTainted(method, originalOperation.Syntax, this.OwningSymbol); } - return TaintedDataAbstractValue.NotTainted; - } - else if (visitedInstance != null && this.IsSanitizingInstanceMethod(method)) - { - if (AnalysisEntityFactory.TryCreate(visitedInstance, out AnalysisEntity analysisEntity)) - { - this.CurrentAnalysisData.SetAbstractValue(analysisEntity, TaintedDataAbstractValue.NotTainted); - } - return TaintedDataAbstractValue.NotTainted; } }