Skip to content

Commit

Permalink
Fix seg fault(s) in AuthV4Signer, test more combos with checksumming
Browse files Browse the repository at this point in the history 
When issuing PutObject with zero length, and setting the checksum
algorithm to SHA256, and setting the checksum value (instead of letting
the SDK calculate it) resulted in a null pointer dereference with
payload signing enabled.

Added a matrix test to verify more code paths in AWSAuthV4Signer and
fixed two seg faults.
  • Loading branch information
@jeking3
jeking3 committed Jan 5, 2024
1 parent fdce535 commit 2c1848b
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 62 deletions.
37 changes: 24 additions & 13 deletions src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,25 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r
payloadHash = ComputePayloadHash(request);
if (payloadHash.empty())
{
// this indicates a hashing error occurred, which was logged
return false;
}

if (request.GetRequestHash().second != nullptr)
{
Aws::String checksumHeaderKey = Aws::String("x-amz-checksum-") + request.GetRequestHash().first;
Aws::String checksumHeaderValue = HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate(*(request.GetContentBody())).GetResult());
Aws::String checksumHeaderValue;
if (request.GetRequestHash().first == "sha256") {
// we already calculated the payload hash so just reverse the hex string to
// a ByteBuffer and Base64Encode it - otherwise we're re-hashing the content
checksumHeaderValue = HashingUtils::Base64Encode(HashingUtils::HexDecode(payloadHash));
} else {
// if it is one of the other hashes, we must be careful if there is no content body
auto body = request.GetContentBody();
checksumHeaderValue = (body)
? HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate(*body).GetResult())
: HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate({}).GetResult());
}
request.SetHeaderValue(checksumHeaderKey, checksumHeaderValue);
request.SetRequestHash("", nullptr);
}
Expand All @@ -254,8 +267,10 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r
request.SetHeaderValue(Http::AWS_TRAILER_HEADER, trailerHeaderValue);
request.SetTransferEncoding(CHUNKED_VALUE);
request.SetHeaderValue(Http::CONTENT_ENCODING_HEADER, Http::AWS_CHUNKED_VALUE);
request.SetHeaderValue(Http::DECODED_CONTENT_LENGTH_HEADER, request.GetHeaderValue(Http::CONTENT_LENGTH_HEADER));
request.DeleteHeader(Http::CONTENT_LENGTH_HEADER);
if (request.HasHeader(Http::CONTENT_LENGTH_HEADER)) {
request.SetHeaderValue(Http::DECODED_CONTENT_LENGTH_HEADER, request.GetHeaderValue(Http::CONTENT_LENGTH_HEADER));
request.DeleteHeader(Http::CONTENT_LENGTH_HEADER);
}
}
}

