Skip to content

Commit dbd7115

Browse files
marco-bearzi-sonarsourcemarco-bearzi-sonarsource
authored
SONARPY-1011 Rule S6265: Granting access to S3 buckets to all or authenticated users is security-sensitive (#1129)
1 parent a92edcc commit dbd7115

File tree

9 files changed

+309
-2
lines changed

9 files changed

+309
-2
lines changed

‎python-checks/src/main/java/org/sonar/python/checks/CheckList.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.HashSet;
2525

2626
import org.sonar.python.checks.cdk.S3BucketBlockPublicAccessCheck;
27+
import org.sonar.python.checks.cdk.S3BucketGrantedAccessCheck;
2728
import org.sonar.python.checks.cdk.S3BucketServerEncryptionCheck;
2829
import org.sonar.python.checks.cdk.S3BucketVersioningCheck;
2930
import org.sonar.python.checks.hotspots.ClearTextProtocolsCheck;
@@ -229,6 +230,7 @@ public static Iterable<Class> getChecks() {
229230
ReturnYieldOutsideFunctionCheck.class,
230231
RobustCipherAlgorithmCheck.class,
231232
S3BucketBlockPublicAccessCheck.class,
233+
S3BucketGrantedAccessCheck.class,
232234
S3BucketServerEncryptionCheck.class,
233235
S3BucketVersioningCheck.class,
234236
SameBranchCheck.class,

‎python-checks/src/main/java/org/sonar/python/checks/cdk/AbstractS3BucketCheck.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@
3737

3838
public abstract class AbstractS3BucketCheck extends PythonSubscriptionCheck {
3939

40-
private static final String S3_BUCKET_FQN = "aws_cdk.aws_s3.Bucket";
40+
protected static final String S3_BUCKET_FQN = "aws_cdk.aws_s3.Bucket";
4141

4242
@Override
4343
public void initialize(Context context) {
4444
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::visitNode);
4545
}
4646

47-
private void visitNode(SubscriptionContext ctx) {
47+
protected void visitNode(SubscriptionContext ctx) {
4848
CallExpression node = (CallExpression) ctx.syntaxNode();
4949
Optional.ofNullable(node.calleeSymbol())
5050
.map(Symbol::fullyQualifiedName)
@@ -80,6 +80,7 @@ private static ArgumentTrace build(SubscriptionContext ctx, RegularArgument argu
8080
buildTrace(argument.expression(), trace);
8181
return new ArgumentTrace(ctx, trace);
8282
}
83+
8384
private static void buildTrace(Expression expression, List<Expression> trace) {
8485
trace.add(expression);
8586
if (expression.is(Tree.Kind.NAME)) {

‎python-checks/src/main/java/org/sonar/python/checks/cdk/S3BucketGrantedAccessCheck.java

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2022 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks.cdk;
21+
22+
import java.util.Arrays;
23+
import java.util.List;
24+
import java.util.Optional;
25+
import org.sonar.check.Rule;
26+
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.symbols.Symbol;
28+
import org.sonar.plugins.python.api.tree.CallExpression;
29+
import org.sonar.plugins.python.api.tree.Expression;
30+
import org.sonar.plugins.python.api.tree.ImportFrom;
31+
import org.sonar.plugins.python.api.tree.Name;
32+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
33+
import org.sonar.plugins.python.api.tree.Tree;
34+
35+
@Rule(key = "S6265")
36+
public class S3BucketGrantedAccessCheck extends AbstractS3BucketCheck {
37+
38+
public static final String MESSAGE_POLICY = "Make sure granting %s access is safe here.";
39+
public static final String MESSAGE_GRANT = "Make sure allowing unrestricted access to objects from this bucket is safe here.";
40+
private static final String S3_BUCKET_DEPLOYMENT_FQN = "aws_cdk.aws_s3_deployment.BucketDeployment";
41+
private static final String S3_BUCKET_AUTHENTICATED_READ = "aws_cdk.aws_s3.BucketAccessControl.AUTHENTICATED_READ";
42+
private static final String S3_BUCKET_PUBLIC_READ = "aws_cdk.aws_s3.BucketAccessControl.PUBLIC_READ";
43+
private static final String S3_BUCKET_PUBLIC_READ_WRITE = "aws_cdk.aws_s3.BucketAccessControl.PUBLIC_READ_WRITE";
44+
private static final List<String> S3_BUCKET_FQNS = Arrays.asList(S3_BUCKET_FQN, S3_BUCKET_DEPLOYMENT_FQN);
45+
private static final List<String> S3_BUCKET_SENSITIVE_POLICIES = Arrays.asList(S3_BUCKET_AUTHENTICATED_READ, S3_BUCKET_PUBLIC_READ, S3_BUCKET_PUBLIC_READ_WRITE);
46+
47+
private boolean isAwsCdkImported = false;
48+
49+
@Override
50+
public void initialize(Context context) {
51+
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, ctx -> isAwsCdkImported = false);
52+
context.registerSyntaxNodeConsumer(Tree.Kind.IMPORT_FROM, this::checkAWSImport);
53+
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::visitNode);
54+
}
55+
56+
private void checkAWSImport(SubscriptionContext ctx) {
57+
ImportFrom imports = (ImportFrom) ctx.syntaxNode();
58+
Optional.ofNullable(imports.module())
59+
.filter(dottedName -> dottedName.names()
60+
.stream()
61+
.map(Name::name)
62+
.anyMatch("aws_cdk"::equals))
63+
.ifPresent(n -> isAwsCdkImported = true);
64+
}
65+
66+
@Override
67+
protected void visitNode(SubscriptionContext ctx) {
68+
CallExpression node = (CallExpression) ctx.syntaxNode();
69+
Optional<Symbol> symbol = Optional.ofNullable(node.calleeSymbol());
70+
71+
symbol
72+
.map(Symbol::fullyQualifiedName)
73+
.filter(S3_BUCKET_FQNS::contains)
74+
.ifPresent(s -> visitBucketConstructor(ctx, node));
75+
76+
if (isAwsCdkImported) {
77+
symbol
78+
.map(Symbol::name)
79+
.filter("grant_public_access"::equals)
80+
.ifPresent(s -> ctx.addIssue(node.callee(), MESSAGE_GRANT));
81+
}
82+
}
83+
84+
@Override
85+
void visitBucketConstructor(SubscriptionContext ctx, CallExpression bucket) {
86+
getArgument(ctx, bucket, "access_control")
87+
.ifPresent(argument -> argument.addIssueIf(this::isSensitivePolicy, getSensitivePolicyMessage(argument)));
88+
}
89+
90+
protected boolean isSensitivePolicy(Expression expression) {
91+
return Optional.ofNullable(expression)
92+
.filter(QualifiedExpression.class::isInstance)
93+
.map(QualifiedExpression.class::cast)
94+
.map(QualifiedExpression::symbol)
95+
.map(Symbol::fullyQualifiedName)
96+
.filter(S3_BUCKET_SENSITIVE_POLICIES::contains)
97+
.isPresent();
98+
}
99+
100+
private static String getSensitivePolicyMessage(ArgumentTrace argumentTrace){
101+
Expression lastExpression = argumentTrace.trace()
102+
.get(argumentTrace.trace().size()-1);
103+
String attribute = ((QualifiedExpression) lastExpression).name().name();
104+
return String.format(MESSAGE_POLICY, attribute);
105+
}
106+
107+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6265.html

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<p>Predefined permissions, also known as <a href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl">canned ACLs</a>,
2+
are an easy way to grant large privileges to predefined groups or users.</p>
3+
<p>The following canned ACLs are security-sensitive:</p>
4+
<ul>
5+
<li> <code>PUBLIC_READ</code>, <code>PUBLIC_READ_WRITE</code> grant respectively "read" and "read and write" privileges to everyone in the world
6+
(<code>AllUsers</code> group). </li>
7+
<li> <code>AUTHENTICATED_READ</code> grants "read" privilege to all authenticated users (<code>AuthenticatedUsers</code> group). </li>
8+
</ul>
9+
<h2>Ask Yourself Whether</h2>
10+
<ul>
11+
<li> The S3 bucket stores sensitive data. </li>
12+
<li> The S3 bucket is not used to store static resources of websites (images, css …​). </li>
13+
</ul>
14+
<p>There is a risk if you answered yes to any of those questions.</p>
15+
<h2>Recommended Secure Coding Practices</h2>
16+
<p>It’s recommended to implement the least privilege policy, i.e., to grant necessary permissions only to users for their required tasks. In the
17+
context of canned ACL, set it to <code>PRIVATE</code> (the default one), and if needed more granularity then use an appropriate S3 policy.</p>
18+
<h2>Sensitive Code Example</h2>
19+
<p>All users (ie: anyone in the world authenticated or not) have read and write permissions with the <code>PUBLIC_READ_WRITE</code> access
20+
control:</p>
21+
<pre>
22+
bucket = s3.Bucket(self,
23+
"bucket",
24+
access_control=s3.BucketAccessControl.PUBLIC_READ_WRITE # Sensitive
25+
)
26+
27+
# Another example
28+
s3deploy.BucketDeployment(self,
29+
"DeployWebsite",
30+
...,
31+
access_control=s3.BucketAccessControl.PUBLIC_READ_WRITE # Sensitive
32+
)
33+
</pre>
34+
<h2>Compliant Solution</h2>
35+
<p>With the <code>PRIVATE</code> access control (default), only the bucket owner has the read/write permissions on the buckets and its ACL.</p>
36+
<pre>
37+
bucket = s3.Bucket(self, "bucket",
38+
access_control=s3.BucketAccessControl.PRIVATE # Compliant
39+
)
40+
41+
# Another example
42+
s3deploy.BucketDeployment(self, "DeployWebsite",
43+
access_control=s3.BucketAccessControl.PRIVATE # Compliant
44+
)
45+
</pre>
46+
<h2>See</h2>
47+
<ul>
48+
<li> <a href="https://owasp.org/Top10/A01_2021-Broken_Access_Control/">OWASP Top 10 2021 Category A1</a> - Broken Access Control </li>
49+
<li> <a href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl">AWS Documentation</a> - Access control list (ACL)
50+
overview (canned ACLs) </li>
51+
<li> <a href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/walkthrough1.html">AWS Documentation</a> - Controlling access to a bucket with
52+
user policies </li>
53+
<li> <a href="https://cwe.mitre.org/data/definitions/732">MITRE, CWE-732</a> - Incorrect Permission Assignment for Critical Resource </li>
54+
<li> <a href="https://cwe.mitre.org/data/definitions/284">MITRE, CWE-284</a> - Improper Access Control </li>
55+
<li> <a href="https://owasp.org/www-project-top-ten/2017/A5_2017-Broken_Access_Control">OWASP Top 10 2017 Category A5</a> - Broken Access Control
56+
</li>
57+
<li> <a href="https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.Bucket.html">AWS CDK version 2</a> - Class Bucket (construct) </li>
58+
</ul>
59+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6265.json

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
{
2+
"title": "Granting access to S3 buckets to all or authenticated users is security-sensitive",
3+
"type": "SECURITY_HOTSPOT",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"aws",
11+
"cwe",
12+
"owasp-a5"
13+
],
14+
"defaultSeverity": "Blocker",
15+
"ruleSpecification": "RSPEC-6265",
16+
"sqKey": "S6265",
17+
"scope": "Main",
18+
"securityStandards": {
19+
"CWE": [
20+
284,
21+
732
22+
],
23+
"OWASP": [
24+
"A5"
25+
],
26+
"CIS": [
27+
"3.3"
28+
],
29+
"OWASP Top 10 2021": [
30+
"A1"
31+
]
32+
}
33+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@
154154
"S6035",
155155
"S6245",
156156
"S6252",
157+
"S6265",
157158
"S6281",
158159
"S6323",
159160
"S6326",

‎python-checks/src/test/java/org/sonar/python/checks/cdk/S3BucketGrantedAccessCheckTest.java

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2022 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks.cdk;
21+
22+
import org.junit.Test;
23+
import org.sonar.python.checks.utils.PythonCheckVerifier;
24+
25+
public class S3BucketGrantedAccessCheckTest {
26+
27+
@Test
28+
public void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/cdk/s3BucketGrantedAccess.py", new S3BucketGrantedAccessCheck());
30+
}
31+
32+
@Test
33+
public void noImport() {
34+
PythonCheckVerifier.verifyNoIssue("src/test/resources/checks/cdk/s3BucketGrantedAccess_noImport.py", new S3BucketGrantedAccessCheck());
35+
}
36+
37+
}

‎python-checks/src/test/resources/checks/cdk/s3BucketGrantedAccess.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
from aws_cdk import aws_s3 as s3, aws_s3_deployment as s3deploy
2+
3+
4+
bucket = s3.Bucket(self, "bucket") # Compliant by default
5+
bucket.grant_public_access() # NonCompliant
6+
7+
def grant_noncompliant():
8+
s1 = s3.Bucket(self, "BucketToDeploy")
9+
s2 = s1
10+
s2.grant_public_access() # NonCompliant {{Make sure allowing unrestricted access to objects from this bucket is safe here.}}
11+
# ^^^^^^^^^^^^^^^^^^^^^^
12+
13+
foo = Foo()
14+
# FP : due to the fact we check whether aws_cdk is imported and the below function is called
15+
foo.grant_public_access() # NonCompliant {{Make sure allowing unrestricted access to objects from this bucket is safe here.}}
16+
17+
bucket = s3.Bucket(self, "bucket",
18+
access_control=s3.bar.WHAT_EVER # Compliant
19+
)
20+
21+
bucket = s3.Bucket(self, "bucket",
22+
access_control=s3.BucketAccessControl.PRIVATE # Compliant
23+
)
24+
25+
bucket = s3.Bucket(self, "bucket",
26+
access_control=s3.BucketAccessControl.PUBLIC_READ_WRITE # NonCompliant {{Make sure granting PUBLIC_READ_WRITE access is safe here.}}
27+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
28+
)
29+
30+
bucket = s3.Bucket(self, "bucket",
31+
access_control=s3.BucketAccessControl.PUBLIC_READ # NonCompliant {{Make sure granting PUBLIC_READ access is safe here.}}
32+
)
33+
34+
bucket = s3.Bucket(self, "bucket",
35+
access_control=s3.BucketAccessControl.AUTHENTICATED_READ # NonCompliant {{Make sure granting AUTHENTICATED_READ access is safe here.}}
36+
)
37+
38+
def create_public_bucket():
39+
control1 = s3.BucketAccessControl.PUBLIC_READ
40+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^> {{Propagated setting.}}
41+
bucket = s3.Bucket(self, "bucket",
42+
access_control=control1 # NonCompliant {{Make sure granting PUBLIC_READ access is safe here.}}
43+
# ^^^^^^^^^^^^^^^^^^^^^^^
44+
)
45+
46+
47+
bucket_to_deplay = s3.Bucket(self, "BucketToDeploy")
48+
49+
s3deploy.BucketDeployment(self, "Deploy", # Compliant
50+
sources=[s3deploy.Source.asset("./deploy-dist")],
51+
destination_bucket=bucket_to_deploy
52+
)
53+
54+
s3deploy.BucketDeployment(self, "Deploy2",
55+
destination_bucket=bucket_to_deploy,
56+
access_control=s3.BucketAccessControl.PUBLIC_READ_WRITE # NonCompliant {{Make sure granting PUBLIC_READ_WRITE access is safe here.}}
57+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
58+
)
59+
60+
s3deploy.BucketDeployment(self, "Deploy3",
61+
destination_bucket=bucket_to_deploy,
62+
access_control=s3.BucketAccessControl.PRIVATE # Compliant
63+
)
64+

‎python-checks/src/test/resources/checks/cdk/s3BucketGrantedAccess_noImport.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
def grant_compliant():
2+
foo = Foo()
3+
foo.grant_public_access() # Compliant since no aws_cdk import

0 commit comments

Comments
 (0)