Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect the signature process against null terminators #3141

Closed
1 of 2 tasks
Dalzhim opened this issue Oct 9, 2024 · 4 comments
Closed
1 of 2 tasks

Protect the signature process against null terminators #3141

Dalzhim opened this issue Oct 9, 2024 · 4 comments
Assignees
Labels
feature-request A feature should be added or improved.

Comments

@Dalzhim
Copy link

Dalzhim commented Oct 9, 2024

Describe the feature

The following code leads to painful request signature issues for which solutions are hard to come by:

std::string region = GetRegionFromIMDSv2();
if (!region.empty()) {
    // Erase the character that represents the availability zone (i.e. : a, b, c, d)
    region[region.length() - 1] = '\0';
}

The reason this is a problem is that the null terminator is part of the std::string's content. When the AWSAuthV4Signer.cpp builds the Authorization header for the request, it inserts the region into a StringStream, including the null-terminator. Later in the process, the header is effectively truncated in the middle of the Credential parameter and the other parameters (SignedHeaders and Signature) are completely cut off.

Use Case

Prevent obscure and preventable errors caused by unanticipated use of the SDK.

Proposed Solution

It would be trivial to prevent this kind of issue by streaming the underlying char* buffer of the string to defend against this kind of issue, like so: sstr << regionStr.c_str().

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@Dalzhim Dalzhim added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 9, 2024
@bhavya2109sharma bhavya2109sharma removed the needs-triage This issue or PR still needs to be triaged. label Oct 10, 2024
@bhavya2109sharma bhavya2109sharma self-assigned this Oct 10, 2024
@bhavya2109sharma
Copy link

Hello @Dalzhim,

Thanks for reaching out.
Could you please share your use case and an enough self-contained code which can reproduce the issue.

Thanks,
Bhavya

@bhavya2109sharma bhavya2109sharma added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Oct 10, 2024
@Dalzhim
Copy link
Author

Dalzhim commented Oct 10, 2024

If you start with the PutMetricData example in the AWS documentation:

Aws::CloudWatch::CloudWatchClient cw;

Aws::CloudWatch::Model::Dimension dimension;
dimension.SetName("UNIQUE_PAGES");
dimension.SetValue("URLS");

Aws::CloudWatch::Model::MetricDatum datum;
datum.SetMetricName("PAGES_VISITED");
datum.SetUnit(Aws::CloudWatch::Model::StandardUnit::None);
datum.SetValue(data_point);
datum.AddDimensions(dimension);

Aws::CloudWatch::Model::PutMetricDataRequest request;
request.SetNamespace("SITE/TRAFFIC");
request.AddMetricData(datum);

auto outcome = cw.PutMetricData(request);
if (!outcome.IsSuccess())
{
    std::cout << "Failed to put sample metric data:" <<
        outcome.GetError().GetMessage() << std::endl;
}
else
{
    std::cout << "Successfully put sample metric data" << std::endl;
}

You just need to change the first line with the following snippet in order to reproduce the issue:

std::string region = "us-east-1a";
region.back() = '\0';
Aws::Client::ClientConfiguration clientConfig;
clientConfig.region = region;
Aws::CloudWatch::CloudWatchClient cw{clientConfig};

Now is it important to be able to proceed this way? Not inherently. What's important though is that applying the proposed solution can help save a lot of time to the library users who unknowingly break the signature process.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Oct 11, 2024
@bhavya2109sharma
Copy link

Hello @Dalzhim

Thanks for providing the information.
In the latest versions of C++, the standard way of handling strings is that the null terminator is considered part of the string. If the customer wants to remove the last character of the string, they should use a method like str.pop_back() instead of manually setting the last character to '\0'. Also, Customer should protect the signature against null terminators as SDK cannot do at their side.

Thanks,
Bhavya

Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

2 participants