Expand Down Expand Up @@ -509,20 +524,17 @@ Aws::String AWSAuthV4Signer::GenerateSignature(const Aws::String& stringToSign,

Aws::String AWSAuthV4Signer::ComputePayloadHash(Aws::Http::HttpRequest& request) const
{
if (!request.GetContentBody())
const std::shared_ptr<Aws::IOStream>& body = request.GetContentBody();
if (!body)
{
AWS_LOGSTREAM_DEBUG(v4LogTag, "Using cached empty string sha256 " << EMPTY_STRING_SHA256 << " because payload is empty.");
return EMPTY_STRING_SHA256;
}

//compute hash on payload if it exists.
auto hashResult = m_hash->Calculate(*request.GetContentBody());

if(request.GetContentBody())
{
request.GetContentBody()->clear();
request.GetContentBody()->seekg(0);
}
// compute hash on payload if it exists
auto hashResult = m_hash->Calculate(*body);
body->clear(); // clears ios_flags
body->seekg(0);

if (!hashResult.IsSuccess())
{
Expand All @@ -531,7 +543,6 @@ Aws::String AWSAuthV4Signer::ComputePayloadHash(Aws::Http::HttpRequest& request)
}

auto sha256Digest = hashResult.GetResult();

Aws::String payloadHash(HashingUtils::HexEncode(sha256Digest));
AWS_LOGSTREAM_DEBUG(v4LogTag, "Calculated sha256 " << payloadHash << " for payload.");
return payloadHash;
Expand Down
125 changes: 76 additions & 49 deletions tests/aws-cpp-sdk-core-tests/aws/auth/AWSAuthSignerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,23 @@
#include <aws/core/http/standard/StandardHttpResponse.h>
#include <aws/core/platform/FileSystem.h>
#include <aws/core/platform/Platform.h>
#include <aws/core/utils/crypto/CRC32.h>
#include <aws/core/utils/crypto/Sha256.h>
#include <aws/core/utils/StringUtils.h>
#include <aws/core/utils/HashingUtils.h>
#include <aws/auth/signing.h>
#include <aws/cal/ecc.h>
#include <aws/common/encoding.h>
#include <fstream>
#include <tuple>

using namespace Aws::Client;
using namespace Aws::Utils;
using namespace Aws::Http;

static const char ALLOC_TAG[] = "AwsAuthV4SignerTest";
static const char EMPTY_STRING_SHA256[] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
static const char STREAMING_UNSIGNED_PAYLOAD_TRAILER[] = "STREAMING-UNSIGNED-PAYLOAD-TRAILER";
static const char UNSIGNED_PAYLOAD[] = "UNSIGNED-PAYLOAD";
static const char X_AMZ_SIGNATURE[] = "X-Amz-Signature";

Expand Down Expand Up @@ -389,17 +393,28 @@ static bool SignPayload(AWSAuthV4Signer::PayloadSigningPolicy policy, Aws::Http:
return true;
}

static void RunTestCaseWithPayload(const char* testCaseName, AWSAuthV4Signer::PayloadSigningPolicy policy, Aws::Http::Scheme scheme, bool requestSignPayload/*sign flag in request*/)
static void RunTestCaseWithPayload(const char* testCaseName, AWSAuthV4Signer::PayloadSigningPolicy policy, Aws::Http::Scheme scheme, bool requestSignPayload, const Aws::String& requestHash)
{
DateTime timestampForSigner;
std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credProvider = MakeStaticCredentialsProvider();
TestableAuthv4Signer signer(credProvider, "service", "us-east-1", policy, false);
bool signPayload = SignPayload(policy, scheme, requestSignPayload);
auto requestToMake = GetHttpRequestFromTestCase(testCaseName, timestampForSigner, scheme);

// simulate what AWSClient does if client chooses a specific request hash type
if (requestHash == "crc32") {
requestToMake.SetRequestHash("crc32", Aws::MakeShared<Crypto::CRC32>("crc32"));
} else if (requestHash == "sha256") {
requestToMake.SetRequestHash("sha256", Aws::MakeShared<Crypto::Sha256>("sha256"));
} else {
ASSERT_STREQ("none", requestHash.c_str());
}

signer.SetSigningTimestamp(timestampForSigner);
ASSERT_TRUE(signer.SignRequest(requestToMake, requestSignPayload));
ASSERT_STREQ(GetHttpRequestSignatureFromTestCase(testCaseName, signPayload).c_str(), requestToMake.GetAwsAuthorization().c_str());
if (requestHash == "none") {
ASSERT_STREQ(GetHttpRequestSignatureFromTestCase(testCaseName, signPayload).c_str(), requestToMake.GetAwsAuthorization().c_str());
}

TestableAuthv4Signer signerV4a(credProvider, "service", "us-east-1", policy, false, Aws::Auth::AWSSigningAlgorithm::ASYMMETRIC_SIGV4);
requestToMake = GetHttpRequestFromTestCase(testCaseName, timestampForSigner, scheme);
Expand Down Expand Up @@ -435,26 +450,37 @@ static void RunTestCaseWithPayload(const char* testCaseName, AWSAuthV4Signer::Pa
VerifyV4ASignature(stringToSign, signature, eccPublicKeyX, eccPublicKeyY);
}

static void RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy policy, Aws::Http::Scheme scheme, bool requestSignPayload/*sign flag in request*/)
static void RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy policy, Aws::Http::Scheme scheme, bool requestSignPayload, const Aws::String& requestHash)
{
bool signPayload = SignPayload(policy, scheme, requestSignPayload);
auto request = Standard::StandardHttpRequest(scheme == Aws::Http::Scheme::HTTP ? "http://test.com/query?key=val" : "https://test.com/query?key=val", Aws::Http::HttpMethod::HTTP_GET);

const char *expectedContentHashHeader =
signPayload ? EMPTY_STRING_SHA256 : (requestHash == "none") ? UNSIGNED_PAYLOAD : STREAMING_UNSIGNED_PAYLOAD_TRAILER;

if (requestHash == "crc32") {
request.SetRequestHash("crc32", Aws::MakeShared<Crypto::CRC32>("crc32"));
} else if (requestHash == "sha256") {
request.SetRequestHash("sha256", Aws::MakeShared<Crypto::Sha256>("sha256"));
} else {
ASSERT_STREQ("none", requestHash.c_str());
}

std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credProvider = MakeStaticCredentialsProvider();
AWSAuthV4Signer signer(credProvider, "service", "us-east-1", policy, false);
ASSERT_TRUE(signer.SignRequest(request, requestSignPayload));
ASSERT_STREQ(signPayload ? EMPTY_STRING_SHA256 : UNSIGNED_PAYLOAD, request.GetHeaderValue("x-amz-content-sha256").c_str());
ASSERT_STREQ(expectedContentHashHeader, request.GetHeaderValue("x-amz-content-sha256").c_str());

AWSAuthV4Signer signerV4a(credProvider, "service", "us-east-1", policy, false, Aws::Auth::AWSSigningAlgorithm::ASYMMETRIC_SIGV4);
ASSERT_TRUE(signer.SignRequest(request, requestSignPayload));
ASSERT_STREQ(signPayload ? EMPTY_STRING_SHA256 : UNSIGNED_PAYLOAD, request.GetHeaderValue("x-amz-content-sha256").c_str());
ASSERT_STREQ(expectedContentHashHeader, request.GetHeaderValue("x-amz-content-sha256").c_str());
}

class AWSAuthSignerTest : public Aws::Testing::AwsCppSdkGTestSuite
class AWSAuthSignerEmptyTest : public Aws::Testing::AwsCppSdkGTestSuite
{
};

TEST_F(AWSAuthSignerTest, HeadersWithEmptyValues)
TEST_F(AWSAuthSignerEmptyTest, HeadersWithEmptyValues)
{
auto request = Standard::StandardHttpRequest("https://test.com/query?key=val", Aws::Http::HttpMethod::HTTP_GET);
request.SetHeaderValue("same_key", " ");
Expand All @@ -470,50 +496,51 @@ TEST_F(AWSAuthSignerTest, HeadersWithEmptyValues)
ASSERT_STREQ(UNSIGNED_PAYLOAD, request.GetHeaderValue("x-amz-content-sha256").c_str());
}

TEST_F(AWSAuthSignerTest, PayloadSigningPolicyNever)
{
// Test without payload(empty body)
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTPS, false);

// Test with payload(non-empty body)
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Never, Aws::Http::Scheme::HTTPS, false);
}

