Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule S4834: Controlling permissions is security-sensitive #2096

Merged
merged 2 commits into from
Nov 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy\Owin\NancyMiddleware.cs",
"region": {
"startLine": 253,
"startColumn": 57,
"endLine": 253,
"endColumn": 73
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs",
"region": {
"startLine": 18,
"startColumn": 28,
"endLine": 18,
"endColumn": 43
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs",
"region": {
"startLine": 29,
"startColumn": 28,
"endLine": 29,
"endColumn": 37
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs",
"region": {
"startLine": 40,
"startColumn": 28,
"endLine": 40,
"endColumn": 39
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs",
"region": {
"startLine": 52,
"startColumn": 28,
"endLine": 52,
"endColumn": 42
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication\AuthenticationBootstrapper.cs",
"region": {
"startLine": 27,
"startColumn": 39,
"endLine": 27,
"endColumn": 116
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication\AuthenticationBootstrapper.cs",
"region": {
"startLine": 27,
"startColumn": 59,
"endLine": 27,
"endColumn": 115
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Basic\UserValidator.cs",
"region": {
"startLine": 14,
"startColumn": 24,
"endLine": 14,
"endColumn": 74
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Basic\UserValidator.cs",
"region": {
"startLine": 14,
"startColumn": 44,
"endLine": 14,
"endColumn": 73
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Forms\UserDatabase.cs",
"region": {
"startLine": 27,
"startColumn": 26,
"endLine": 27,
"endColumn": 84
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Forms\UserDatabase.cs",
"region": {
"startLine": 27,
"startColumn": 46,
"endLine": 27,
"endColumn": 83
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Stateless\UserDatabase.cs",
"region": {
"startLine": 30,
"startColumn": 20,
"endLine": 30,
"endColumn": 91
}
}
},
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Stateless\UserDatabase.cs",
"region": {
"startLine": 30,
"startColumn": 40,
"endLine": 30,
"endColumn": 90
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"issues": [
{
"id": "S4834",
"message": "Make sure that permissions are controlled safely here.",
"location": {
"uri": "sources\Nancy\src\Nancy.Hosting.Self\NancyHost.cs",
"region": {
"startLine": 213,
"startColumn": 19,
"endLine": 213,
"endColumn": 47
}
}
}
]
}
80 changes: 80 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S4834_c#.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<p>Controlling permissions is security-sensitive. It has led in the past to the following vulnerabilities:</p>
<ul>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-12999">CVE-2018-12999</a> </li>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-10285">CVE-2018-10285</a> </li>
<li> <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-7455">CVE-2017-7455</a> </li>
</ul>
<p>Attackers can only damage what they have access to. Thus limiting their access is a good way to prevent them from wreaking havoc, but it has to be
done properly.</p>
<p>This rule flags code that controls the access to resources and actions or configures this access. The goal is to guide security code reviews.</p>
<h2>Ask Yourself Whether</h2>
<ul>
<li> at least one accessed action or resource is security-sensitive. </li>
<li> there is no access control in place or it does not cover all sensitive actions and resources. </li>
<li> users have permissions they don't need. </li>
<li> the access control is based on a user input or on some other unsafe data. </li>
<li> permissions are difficult to remove or take a long time to be updated. </li>
</ul>
<p>You are at risk if you answered yes to the first question and any of the following ones.</p>
<h2>Recommended Secure Coding Practices</h2>
<p>The first step is to restrict all sensitive actions to authenticated users.</p>
<p>Each user should have the lowest privileges possible. The access control granularity should match the sensitivity of each resource or action. The
more sensitive it is, the less people should have access to it. </p>
<p>Do not base the access control on a user input or on a value which might have been tampered with. For example, the developer should not read a
user's permissions from an HTTP cookie as it can be modified client-side.</p>
<p>Check that the access to each action and resource is properly restricted.</p>
<p>Enable administrators to swiftly remove permissions when necessary. This enables them to reduce the time an attacker can have access to your
systems when a breach occurs.</p>
<p>Log and monitor refused access requests as they can reveal an attack.</p>
<h2>Questionable Code Example</h2>
<pre>
using System.Threading;
using System.Security.Permissions;
using System.Security.Principal;
using System.IdentityModel.Tokens;

class SecurityPrincipalDemo
{
class MyIdentity : IIdentity // Questionable, custom IIdentity implementations should be reviewed
{
// ...
}

class MyPrincipal : IPrincipal // Questionable, custom IPrincipal implementations should be reviewed
{
// ...
}
[System.Security.Permissions.PrincipalPermission(SecurityAction.Demand, Role = "Administrators")] // Questionable. The access restrictions enforced by this attribute should be reviewed.
static void CheckAdministrator()
{
WindowsIdentity MyIdentity = WindowsIdentity.GetCurrent(); // Questionable
HttpContext.User = ...; // Questionable: review all reference (set and get) to System.Web HttpContext.User
AppDomain domain = AppDomain.CurrentDomain;
domain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal); // Questionable
MyIdentity identity = new MyIdentity(); // Questionable
MyPrincipal MyPrincipal = new MyPrincipal(MyIdentity); // Questionable
Thread.CurrentPrincipal = MyPrincipal; // Questionable
domain.SetThreadPrincipal(MyPrincipal); // Questionable

// All instantiation of PrincipalPermission should be reviewed.
PrincipalPermission principalPerm = new PrincipalPermission(null, "Administrators"); // Questionable
principalPerm.Demand();

SecurityTokenHandler handler = ...;
// Questionable: this creates an identity.
ReadOnlyCollection&lt;ClaimsIdentity&gt; identities = handler.ValidateToken(…);
}

// Questionable: review how this function uses the identity and principal.
void modifyPrincipal(MyIdentity identity, MyPrincipal principal)
{
// ...
}
}
</pre>
<h2>See</h2>
<ul>
<li> OWASP Top 10 2017 Category A5 - Broken Access Control </li>
<li> <a href="https://www.sans.org/top25-software-errors/#cat3">SANS Top 25</a> - Porous Defenses </li>
</ul>

21 changes: 21 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S4834_c#.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"title": "Controlling permissions is security-sensitive",
"type": "SECURITY_HOTSPOT",
"status": "ready",
"tags": [
"sans-top25-porous",
"owasp-a5"
],
"standards": [
"OWASP Top Ten"
],
"defaultSeverity": "Critical",
"ruleSpecification": "RSPEC-4834",
"sqKey": "S4834",
"scope": "Main",
"securityStandards": {
"OWASP": [
"A5"
]
}
}
3 changes: 2 additions & 1 deletion sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@
"S4818",
"S4823",
"S4825",
"S4829"
"S4829",
"S4834"
]
}
Loading