Skip to content

cryptomb: remove PKCS1_5 padding support. #20408

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.

Sign up for GitHub

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

Merged
ggreenway merged 4 commits into from
Mar 18, 2022
Merged

cryptomb: remove PKCS1_5 padding support. #20408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b03ecb6
Remove PKCS1_5 padding support.
ipuustin Mar 11, 2022
63965f6
Add release note.
ipuustin Mar 18, 2022
dba9a33
Add a debug message when using PKCS#1 v1.5.
ipuustin Mar 18, 2022
7f2b5a6
Add a test to validate the failure case.
ipuustin Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 18 additions & 31 deletions contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "source/common/config/datasource.h"

#include "openssl/ec.h"
#include "openssl/ssl.h"

namespace Envoy {
Expand Down Expand Up @@ -170,31 +169,24 @@ ssl_private_key_result_t rsaPrivateKeySignInternal(CryptoMbPrivateKeyConnection*
return status;
}

// Add RSA padding to the the hash. Only `PSS` padding is supported.
// TODO(ipuustin): add PKCS #1 v1.5 padding scheme later if requested.
if (!SSL_is_signature_algorithm_rsa_pss(signature_algorithm)) {
ops->logDebugMsg("non-supported RSA padding");
return status;
}

uint8_t* msg;
size_t msg_len;
int prefix_allocated = 0;

// Add RSA padding to the the hash. Supported types are `PSS` and `PKCS1`.
if (SSL_is_signature_algorithm_rsa_pss(signature_algorithm)) {
msg_len = RSA_size(rsa.get());
// We have to do manual memory management here, because BoringSSL tells in `prefix_allocated`
// variable whether or not memory needs to be freed.
msg = static_cast<uint8_t*>(OPENSSL_malloc(msg_len));
if (msg == nullptr) {
return status;
}
prefix_allocated = 1;
if (!RSA_padding_add_PKCS1_PSS_mgf1(rsa.get(), msg, hash, md, nullptr, -1)) {
OPENSSL_free(msg);
return status;
}
} else {
if (!RSA_add_pkcs1_prefix(&msg, &msg_len, &prefix_allocated, EVP_MD_type(md), hash, hash_len)) {
if (prefix_allocated) {
OPENSSL_free(msg);
}
return status;
}

msg_len = RSA_size(rsa.get());
msg = static_cast<uint8_t*>(OPENSSL_malloc(msg_len));
if (msg == nullptr) {
return status;
}
if (!RSA_padding_add_PKCS1_PSS_mgf1(rsa.get(), msg, hash, md, nullptr, -1)) {
OPENSSL_free(msg);
return status;
}

// Create MB context which will be used for this particular
Expand All @@ -203,18 +195,13 @@ ssl_private_key_result_t rsaPrivateKeySignInternal(CryptoMbPrivateKeyConnection*
std::make_shared<CryptoMbRsaContext>(std::move(pkey), ops->dispatcher_, ops->cb_);

if (!mb_ctx->rsaInit(msg, msg_len)) {
if (prefix_allocated) {
OPENSSL_free(msg);
}
return status;
}

if (prefix_allocated) {
OPENSSL_free(msg);
return status;
}

ops->addToQueue(mb_ctx);
status = ssl_private_key_retry;
OPENSSL_free(msg);
return status;
}

Expand Down
41 changes: 10 additions & 31 deletions contrib/cryptomb/private_key_providers/test/ops_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,34 +133,13 @@ TEST_F(CryptoMbProviderEcdsaTest, TestEcdsaSigning) {
}

TEST_F(CryptoMbProviderRsaTest, TestRsaPkcs1Signing) {
// Initialize connections.
TestCallbacks cbs[CryptoMbQueue::MULTIBUFF_BATCH];
std::vector<std::unique_ptr<CryptoMbPrivateKeyConnection>> connections;
for (auto& cb : cbs) {
connections.push_back(std::make_unique<CryptoMbPrivateKeyConnection>(
cb, *dispatcher_, bssl::UpRef(pkey_), queue_));
}

// Create MULTIBUFF_BATCH amount of signing operations.
for (uint32_t i = 0; i < CryptoMbQueue::MULTIBUFF_BATCH; i++) {
// Create request.
res_ = rsaPrivateKeySignForTest(connections[i].get(), nullptr, nullptr, max_out_len_,
SSL_SIGN_RSA_PKCS1_SHA256, in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_retry);

// No processing done after first requests.
// After the last request, the status is set only from the event loop which is not run. This
// should still be "retry", the cryptographic result is present anyway.
res_ = privateKeyCompleteForTest(connections[i].get(), nullptr, nullptr, max_out_len_);
EXPECT_EQ(res_, ssl_private_key_retry);
}

// Timeout does not have to be triggered when queue is at maximum size.
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
// PKCS #1 v1.5 is not supported.
TestCallbacks cb;
CryptoMbPrivateKeyConnection op(cb, *dispatcher_, bssl::UpRef(pkey_), queue_);

res_ = privateKeyCompleteForTest(connections[0].get(), out_, &out_len_, max_out_len_);
EXPECT_EQ(res_, ssl_private_key_success);
EXPECT_NE(out_len_, 0);
res_ = rsaPrivateKeySignForTest(&op, nullptr, nullptr, max_out_len_, SSL_SIGN_RSA_PKCS1_SHA256,
in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_failure);
}

TEST_F(CryptoMbProviderRsaTest, TestRsaPssSigning) {
Expand Down Expand Up @@ -279,7 +258,7 @@ TEST_F(CryptoMbProviderRsaTest, TestRSATimer) {

// Successful operation with timer.
CryptoMbPrivateKeyConnection op0(cbs[0], *dispatcher_, bssl::UpRef(pkey_), queue_);
res_ = rsaPrivateKeySignForTest(&op0, nullptr, nullptr, max_out_len_, SSL_SIGN_RSA_PKCS1_SHA256,
res_ = rsaPrivateKeySignForTest(&op0, nullptr, nullptr, max_out_len_, SSL_SIGN_RSA_PSS_SHA256,
in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_retry);

Expand All @@ -300,7 +279,7 @@ TEST_F(CryptoMbProviderRsaTest, TestRSATimer) {

CryptoMbPrivateKeyConnection op1(cbs[1], *dispatcher_, bssl::UpRef(pkey_), queue_);

res_ = rsaPrivateKeySignForTest(&op1, nullptr, nullptr, max_out_len_, SSL_SIGN_RSA_PKCS1_SHA256,
res_ = rsaPrivateKeySignForTest(&op1, nullptr, nullptr, max_out_len_, SSL_SIGN_RSA_PSS_SHA256,
in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_retry);

Expand Down Expand Up @@ -329,7 +308,7 @@ TEST_F(CryptoMbProviderRsaTest, TestRSAQueueSizeStatistics) {
// Create correct amount of signing operations for current index.
for (uint32_t j = 0; j < i; j++) {
res_ = rsaPrivateKeySignForTest(connections[j].get(), nullptr, nullptr, max_out_len_,
SSL_SIGN_RSA_PKCS1_SHA256, in_, in_len_);
SSL_SIGN_RSA_PSS_SHA256, in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_retry);
}

Expand All @@ -352,7 +331,7 @@ TEST_F(CryptoMbProviderRsaTest, TestRSAQueueSizeStatistics) {
// Create an amount of signing operations equal to maximum queue size.
for (uint32_t j = 0; j < CryptoMbQueue::MULTIBUFF_BATCH; j++) {
res_ = rsaPrivateKeySignForTest(connections[j].get(), nullptr, nullptr, max_out_len_,
SSL_SIGN_RSA_PKCS1_SHA256, in_, in_len_);
SSL_SIGN_RSA_PSS_SHA256, in_, in_len_);
EXPECT_EQ(res_, ssl_private_key_retry);
}

Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Minor Behavior Changes

* access_log: log all header values in the grpc access log.
* config: warning messages for protobuf unknown fields now contain ancestors for easier troubleshooting.
* cryptomb: remove RSA PKCS1 v1.5 padding support.
* decompressor: decompressor does not duplicate `accept-encoding` header values anymore. This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.append_to_accept_content_encoding_only_once`` to false.
* dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host.
* ext_authz: added requested server name in ext_authz network filter for auth review.
Expand Down