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

Adds verification key for Wasm module #177

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 0 additions & 4 deletions .github/workflows/cpp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,3 @@ jobs:
- name: Test
run: |
bazel test --define runtime=${{ matrix.runtime }} //test/...

- name: Test (signed Wasm module)
run: |
bazel test --define runtime=${{ matrix.runtime }} --per_file_copt=//...@-DPROXY_WASM_VERIFY_WITH_ED25519_PUBKEY=\"$(xxd -p -c 256 test/test_data/signature_key1.pub | cut -b9-)\" //test:signature_util_test
2 changes: 1 addition & 1 deletion include/proxy-wasm/signature_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class SignatureUtil {
* @param message is the reference to store the message (success or error).
* @return indicates whether the bytecode has a valid Wasm signature.
*/
static bool verifySignature(std::string_view bytecode, std::string &message);
static bool verifySignature(std::string_view bytecode, const std::string pubkey, std::string &message);
Copy link
Member

Choose a reason for hiding this comment

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

Style: clang-format warning (line too long).

};

} // namespace proxy_wasm
6 changes: 4 additions & 2 deletions include/proxy-wasm/wasm.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class WasmBase : public std::enable_shared_from_this<WasmBase> {
WasmBase(const std::shared_ptr<WasmHandleBase> &other, WasmVmFactory factory);
virtual ~WasmBase();

bool load(const std::string &code, bool allow_precompiled = false);
bool load(const std::string &code, bool allow_precompiled = false, const std::string pubkey = "");
bool initialize();
void startVm(ContextBase *root_context);
bool configure(ContextBase *root_context, std::shared_ptr<PluginBase> plugin);
Expand Down Expand Up @@ -311,8 +311,10 @@ using WasmHandleCloneFactory =
std::function<std::shared_ptr<WasmHandleBase>(std::shared_ptr<WasmHandleBase> wasm)>;

// Returns nullptr on failure (i.e. initialization of the VM fails).
// TODO: Consider a VerificationOptions struct rather than a single pubkey.
Copy link
Member

Choose a reason for hiding this comment

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

s/TODO/TODO(asraa)/

std::shared_ptr<WasmHandleBase>
createWasm(std::string vm_key, std::string code, std::shared_ptr<PluginBase> plugin,
createWasm(std::string vm_key, std::string code, std::string pubkey,
std::shared_ptr<PluginBase> plugin,
Copy link
Member

Choose a reason for hiding this comment

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

Style: clang-format warning (weird indent).

WasmHandleFactory factory, WasmHandleCloneFactory clone_factory, bool allow_precompiled);
// Get an existing ThreadLocal VM matching 'vm_id' or nullptr if there isn't one.
std::shared_ptr<WasmHandleBase> getThreadLocalWasm(std::string_view vm_id);
Expand Down
15 changes: 6 additions & 9 deletions src/signature_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

namespace {

#ifdef PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY

static uint8_t hex2dec(const unsigned char c) {
if (c >= '0' && c <= '9') {
Expand All @@ -46,15 +45,13 @@ template <size_t N> constexpr std::array<uint8_t, N> hex2pubkey(const char (&hex
return pubkey;
}

#endif

} // namespace

namespace proxy_wasm {

bool SignatureUtil::verifySignature(std::string_view bytecode, std::string &message) {

#ifdef PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY
bool SignatureUtil::verifySignature(std::string_view bytecode, std::string pubkey, std::string &message) {
if (!pubkey.empty()) {

/*
* Ed25519 signature generated using https://github.com/jedisct1/wasmsign
Expand Down Expand Up @@ -101,7 +98,9 @@ bool SignatureUtil::verifySignature(std::string_view bytecode, std::string &mess
uint8_t hash[SHA512_DIGEST_LENGTH];
SHA512_Final(hash, &ctx);

static const auto ed25519_pubkey = hex2pubkey<32>(PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY);
char pubkey_char[65];
strcpy(pubkey_char, pubkey.c_str());
static const auto ed25519_pubkey = hex2pubkey<32>(pubkey_char);
Copy link
Member

Choose a reason for hiding this comment

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

This is static variable, so the first key used would persist forever and it would be used for all verifications, leading to false negatives.


if (!ED25519_verify(hash, sizeof(hash), signature, ed25519_pubkey.data())) {
message = "Signature mismatch";
Expand All @@ -110,9 +109,7 @@ bool SignatureUtil::verifySignature(std::string_view bytecode, std::string &mess

message = "Wasm signature OK (Ed25519)";
return true;

#endif

}
return true;
}

Expand Down
21 changes: 12 additions & 9 deletions src/wasm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ WasmBase::~WasmBase() {
pending_delete_.clear();
}

bool WasmBase::load(const std::string &code, bool allow_precompiled) {
bool WasmBase::load(const std::string &code, bool allow_precompiled, const std::string pubkey) {
assert(!started_from_.has_value());

if (!wasm_vm_) {
Expand All @@ -251,13 +251,15 @@ bool WasmBase::load(const std::string &code, bool allow_precompiled) {
return true;
}

// Verify signature.
std::string message;
if (!SignatureUtil::verifySignature(code, message)) {
fail(FailState::UnableToInitializeCode, message);
return false;
} else {
wasm_vm_->integration()->trace(message);
// Verify signature if a pubkey is present.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Verify signature if a pubkey is present.
// Verify signature if a public key is present (embedded at build-time or provided via pubkey).

if (!pubkey.empty()) {
std::string message;
if (!SignatureUtil::verifySignature(code, pubkey, message)) {
fail(FailState::UnableToInitializeCode, message);
return false;
} else {
wasm_vm_->integration()->trace(message);
}
}

// Get ABI version from the module.
Expand Down Expand Up @@ -465,6 +467,7 @@ WasmForeignFunction WasmBase::getForeignFunction(std::string_view function_name)
}

std::shared_ptr<WasmHandleBase> createWasm(std::string vm_key, std::string code,
std::string pubkey,
Copy link
Member

Choose a reason for hiding this comment

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

Style: clang-format warning (should be in the previous line).

std::shared_ptr<PluginBase> plugin,
WasmHandleFactory factory,
WasmHandleCloneFactory clone_factory,
Expand Down Expand Up @@ -492,7 +495,7 @@ std::shared_ptr<WasmHandleBase> createWasm(std::string vm_key, std::string code,
(*base_wasms)[vm_key] = wasm_handle;
}

if (!wasm_handle->wasm()->load(code, allow_precompiled)) {
if (!wasm_handle->wasm()->load(code, allow_precompiled, pubkey)) {
wasm_handle->wasm()->fail(FailState::UnableToInitializeCode, "Failed to load Wasm code");
return nullptr;
}
Expand Down
1 change: 1 addition & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ cc_test(
"//test/test_data:abi_export.signed.with.key1.wasm",
"//test/test_data:abi_export.signed.with.key2.wasm",
"//test/test_data:abi_export.wasm",
"//test/test_data:signature_key1.pub",
],
# Test only when compiled to verify plugins.
tags = ["manual"],
Expand Down
43 changes: 28 additions & 15 deletions test/signature_util_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,36 +24,49 @@

namespace proxy_wasm {

TEST(TestSignatureUtil, GoodSignature) {
#ifndef PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY
FAIL() << "Built without a key for verifying signed Wasm modules.";
#endif
std::string BytesToHex(std::vector<uint8_t> bytes) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string BytesToHex(std::vector<uint8_t> bytes) {
static std::string bytesToHex(std::vector<uint8_t> bytes) {

static const char *const hex = "0123456789ABCDEF";
std::string result;
result.reserve(bytes.size() * 2);
for (auto byte : bytes) {
result.push_back(hex[byte >> 4]);
result.push_back(hex[byte & 0xf]);
}
return result;
}

static std::string publickey(std::string filename) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static std::string publickey(std::string filename) {
static std::string publicKey(std::string filename) {

auto path = "test/test_data/" + filename;
std::ifstream file(path, std::ios::binary);
EXPECT_FALSE(file.fail()) << "failed to open: " << path;
std::stringstream file_string_stream;
file_string_stream << file.rdbuf();
std::string input = file_string_stream.str();
std::vector<uint8_t> bytes(input.begin() + 4, input.end());
return BytesToHex(bytes);
}

TEST(TestSignatureUtil, GoodSignature) {
std::string pubkey = publickey("signature_key1.pub");
const auto bytecode = readTestWasmFile("abi_export.signed.with.key1.wasm");
std::string message;
EXPECT_TRUE(SignatureUtil::verifySignature(bytecode, message));
EXPECT_TRUE(SignatureUtil::verifySignature(bytecode, pubkey, message));
EXPECT_EQ(message, "Wasm signature OK (Ed25519)");
}

TEST(TestSignatureUtil, BadSignature) {
#ifndef PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY
FAIL() << "Built without a key for verifying signed Wasm modules.";
#endif

std::string pubkey = publickey("signature_key1.pub");
const auto bytecode = readTestWasmFile("abi_export.signed.with.key2.wasm");
std::string message;
EXPECT_FALSE(SignatureUtil::verifySignature(bytecode, message));
EXPECT_FALSE(SignatureUtil::verifySignature(bytecode, pubkey, message));
EXPECT_EQ(message, "Signature mismatch");
}

TEST(TestSignatureUtil, NoSignature) {
#ifndef PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY
FAIL() << "Built without a key for verifying signed Wasm modules.";
#endif

std::string pubkey = publickey("signature_key1.pub");
const auto bytecode = readTestWasmFile("abi_export.wasm");
std::string message;
EXPECT_FALSE(SignatureUtil::verifySignature(bytecode, message));
EXPECT_FALSE(SignatureUtil::verifySignature(bytecode, pubkey, message));
EXPECT_EQ(message, "Custom Section \"signature_wasmsign\" not found");
}

Expand Down