TEST_F(AWSAuthSignerTest, PayloadSigningPolicyAlways)
{
// Test without payload(empty body)
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTPS, false);

// Test with payload(non-empty body)
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::Always, Aws::Http::Scheme::HTTPS, false);
}

TEST_F(AWSAuthSignerTest, PayloadSigningPolicyRequestDependent)
class AWSAuthSignerTestSuite
: public Aws::Testing::AwsCppSdkGTestSuite,
public ::testing::WithParamInterface<
std::tuple<
AWSAuthV4Signer::PayloadSigningPolicy,
Aws::Http::Scheme,
bool /* requestSignPayload */,
Aws::String /* requestHash */
>
>
{
// Test without payload(empty body)
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithoutPayload(AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTPS, false);
};

// Test with payload(non-empty body)
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTP, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTPS, true);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTP, false);
RunTestCaseWithPayload("post-x-www-form-urlencoded", AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent, Aws::Http::Scheme::HTTPS, false);
}
TEST_P(AWSAuthSignerTestSuite, Signing)
{
auto params = GetParam();
RunTestCaseWithoutPayload(std::get<0>(params), std::get<1>(params), std::get<2>(params), std::get<3>(params));
RunTestCaseWithPayload("post-x-www-form-urlencoded", std::get<0>(params), std::get<1>(params), std::get<2>(params), std::get<3>(params));
}

INSTANTIATE_TEST_SUITE_P(
AWSAuthSignerTest,
AWSAuthSignerTestSuite,
::testing::Combine(
// welcome to the matrix
::testing::Values(
AWSAuthV4Signer::PayloadSigningPolicy::Never,
AWSAuthV4Signer::PayloadSigningPolicy::RequestDependent,
AWSAuthV4Signer::PayloadSigningPolicy::Always
),
::testing::Values(
Aws::Http::Scheme::HTTP,
Aws::Http::Scheme::HTTPS
),
::testing::Values(
true,
false
),
::testing::Values(
"none",
"crc32",
"sha256" // has a special case to avoid double-hashing the payload
)
)
);

class AWSAuthV4SignerTest : public Aws::Testing::AwsCppSdkGTestSuite
{
Expand Down

0 comments on commit 2c1848b

Please sign in to comment.