-
Notifications
You must be signed in to change notification settings - Fork 231
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
Co-authored-by: mary-georgiou-sonarsource <[email protected]> Co-authored-by: Tim Pohlmann <[email protected]>
- Loading branch information
3 people
authored
Jul 24, 2023
1 parent
be9ca30
commit 14115ea
Showing
15 changed files
with
828 additions
and
32 deletions.
There are no files selected for viewing
17 changes: 17 additions & 0 deletions
17
...its/expected/Ember-MM/scraper.EmberCore-{EF6A550E-DD76-4F4D-8250-8598140F828B}-S5773.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "issues": [ | ||
| { | ||
| "id": "S5773", | ||
| "message": "Restrict types of objects allowed to be deserialized.", | ||
| "location": { | ||
| "uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Addons/scraper.EmberCore/TVScraper/clsScrapeTVDB.vb#L150", | ||
| "region": { | ||
| "startLine": 150, | ||
| "startColumn": 41, | ||
| "endLine": 150, | ||
| "endColumn": 59 | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| <h2>Why is this an issue?</h2> | ||
| <p>During the deserialization process, the state of an object will be reconstructed from the serialized data stream which can contain dangerous | ||
| operations.</p> | ||
| <p>For example, a well-known attack vector consists in serializing an object of type <code><a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.codedom.compiler.tempfilecollection.-ctor?view=netframework-4.8#System_CodeDom_Compiler_TempFileCollection__ctor">TempFileCollection</a></code> | ||
| with arbitrary files (defined by an attacker) which will be deleted on the application deserializing this object (when the <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.codedom.compiler.tempfilecollection.finalize?view=netframework-4.8">finalize() </a>method of | ||
| the TempFileCollection object is called). This kind of types are called "<a href="https://github.com/pwntester/ysoserial.net">gadgets</a>".</p> | ||
| <p>Instead of using <code>BinaryFormatter</code> and similar serializers, it is recommended to use safer alternatives in most of the cases, such as <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.xml.serialization.xmlserializer?view=net-5.0">XmlSerializer</a> or <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.datacontractserializer?view=net-5.0">DataContractSerializer</a>. If | ||
| it’s not possible then try to mitigate the risk by restricting the types allowed to be deserialized:</p> | ||
| <ul> | ||
| <li> by implementing an "allow-list" of types, but keep in mind that novel dangerous types are regularly discovered and this protection could be | ||
| insufficient over time. </li> | ||
| <li> or/and implementing a tamper protection, such as <a href="https://en.wikipedia.org/wiki/HMAC">message authentication codes</a> (MAC). This way | ||
| only objects serialized with the correct MAC hash will be deserialized. </li> | ||
| </ul> | ||
| <h3>Noncompliant code example</h3> | ||
| <p>For <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter?view=netframework-4.8">BinaryFormatter</a>, | ||
| <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.netdatacontractserializer?view=netframework-4.8">NetDataContractSerializer</a>, | ||
| <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.soap.soapformatter?view=netframework-4.8">SoapFormatter</a> | ||
| serializers:</p> | ||
| <pre> | ||
| Dim myBinaryFormatter = New BinaryFormatter() | ||
| myBinaryFormatter.Deserialize(stream) ' Noncompliant: a binder is not used to limit types during deserialization | ||
| </pre> | ||
| <p><a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.web.script.serialization.javascriptserializer?view=netframework-4.8">JavaScriptSerializer</a> | ||
| should not use SimpleTypeResolver or other weak resolvers:</p> | ||
| <pre> | ||
| Dim serializer1 As JavaScriptSerializer = New JavaScriptSerializer(New SimpleTypeResolver()) ' Noncompliant: SimpleTypeResolver is unsecure (every types is resolved) | ||
| serializer1.Deserialize(Of ExpectedType)(json) | ||
| </pre> | ||
| <p><a href="https://docs.microsoft.com/en-us/dotnet/api/system.web.ui.losformatter?view=netframework-4.8">LosFormatter</a> should not be used without | ||
| MAC verification:</p> | ||
| <pre> | ||
| Dim formatter As LosFormatter = New LosFormatter() ' Noncompliant | ||
| formatter.Deserialize(fs) | ||
| </pre> | ||
| <h3>Compliant solution</h3> | ||
| <p><a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter?view=netframework-4.8">BinaryFormatter</a>, | ||
| <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.netdatacontractserializer?view=netframework-4.8">NetDataContractSerializer | ||
| </a>, <a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.soap.soapformatter?view=netframework-4.8">SoapFormatter</a> | ||
| serializers should use a binder implementing a whitelist approach to limit types during deserialization (at least one exception should be thrown or a | ||
| null value returned):</p> | ||
| <pre> | ||
| NotInheritable Class CustomBinder | ||
| Inherits SerializationBinder | ||
| Public Overrides Function BindToType(assemblyName As String, typeName As String) As Type | ||
| If Not (Equals(typeName, "type1") OrElse Equals(typeName, "type2") OrElse Equals(typeName, "type3")) Then | ||
| Throw New SerializationException("Only type1, type2 and type3 are allowed") ' Compliant | ||
| End If | ||
| Return Assembly.Load(assemblyName).[GetType](typeName) | ||
| End Function | ||
| End Class | ||
|
|
||
| Dim myBinaryFormatter = New BinaryFormatter() | ||
| myBinaryFormatter.Binder = New CustomBinder() | ||
| myBinaryFormatter.Deserialize(stream) | ||
| </pre> | ||
| <p><a | ||
| href="https://docs.microsoft.com/en-us/dotnet/api/system.web.script.serialization.javascriptserializer?view=netframework-4.8">JavaScriptSerializer</a> | ||
| should use a resolver implementing a whitelist to limit types during deserialization (at least one exception should be thrown or a null value | ||
| returned):</p> | ||
| <pre> | ||
| Public Class CustomSafeTypeResolver | ||
| Inherits JavaScriptTypeResolver | ||
| Public Overrides Function ResolveType(id As String) As Type | ||
| If Not Equals(id, "ExpectedType") Then | ||
| Throw New ArgumentNullException("Only ExpectedType is allowed during deserialization") ' Compliant | ||
| End If | ||
| Return Type.[GetType](id) | ||
| End Function | ||
| End Class | ||
|
|
||
| Dim serializer As JavaScriptSerializer = New JavaScriptSerializer(New CustomSafeTypeResolver()) ' Compliant | ||
| serializer.Deserialize(Of ExpectedType)(json) | ||
| </pre> | ||
| <p><a href="https://docs.microsoft.com/en-us/dotnet/api/system.web.ui.losformatter?view=netframework-4.8">LosFormatter</a> serializer with MAC | ||
| verification:</p> | ||
| <pre> | ||
| Dim formatter As LosFormatter = New LosFormatter(True, secret) ' Compliant | ||
| formatter.Deserialize(fs) | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <ul> | ||
| <li> <a href="https://owasp.org/Top10/A08_2021-Software_and_Data_Integrity_Failures/">OWASP Top 10 2021 Category A8</a> - Software and Data | ||
| Integrity Failures </li> | ||
| <li> <a href="https://docs.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide?s=03">docs.microsoft.com</a> - | ||
| BinaryFormatter security guide </li> | ||
| <li> <a href="https://owasp.org/www-project-top-ten/2017/A8_2017-Insecure_Deserialization">OWASP Top 10 2017 Category A8</a> - Insecure | ||
| Deserialization </li> | ||
| <li> <a href="https://cwe.mitre.org/data/definitions/134">MITRE, CWE-134</a> - Use of Externally-Controlled Format String </li> | ||
| <li> <a href="https://cwe.mitre.org/data/definitions/502">MITRE, CWE-502</a> - Deserialization of Untrusted Data </li> | ||
| <li> <a href="https://www.sans.org/top25-software-errors/#cat2">SANS Top 25</a> - Risky Resource Management </li> | ||
| <li> <a href="https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Deserialization_Cheat_Sheet.md">OWASP Deserialization Cheat | ||
| Sheet</a> </li> | ||
| </ul> | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "title": "Types allowed to be deserialized should be restricted", | ||
| "type": "VULNERABILITY", | ||
| "status": "ready", | ||
| "quickfix": "targeted", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "30min" | ||
| }, | ||
| "tags": [ | ||
| "cwe" | ||
| ], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-5773", | ||
| "sqKey": "S5773", | ||
| "scope": "Main", | ||
| "securityStandards": { | ||
| "CWE": [ | ||
| 502 | ||
| ], | ||
| "OWASP": [ | ||
| "A8" | ||
| ], | ||
| "OWASP Top 10 2021": [ | ||
| "A8" | ||
| ], | ||
| "ASVS 4.0": [ | ||
| "1.5.2", | ||
| "5.5.1", | ||
| "5.5.3" | ||
| ] | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,7 @@ | |
| "S5659", | ||
| "S5693", | ||
| "S5753", | ||
| "S5773", | ||
| "S5856", | ||
| "S5944", | ||
| "S6145", | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
82 changes: 82 additions & 0 deletions
82
...yzers/src/SonarAnalyzer.VisualBasic/SymbolicExecution/Roslyn/RestrictDeserializedTypes.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * SonarAnalyzer for .NET | ||
| * Copyright (C) 2015-2023 SonarSource SA | ||
| * mailto: contact AT sonarsource DOT com | ||
| * | ||
| * This program is free software; you can redistribute it and/or | ||
| * modify it under the terms of the GNU Lesser General Public | ||
| * License as published by the Free Software Foundation; either | ||
| * version 3 of the License, or (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| * Lesser General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public License | ||
| * along with this program; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| using System.Runtime.Serialization; | ||
| using StyleCop.Analyzers.Lightup; | ||
|
|
||
| namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.VisualBasic; | ||
|
|
||
| public sealed class RestrictDeserializedTypes : RestrictDeserializedTypesBase | ||
| { | ||
| public static readonly DiagnosticDescriptor S5773 = DescriptorFactory.Create(DiagnosticId, MessageFormat); | ||
|
|
||
| protected override DiagnosticDescriptor Rule => S5773; | ||
|
|
||
| public override bool ShouldExecute() | ||
| { | ||
| var walker = new Walker(); | ||
| walker.SafeVisit(Node); | ||
| return walker.Result; | ||
| } | ||
|
|
||
| protected override SyntaxNode FindBindToTypeMethodDeclaration(IOperation operation) => | ||
| MethodCandidates(operation).FirstOrDefault(x => | ||
| x is MethodBlockSyntax { SubOrFunctionStatement: { Identifier.Text: nameof(SerializationBinder.BindToType), ParameterList: { Parameters.Count: 2 } parameterList } } | ||
| && parameterList.EnsureCorrectSemanticModelOrDefault(SemanticModel) is { } semanticModel | ||
| && parameterList.Parameters[0].AsClause.Type.IsKnownType(KnownType.System_String, semanticModel) | ||
| && parameterList.Parameters[1].AsClause.Type.IsKnownType(KnownType.System_String, semanticModel)); | ||
|
|
||
| protected override SyntaxNode FindResolveTypeMethodDeclaration(IOperation operation) => | ||
| MethodCandidates(operation)?.FirstOrDefault(x => | ||
| x is MethodBlockSyntax { SubOrFunctionStatement: { Identifier.Text: "ResolveType", ParameterList: { Parameters.Count: 1 } parameterList } } | ||
| && parameterList.EnsureCorrectSemanticModelOrDefault(SemanticModel) is { } semanticModel | ||
| && parameterList.Parameters[0].AsClause.Type.IsKnownType(KnownType.System_String, semanticModel)); | ||
|
|
||
| protected override bool ThrowsOrReturnsNull(SyntaxNode methodDeclaration) => | ||
| methodDeclaration.DescendantNodes().OfType<ThrowStatementSyntax>().Any() || | ||
| methodDeclaration.DescendantNodes().OfType<ExpressionSyntax>().Any(expression => expression.IsKind(SyntaxKindEx.ThrowExpression)) || | ||
| methodDeclaration.DescendantNodes().OfType<ReturnStatementSyntax>().Any(returnStatement => returnStatement.Expression.IsKind(SyntaxKind.NothingLiteralExpression)) || | ||
| // For simplicity this returns true for any method witch contains a NullLiteralExpression but this could be a source of FNs | ||
| methodDeclaration.DescendantNodes().OfType<ExpressionSyntax>().Any(expression => expression.IsKind(SyntaxKind.NothingLiteralExpression)); | ||
|
|
||
| protected override SyntaxToken GetIdentifier(SyntaxNode methodDeclaration) => ((MethodBlockSyntax)methodDeclaration).SubOrFunctionStatement.Identifier; | ||
|
|
||
| private static IEnumerable<SyntaxNode> MethodCandidates(IOperation operation) => | ||
| operation.Type?.DeclaringSyntaxReferences.SelectMany(x => x.GetSyntax().Parent.DescendantNodes()); | ||
|
|
||
| private sealed class Walker : SafeVisualBasicSyntaxWalker | ||
| { | ||
| public bool Result { get; private set; } | ||
|
|
||
| public override void Visit(SyntaxNode node) | ||
| { | ||
| if (!Result) | ||
| { | ||
| base.Visit(node); | ||
| } | ||
| } | ||
|
|
||
| public override void VisitInvocationExpression(InvocationExpressionSyntax node) => | ||
| Result = node.NameIs(nameof(IFormatter.Deserialize)); | ||
|
|
||
| public override void VisitObjectCreationExpression(ObjectCreationExpressionSyntax node) => | ||
| Result = node.Type.NameIs("LosFormatter"); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.