Skip to content

fix(s3-deployment): sanitize all log outputs to mitigate CWE-117/93#37670

Open
jonmiller-iv wants to merge 4 commits into
aws:mainfrom
jonmiller-iv:fix/s3-deployment-log-injection-cwe-117-93
Open

fix(s3-deployment): sanitize all log outputs to mitigate CWE-117/93#37670
jonmiller-iv wants to merge 4 commits into
aws:mainfrom
jonmiller-iv:fix/s3-deployment-log-injection-cwe-117-93

Conversation

@jonmiller-iv
Copy link
Copy Markdown

@jonmiller-iv jonmiller-iv commented Apr 22, 2026

Issue

Closes #37671.

Reason for this change

The partial fix in #30746 only applied sanitize_message() to 2 log statements (s3_dest and old_s3_dest). AWS Inspector continues to flag the CustomCDKBucketDeployment Lambda with CWE-117/93 findings because several other log outputs still use unsanitized user-controlled input.

Related prior issues: #28469, #30211

Description of changes

  • Extended sanitize_message() usage to all remaining unsanitized log outputs in index.py:
    • cfn_error — now uses sanitize_message() instead of message.encode()
    • Event dict logging — wrapped with sanitize_message()
    • aws_command — CLI args logged via sanitize_message()
    • cfn_send — response body logged via sanitize_message()
  • Updated sanitize_message() to handle non-string inputs (dicts, lists) by converting to string before sanitizing

Description of how you validated changes

  • Updated test_error_logger and test_error_logger_encoding_input to match new sanitized output format
  • Added test_error_logger_crlf_injection — verifies \r\n characters are stripped from error log output
  • Added test_sanitize_message_non_string — verifies dict inputs are converted and sanitized
  • Added test_sanitize_message_none — verifies None passthrough
  • All existing tests continue to pass

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@jonmiller-iv
Copy link
Copy Markdown
Author

Exemption Request

This fix modifies only the Python runtime handler (index.py) for the S3 BucketDeployment custom resource — specifically sanitizing log outputs to mitigate CWE-117/93. It does not change any CDK construct behavior, CloudFormation resource configuration, or synthesized template output.

An integration test would not validate this change, as the fix affects runtime log sanitization only (no change to deployed infrastructure). Unit tests have been added and updated to cover the new sanitization behavior.

The prior merged fix for this same issue (#30746) also only modified index.py and test.py without integration test changes.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Apr 22, 2026
@pahud
Copy link
Copy Markdown
Contributor

pahud commented May 12, 2026

bump to p1 in consist with the issue.

@pahud pahud added p1 and removed p2 labels May 12, 2026
@jonmiller-iv
Copy link
Copy Markdown
Author

@
The 15 integ snapshot failures in the build are all asset-hash drift from the index.py change — every failing test is a direct or transitive consumer of Custom::CDKBucketDeployment, and the snapshot S3 keys embed the old Lambda asset hash. Reviewed the diff: all six edits are pure log-format changes (replacing .encode() and unsanitized inputs with sanitize_message()); no CFN response data, S3 operations, or subprocess arguments are affected.

Affected tests:

  • aws-s3-deployment/test/integ.bucket-deployment* (5)
  • aws-codepipeline-actions/test/integ.cross-account-pipeline-*, integ.pipeline-{commands,ecr-image-scan-action,elastic-beanstalk-deploy} (5)
  • aws-dynamodb/test/integ.import-source
  • aws-ecs/test/ec2/integ.environment-file
  • aws-servicecatalog/test/integ.{product,two-products}
  • pipelines/test/integ.cross-account-pipeline-action

I don't have a CDK integ-test deploy environment set up. "Allow edits from maintainers" is enabled — happy to have someone regen via the internal pipeline, or guide me through the right exemption flow.
@

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p1 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(s3-deployment): CWE-117/93 log injection finding persists after partial fix in #30746

3 participants