(S3): Rule S3_BUCKET_SSL_REQUESTS_ONLY is overly permissive

[fabiodouek]

What is the problem?

The rule https://github.com/aws-cloudformation/aws-guard-rules-registry/blob/main/rules/aws/amazon_s3/s3_bucket_ssl_requests_only.guard is overly permissive.

Following the main points:

• The rule checks only for Effect==Deny and Condition Bool.'aws:SecureTransport' == false. It should also be checking for all the attributes specified in the error message: Fix: Set a bucket policy statement to '"Action":"s3:","Effect":"Deny","Principal":"","Resource":"*","Condition":{"Bool":{"aws:SecureTransport":false}}'
• When evaluating a Deny statement, evaluating a single Condition key is not enough. The reason for that is if there is a second condition the statement might not be evaluated to true
• "*" it's not a valid Resource for S3 Bucket

Reproduction Steps

Example below specifying an Action in the Deny to force a no match rule. The same applies for Principal, Resource.

- name: S3 Bucket Policy statement only allows requests to use Secure Socket Layer (SSL), FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:AbortMultipartUpload"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                Resource: "*"
  
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

Example below specifying an additional Condition so the statement returns false

- name: S3 Bucket Policy statement only allows requests to use Secure Socket Layer (SSL), FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:*"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                  StringEquals:
                     "aws:PrincipalAccount": "123456789012"
                Resource: "*"
  
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

What did you expect to happen?

I expected the rule to provide the intended guardrail for all scenarios.

What actually happened?

The rule is overly permissive, it does not provide its main objective.

CloudFormation Guard Version

2.1.3

OS

Amazon Linux

OS Version

No response

Other information

The rule should have additional assertions to cover the possible scenarios.

[grolston]

Thank you for submitting this issue. We are reviewing on our side with the tests you provided.

[grolston]

@fabiodouek looking at this we can require that it has the Action, Resource * and Principal * in the statement to tighten down. Have you tested anything similar that meets the requirements?

[fabiodouek]

Hi @grolston , the Conditions also have to be tweaked.

Unfortunately it seems to be a challenge in Guard to do a dict exact match in a simple way. So it has to check the non-presence of the unexpected attributes. Following an example of a rule and tests tweaked. Maybe there is a way to simplify this?

Rule

#
#####################################
##           Gherkin               ##
#####################################
# Rule Identifier:
#    S3_BUCKET_SSL_REQUESTS_ONLY
#
# Description:
#   Checks if Amazon S3 buckets have policies that require requests to use Secure Socket Layer (SSL).
#
# Reports on:
#    AWS::S3::BucketPolicy
#
# Evaluates:
#    AWS CloudFormation
#
# Rule Parameters:
#    NA
#
# Scenarios:
# a) SKIP: when there are no S3 Bucket Policy Document resource present
# b) PASS: when all S3 Bucket Policy Document set to deny if condition SecureTransport not true
# c) FAIL: when all S3 Bucket Policy Document does not have deny on insecure transport actions
# d) SKIP: when metadata includes the suppression for rule S3_BUCKET_SSL_REQUESTS_ONLY

#
# Select all S3 resources from incoming template (payload)
#
let s3_buckets_policies_ssl_requests_only = Resources.*[ Type == 'AWS::S3::BucketPolicy'
  Metadata.guard.SuppressedRules not exists or
  Metadata.guard.SuppressedRules.* != "S3_BUCKET_SSL_REQUESTS_ONLY"
]

# Select secure S3 Bucket Policy resources from incoming template
let ssl_secure_bucket_policies = %s3_buckets_policies_ssl_requests_only[
  Properties.PolicyDocument {
    some Statement[*] {
      Effect == 'Deny'
      Action == 's3:*'
      Condition[ keys != 'Bool'] empty
      Condition[ keys == 'Bool'] not empty {
        this[ keys == /(?i)^aws:SecureTransport$/ ] == false
        this[ keys != /(?i)^aws:SecureTransport$/ ] !exists
      }
      some Resource == /^[^\/\*]*$/
      some Resource == /^[^\/]*\/\*$/
      Resource IN [/^[^\/\*]*$/, /^[^\/]*\/\*$/]
    }
  }
]

rule S3_BUCKET_SSL_REQUESTS_ONLY when %s3_buckets_policies_ssl_requests_only !empty {
  %ssl_secure_bucket_policies !empty
  <<
    Violation: Bucket policies must feature a statement to enforce TLS usage.
    Fix: Set a bucket policy statement to '"Action":"s3:*","Effect":"Deny","Principal":"*","Resource":"*","Condition":{"Bool":{"aws:SecureTransport":false}}' .
  >>
}

Tests

###
# S3_BUCKET_SSL_REQUESTS_ONLY tests
###
---
- name: Empty, SKIP
  input: {}
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: SKIP

- name: No resources, SKIP
  input:
    Resources: {}
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: SKIP

- name: S3 Bucket Policy statement only allows requests to use Secure Socket Layer (SSL), PASS
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:*"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"

  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS

