GHAS/CodeQL reporting missing input sanitization

[automartin5000]

Checkboxes for prior research

• I've gone through Developer Guide and API reference
• I've checked AWS Forums and StackOverflow.
• I've searched for previous similar issues and didn't find any solution.

Describe the bug

A few weeks ago, we started seeing GitHub Advanced Security alerts on Lambda functions that bundle AWS SDK code. The alert is:

Incomplete string escaping or encoding
This does not escape backslash characters in the input.

Specifically, multiple alerts point to the following block of code:

part = `"${part.replace(/"/g, '\\"')}"`;

The full code block is

    function quoteHeader(part) {
      if (part.includes(",") || part.includes('"')) {
        part = `"${part.replace(/"/g, '\\"')}"`;
      }
      return part;
    }

esbuild says the code is in node_modules/@smithy/smithy-client/dist-cjs/index.js

Regression Issue

• Select this option if this issue appears to be a regression.

SDK version number

@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Node v20.11.1

Reproduction Steps

Open PR with code bundled with 3.682.0

Observed Behavior

GitHub Advanced Security throws alert

Expected Behavior

No security alert

Possible Solution

Unclear if this is a true finding or a false positive given this is a client SDK.

Additional Information/Context

No response

[automartin5000]

Hey @aBurmeseDev, that's not our code. As I mentioned, that code seems to be coming from the node module @smithy/smithy-client which I believe is owned by AWS?

I believe I just found the original source.

I thought that was AWS source, but maybe not? If not, it still seems like that's a module being used by the AWS SDK.

[aBurmeseDev]

Understood, sorry for the confusion. I'll take a further look at the code and attempt to reproduce this first. A few questions to help me identify the culprit:

• when did this start?
• can you share your SDK and/or lambda code without any sensitive info?
• any logs you can share?

[automartin5000]

We started seeing the security alert a few weeks ago, which seems like it coincides with the timing of the commit from the linked repo. Can't really share any other code or logs other than the ones I've sent. I'm guessing if whoever owns that repo enables GHAS, they'll start seeing it too.

[kuhe]

The CodeQL CI was eventually passing in aws/aws-cdk#32073 and the PR was merged. So, is it still an issue?

And, the detection seems overly broad, since it considers SQL injection the primary risk in https://codeql.github.com/codeql-query-help/javascript/js-incomplete-sanitization/. The code is written for a specific use case with known inputs, and it does not involve SQL. We are not going to import a sanitization library at the SDK level to handle this.

[phuhung273]

Thanks for taking a look @kuhe. I believe it is still an issue. Seems like AWS CDK maintainers aware it fails on most PRs so they approved ignoring CodeQL security alert. Here are some others for references:

• fix(stepfunctions-tasks): SageMakerCreateTrainingJob add sagemaker:AddTags permission aws-cdk#32536
• feat(eks): Nodegroup support nodeRepairConfig aws-cdk#32626
• feat(ecs): ExternalService support daemon scheduling strategy aws-cdk#32630

[automartin5000]

The CodeQL CI was eventually passing in aws/aws-cdk#32073 and the PR was merged. So, is it still an issue?

And, the detection seems overly broad, since it considers SQL injection the primary risk in https://codeql.github.com/codeql-query-help/javascript/js-incomplete-sanitization/. The code is written for a specific use case with known inputs, and it does not involve SQL. We are not going to import a sanitization library at the SDK level to handle this.

Our issue is only tangentially related to the CDK (it's a Lambda custom resource). But this isn't about the specific use case. AFAIK, anyone importing the JS SDK into their code base is going to start seeing this issue with CodeQL.