From 5df4dcc911bb790446176ed5c7581432b5d4dbdf Mon Sep 17 00:00:00 2001 From: Nick Lynch Date: Tue, 16 Feb 2021 18:10:24 +0000 Subject: [PATCH] fix(cloudfront): bucket policy for Origin Access Identities is overly permissive For both `CloudFrontWebDistribution` and `Distribution`, if an origin access identity (OAI) is used to access a S3 bucket origin, the default `bucket.grantRead()` method is used to grant read access to the bucket for the OAI. `grantRead` will create a statement in the bucket policy that grants: `s3:GetObject*`, `s3:GetBucket*`, and `s3:List*` to the OAI. The (official documentation)[https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-restricting-access-to-s3.html] in the developer guide states that to allow the OAI to read objects in the bucket, only `s3:GetObject` is necessary. The impact of the additional permissions is that if a user navigates to the root of the distribution (e.g., https://d12345abcdef.cloudfront.net/) *AND* no default root object (e.g., index.html) is set for the distribution, the user will receive an XML list of the objects in the bucket. This is likely undesired behavior, and potential information leakage. This same policy has been the behavior for OAI + S3 buckets since the first introduction of the `CloudFrontWebDistribution` construct years ago. However, the `CloudFrontWebDistribution` also always defaults the root object to 'index.html' (see #3486 for discussion on that). The side-effect of this is that unless the user selects to use an OAI and overrides the default root object to an empty string, visiting the root will result either in valid content or an AccessDenied message. However, since the `Distribution` constructs do *not* default the root object, exposing the bucket index becomes much more likely. This change restricts the permissions granted to the OAI user to just the necessary `s3:GetObject` permissions. fixes #13086 --- **NOTE:** This PR corrects the behavior without introducing a new feature flag. Any customers who rely on the bucket listing behavior would be broken by this change. However, I anticipate that represents a very small minority, versus the number of customers who may accidentally expose their bucket contents. I would very much like to have a discussion about whether this is the correct choice. --- .../aws-cloudfront-origins/lib/s3-origin.ts | 14 ++++- .../aws-cloudfront-origins/package.json | 6 ++- .../test/integ.origin-group.expected.json | 40 +++++--------- .../test/integ.s3-origin.expected.json | 40 +++++--------- .../test/s3-origin.test.ts | 4 ++ .../aws-cloudfront/lib/web-distribution.ts | 12 +++-- .../test/integ.cloudfront-s3.expected.json | 40 +++++--------- .../test/web-distribution.test.ts | 54 +++++++++++++++++++ 8 files changed, 125 insertions(+), 85 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts b/packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts index 518e3e728ec1a..a0ac4037d9741 100644 --- a/packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts +++ b/packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts @@ -1,4 +1,5 @@ import * as cloudfront from '@aws-cdk/aws-cloudfront'; +import * as iam from '@aws-cdk/aws-iam'; import * as s3 from '@aws-cdk/aws-s3'; import * as cdk from '@aws-cdk/core'; import { HttpOrigin } from './http-origin'; @@ -68,7 +69,7 @@ class S3BucketOrigin extends cloudfront.OriginBase { public bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig { if (!this.originAccessIdentity) { // Using a bucket from another stack creates a cyclic reference with - // the bucket taking a dependency on the generated S3CanonicalUserId when `grantRead` is called, + // the bucket taking a dependency on the generated S3CanonicalUserId for the grant principal, // and the distribution having a dependency on the bucket's domain name. // Fix this by parenting the OAI in the bucket's stack when cross-stack usage is detected. const bucketStack = cdk.Stack.of(this.bucket); @@ -79,7 +80,16 @@ class S3BucketOrigin extends cloudfront.OriginBase { this.originAccessIdentity = new cloudfront.OriginAccessIdentity(oaiScope, oaiId, { comment: `Identity for ${options.originId}`, }); - this.bucket.grantRead(this.originAccessIdentity); + + // Used rather than `grantRead` because `grantRead` will grant overly-permissive policies. + // Only GetObject is needed to retrieve objects for the distribution. + // This also excludes KMS permissions; currently, OAI only supports SSE-S3 for buckets. + // Source: https://aws.amazon.com/blogs/networking-and-content-delivery/serving-sse-kms-encrypted-content-from-s3-using-cloudfront/ + this.bucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [this.bucket.arnForObjects('*')], + actions: ['s3:GetObject'], + principals: [this.originAccessIdentity.grantPrincipal], + })); } return super.bind(scope, options); } diff --git a/packages/@aws-cdk/aws-cloudfront-origins/package.json b/packages/@aws-cdk/aws-cloudfront-origins/package.json index 01046b4b76db3..2a74a796cffdc 100644 --- a/packages/@aws-cdk/aws-cloudfront-origins/package.json +++ b/packages/@aws-cdk/aws-cloudfront-origins/package.json @@ -79,6 +79,7 @@ "dependencies": { "@aws-cdk/aws-cloudfront": "0.0.0", "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0", + "@aws-cdk/aws-iam": "0.0.0", "@aws-cdk/aws-s3": "0.0.0", "@aws-cdk/core": "0.0.0", "constructs": "^3.2.0" @@ -88,8 +89,9 @@ "@aws-cdk/core": "0.0.0", "constructs": "^3.2.0", "@aws-cdk/aws-cloudfront": "0.0.0", - "@aws-cdk/aws-s3": "0.0.0", - "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0" + "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0", + "@aws-cdk/aws-iam": "0.0.0", + "@aws-cdk/aws-s3": "0.0.0" }, "engines": { "node": ">= 10.13.0 <13 || >=13.7.0" diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json b/packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json index 8e123f87338d4..ecd2096329404 100644 --- a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json +++ b/packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json @@ -14,11 +14,7 @@ "PolicyDocument": { "Statement": [ { - "Action": [ - "s3:GetObject*", - "s3:GetBucket*", - "s3:List*" - ], + "Action": "s3:GetObject", "Effect": "Allow", "Principal": { "CanonicalUser": { @@ -28,28 +24,20 @@ ] } }, - "Resource": [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" - ] - }, - { - "Fn::Join": [ - "", - [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" - ] - }, - "/*" - ] + "Resource": { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "Bucket83908E77", + "Arn" + ] + }, + "/*" ] - } - ] + ] + } } ], "Version": "2012-10-17" diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json b/packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json index 5eb310b5b92d7..25461aa9cf7de 100644 --- a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json +++ b/packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json @@ -14,11 +14,7 @@ "PolicyDocument": { "Statement": [ { - "Action": [ - "s3:GetObject*", - "s3:GetBucket*", - "s3:List*" - ], + "Action": "s3:GetObject", "Effect": "Allow", "Principal": { "CanonicalUser": { @@ -28,28 +24,20 @@ ] } }, - "Resource": [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" - ] - }, - { - "Fn::Join": [ - "", - [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" - ] - }, - "/*" - ] + "Resource": { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "Bucket83908E77", + "Arn" + ] + }, + "/*" ] - } - ] + ] + } } ], "Version": "2012-10-17" diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts b/packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts index 54f9861685945..a502dd5cf4f46 100644 --- a/packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts +++ b/packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts @@ -87,9 +87,13 @@ describe('With bucket', () => { expect(stack).toHaveResourceLike('AWS::S3::BucketPolicy', { PolicyDocument: { Statement: [{ + Action: 's3:GetObject', Principal: { CanonicalUser: { 'Fn::GetAtt': ['DistOrigin1S3Origin87D64058', 'S3CanonicalUserId'] }, }, + Resource: { + 'Fn::Join': ['', [{ 'Fn::GetAtt': ['Bucket83908E77', 'Arn'] }, '/*']], + }, }], }, }); diff --git a/packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts b/packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts index f191813dada2b..a678c82ba6087 100644 --- a/packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts +++ b/packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts @@ -1027,9 +1027,15 @@ export class CloudFrontWebDistribution extends cdk.Resource implements IDistribu // first case for backwards compatibility if (originConfig.s3OriginSource.originAccessIdentity) { // grant CloudFront OriginAccessIdentity read access to S3 bucket - originConfig.s3OriginSource.s3BucketSource.grantRead( - originConfig.s3OriginSource.originAccessIdentity, - ); + // Used rather than `grantRead` because `grantRead` will grant overly-permissive policies. + // Only GetObject is needed to retrieve objects for the distribution. + // This also excludes KMS permissions; currently, OAI only supports SSE-S3 for buckets. + // Source: https://aws.amazon.com/blogs/networking-and-content-delivery/serving-sse-kms-encrypted-content-from-s3-using-cloudfront/ + originConfig.s3OriginSource.s3BucketSource.addToResourcePolicy(new iam.PolicyStatement({ + resources: [originConfig.s3OriginSource.s3BucketSource.arnForObjects('*')], + actions: ['s3:GetObject'], + principals: [originConfig.s3OriginSource.originAccessIdentity.grantPrincipal], + })); s3OriginConfig = { originAccessIdentity: `origin-access-identity/cloudfront/${originConfig.s3OriginSource.originAccessIdentity.originAccessIdentityName}`, diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json index 8f6357e72272e..72ed767994cc0 100644 --- a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json +++ b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json @@ -14,11 +14,7 @@ "PolicyDocument": { "Statement": [ { - "Action": [ - "s3:GetObject*", - "s3:GetBucket*", - "s3:List*" - ], + "Action": "s3:GetObject", "Effect": "Allow", "Principal": { "AWS": { @@ -37,28 +33,20 @@ ] } }, - "Resource": [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" + "Resource": { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "Bucket83908E77", + "Arn" + ] + }, + "/*" ] - }, - { - "Fn::Join": [ - "", - [ - { - "Fn::GetAtt": [ - "Bucket83908E77", - "Arn" - ] - }, - "/*" - ] - ] - } - ] + ] + } } ], "Version": "2012-10-17" diff --git a/packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts b/packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts index 8eea2cad5bc92..d24c7aa1220dc 100644 --- a/packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts +++ b/packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts @@ -10,6 +10,7 @@ import { GeoRestriction, KeyGroup, LambdaEdgeEventType, + OriginAccessIdentity, PublicKey, SecurityPolicyProtocol, SSLMethod, @@ -197,6 +198,59 @@ nodeunitShim({ test.done(); }, + 'distribution with bucket and OAI'(test: Test) { + const stack = new cdk.Stack(); + const s3BucketSource = new s3.Bucket(stack, 'Bucket'); + const originAccessIdentity = new OriginAccessIdentity(stack, 'OAI'); + + new CloudFrontWebDistribution(stack, 'AnAmazingWebsiteProbably', { + originConfigs: [{ + s3OriginSource: { s3BucketSource, originAccessIdentity }, + behaviors: [{ isDefaultBehavior: true }], + }], + }); + + expect(stack).to(haveResourceLike('AWS::CloudFront::Distribution', { + DistributionConfig: { + Origins: [ + { + ConnectionAttempts: 3, + ConnectionTimeout: 10, + DomainName: { + 'Fn::GetAtt': [ + 'Bucket83908E77', + 'RegionalDomainName', + ], + }, + Id: 'origin1', + S3OriginConfig: { + OriginAccessIdentity: { + 'Fn::Join': ['', ['origin-access-identity/cloudfront/', { Ref: 'OAIE1EFC67F' }]], + }, + }, + }, + ], + }, + })); + + expect(stack).to(haveResourceLike('AWS::S3::BucketPolicy', { + PolicyDocument: { + Statement: [{ + Action: 's3:GetObject', + Principal: { + CanonicalUser: { 'Fn::GetAtt': ['OAIE1EFC67F', 'S3CanonicalUserId'] }, + }, + Resource: { + 'Fn::Join': ['', [{ 'Fn::GetAtt': ['Bucket83908E77', 'Arn'] }, '/*']], + }, + }], + }, + })); + + test.done(); + }, + + 'distribution with trusted signers on default distribution'(test: Test) { const stack = new cdk.Stack(); const sourceBucket = new s3.Bucket(stack, 'Bucket');