- name: S3 Bucket Policy statement does not allow requests to use Secure Socket Layer (SSL), FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Sid: "DenyNoSSL"
                Effect: "Allow"
                Principal: "*"
                Action: "s3:*"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: S3 Bucket Policy statement does not allow requests to use Secure Socket Layer (SSL), FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Action: "s3:*"
                Effect: "Deny"
                Principal: "*"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  Bool:
                    "aws:SecureTransport": true
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: S3 Bucket Policy statement to only allow requests to use Secure Socket Layer (SSL) missing, FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Sid: "AWSLogDeliveryWrite"
                Effect: "Allow"
                Principal:
                  Service:
                    - "delivery.logs.amazonaws.com"
                Action: "s3:PutObject"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  StringEquals:
                    "s3:x-amz-acl": "bucket-owner-full-control"
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: S3 Bucket Policy statement to only allow requests to use Secure Socket Layer (SSL) missing but rule suppressed, SKIP
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Metadata:
          guard:
            SuppressedRules:
            - S3_BUCKET_SSL_REQUESTS_ONLY
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Sid: "AWSLogDeliveryWrite"
                Effect: "Allow"
                Principal:
                  Service:
                    - "delivery.logs.amazonaws.com"
                Action: "s3:PutObject"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  StringEquals:
                    "s3:x-amz-acl": "bucket-owner-full-control"
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: SKIP

- name: S3 Bucket Policy statement less literal test 2, PASS
  input:
    Resources:
      Bucket:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS

- name: S3 Bucket Policy statement less literal test 3, PASS
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: DeleteProtection
                Action: s3:Delete*
                Effect: Deny
                Principal: '*'
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - !GetAtt Bucket.Arn
                  - !Sub "${Bucket.Arn}/*"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS

###### New tests
- name: Trying to trick, retricted action , FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:AbortMultipartUpload"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                Resource: "*"
  
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: Trying to trick, added new Condition Operator, FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:*"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                  StringEquals:
                     "aws:PrincipalAccount": "123456789012"
                Resource: "*"
  
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: Trying to trick, added new Condition key, FAIL
  input:
    Resources:
      ExampleS3:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref rLogsBucket
          PolicyDocument:
            Version: "2012-10-17"
            Statement:
              - Principal: "*"
                Action: "s3:*"
                Effect: "Deny"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
                    "aws:AddedNewKey": false
                Resource: "*"
  
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

  
- name: Trying to trick Key upper case, PASS
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/*
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET
                Condition:
                  Bool:
                    "aws:SECURETRANSPORT": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS

- name: Resource not list, FAIL
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource: "*"
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL


- name: Correct Resource, PASS
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/*
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS

- name: Correct Resource, PASS
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/*
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: PASS


- name: Incorrect Resource added /, FAIL
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/xxx/*
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

- name: Incorrect Resource added / 2, FAIL
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/*
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL     

- name: 2 correct resources, 1 incorrect, FAIL
  input:
    Resources:
      Bucket:
        Metadata:
          guard:
        Type: AWS::S3::Bucket
      BucketPolicy:
        Type: AWS::S3::BucketPolicy
        Properties:
          Bucket: !Ref Bucket
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Sid: EnforceTls
                Effect: Deny
                Action: "s3:*"
                Principal: "*"
                Resource:
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/*
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET
                  - arn:aws:s3:::DOC-EXAMPLE-BUCKET/
                Condition:
                  Bool:
                    "aws:SecureTransport": false
  expectations:
    rules:
      S3_BUCKET_SSL_REQUESTS_ONLY: FAIL

[grolston]

I ran into the exact same issue: "Unfortunately it seems to be a challenge in Guard to do a dict exact match in a simple way"

My first pass through I made it pretty rigid by defining the exact match. Let me take a look at what you have here and see if that works out.

[grolston]

@fabiodouek this looks fine. Is there any areas you are looking for improvements on this check? Would you mind opening up a pull-request for this?

[grolston]

@fabiodouek looking at what you have above, we are looking at adding the following to the check:

# Select secure S3 Bucket Policy resources from incoming template
let ssl_secure_bucket_policies = %s3_buckets_policies_ssl_requests_only[
  Properties.PolicyDocument {
    some Statement[*] {
      Principal == "*"
      Action == "s3:*"
      Effect == 'Deny'
      Condition {
        Bool.'aws:SecureTransport' == false
      }
    }
  }
]

rule S3_BUCKET_SSL_REQUESTS_ONLY when %s3_buckets_policies_ssl_requests_only !empty {
  %ssl_secure_bucket_policies !empty
  <<
    Violation: Bucket policies must feature a statement to enforce TLS usage.
    Fix: Set a bucket policy statement to '"Action":"s3:*","Effect":"Deny","Principal":"*","Resource":"*","Condition":{"Bool":{"aws:SecureTransport":false}}' .
  >>
}

/cc @akshayrane

[fabiodouek]

Hi @grolston , @akshayrane ,
Is there a reason to lower the bar for this rule?

The enforcement in the rule you are presenting does not achieve the desired outcome and it cannot be trusted.
If you run it against the unit tests I've provided, many of these tests will not pass.

On the other hand, the rule I've provided does a strict check and would prevent the rule to be bypassed either intentionally or unintentionally.

In case you decide to go with a more relaxed rule, I would suggest to add a disclaimer saying that the rule does not prevent to be bypassed, as there are customers trusting that the rules in this repo are reliable.

Regards,
Fabio.