Skip to content

Commit

Permalink
Connector warns on concurrent PC/SC calls (#782)
Browse files Browse the repository at this point in the history
Add warning logs into Smart Card Connector whenever a client
application emits illegal concurrent PC/SC requests: an
SCARDCONTEXT must only be used from a single thread (with the
only exception of SCardCancel that can be called concurrently).

This is preparation for disallowing concurrent PC/SC calls in the
future as planned in #136. Concurrent calls are not allowed by the
PC/SC API specification, and the PC/SC-Lite's implementation
breaks in subtle ways when this is violated.
  • Loading branch information
emaxx-google authored Dec 9, 2022
1 parent 23b88ac commit d640559
Show file tree
Hide file tree
Showing 7 changed files with 246 additions and 0 deletions.
1 change: 1 addition & 0 deletions common/cpp/build/tests/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ SOURCES_PATH := $(ROOT_SOURCES_PATH)/$(ROOT_SOURCES_SUBDIR)
SOURCES := \
$(SOURCES_PATH)/formatting_unittest.cc \
$(SOURCES_PATH)/ipc_emulation_unittest.cc \
$(SOURCES_PATH)/join_string_unittest.cc \
$(SOURCES_PATH)/logging/hex_dumping_unittest.cc \
$(SOURCES_PATH)/logging/logging_unittest.cc \
$(SOURCES_PATH)/messaging/typed_message_router_unittest.cc \
Expand Down
49 changes: 49 additions & 0 deletions common/cpp/src/google_smart_card_common/join_string.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2022 Google Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_
#define GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_

#include <string>

namespace google_smart_card {

template <typename Container>
std::string JoinStrings(const Container& container,
const std::string& separator) {
if (container.empty()) {
return "";
}

size_t need_size = separator.size() * (container.size() - 1);
for (const auto& item : container) {
need_size += item.length();
}

std::string joined;
joined.reserve(need_size);
bool is_first = true;
for (const auto& item : container) {
if (!is_first) {
joined += separator;
}
is_first = false;
joined += item;
}
return joined;
}

} // namespace google_smart_card

#endif // GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_
62 changes: 62 additions & 0 deletions common/cpp/src/google_smart_card_common/join_string_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright 2022 Google Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <google_smart_card_common/join_string.h>

#include <set>
#include <string>
#include <vector>

#include <gtest/gtest.h>

namespace google_smart_card {
namespace {

TEST(JoinString, EmptyVector) {
std::vector<std::string> vec;
EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "");
}

TEST(JoinString, OneItemVector) {
std::vector<std::string> vec = {"foo"};
EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo");
}

TEST(JoinString, TwoItemsVector) {
std::vector<std::string> vec = {"foo", "bar"};
EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo,bar");
}

TEST(JoinString, TwoItemsSet) {
std::set<std::string> vec = {"a", "b"};
EXPECT_EQ(JoinStrings(vec, /*separator=*/" - "), "a - b");
}

TEST(JoinString, EmptySeparator) {
std::vector<std::string> vec = {"foo", "bar", "foo"};
EXPECT_EQ(JoinStrings(vec, /*separator=*/""), "foobarfoo");
}

TEST(JoinString, EmptyItem) {
std::vector<std::string> vec = {"foo", "", "bar"};
EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo,,bar");
}

TEST(JoinString, EmptyItemsOnly) {
std::vector<std::string> vec = {"", "", ""};
EXPECT_EQ(JoinStrings(vec, /*separator=*/","), ",,");
}

} // namespace
} // namespace google_smart_card
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@ bool PcscLiteClientHandlesRegistry::ContainsHandle(
return handle_to_context_map_.count(s_card_handle);
}

SCARDCONTEXT PcscLiteClientHandlesRegistry::FindContextByHandle(
SCARDHANDLE s_card_handle) const {
const std::unique_lock<std::mutex> lock(mutex_);

auto iter = handle_to_context_map_.find(s_card_handle);
if (iter == handle_to_context_map_.end()) {
return 0;
}
return iter->second;
}

void PcscLiteClientHandlesRegistry::AddHandle(SCARDCONTEXT s_card_context,
SCARDHANDLE s_card_handle) {
const std::unique_lock<std::mutex> lock(mutex_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class PcscLiteClientHandlesRegistry final {
std::vector<SCARDCONTEXT> PopAllContexts();

bool ContainsHandle(SCARDHANDLE s_card_handle) const;
// Returns the context that this handle refers to, or zero if none found.
SCARDCONTEXT FindContextByHandle(SCARDHANDLE s_card_handle) const;
// Adds the handle to the data structure. CHECKs that it wasn't already
// present.
void AddHandle(SCARDCONTEXT s_card_context, SCARDHANDLE s_card_handle);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@
#include <cstddef>
#include <cstring>
#include <memory>
#include <mutex>
#include <string>
#include <thread>
#include <tuple>
#include <vector>

#include <google_smart_card_common/formatting.h>
#include <google_smart_card_common/join_string.h>
#include <google_smart_card_common/logging/function_call_tracer.h>
#include <google_smart_card_common/logging/hex_dumping.h>
#include <google_smart_card_common/multi_string.h>
Expand Down Expand Up @@ -215,6 +218,55 @@ void PcscLiteClientRequestProcessor::AsyncProcessRequest(
.detach();
}

PcscLiteClientRequestProcessor::ScopedConcurrencyGuard::ScopedConcurrencyGuard(
const std::string& function_name,
SCARDCONTEXT s_card_context,
PcscLiteClientRequestProcessor& owner)
: function_name_(function_name),
s_card_context_(s_card_context),
owner_(owner) {
if (!s_card_context_) {
return;
}

optional<std::string> concurrent_functions_dump;
{
const std::unique_lock<std::mutex> lock(owner_.mutex_);
std::multiset<std::string>& running_functions =
owner_.context_to_running_functions_[s_card_context_];
if (!running_functions.empty()) {
concurrent_functions_dump =
JoinStrings(running_functions, /*separator=*/", ");
}
running_functions.insert(function_name_);
}

if (concurrent_functions_dump) {
GOOGLE_SMART_CARD_LOG_WARNING
<< owner_.logging_prefix_
<< "Client violates threading: concurrent calls of " << function_name_
<< ", " << *concurrent_functions_dump
<< " against the same SCARDCONTEXT. Future releases of Smart Card "
"Connector will forbid this: every call referring to some "
"SCARDCONTEXT must come after the previous one completed.";
}
}

PcscLiteClientRequestProcessor::ScopedConcurrencyGuard::
~ScopedConcurrencyGuard() {
if (!s_card_context_) {
return;
}
const std::unique_lock<std::mutex> lock(owner_.mutex_);
auto iter = owner_.context_to_running_functions_.find(s_card_context_);
GOOGLE_SMART_CARD_CHECK(iter != owner_.context_to_running_functions_.end());
std::multiset<std::string>& running_functions = iter->second;
running_functions.erase(running_functions.find(function_name_));
if (running_functions.empty()) {
owner_.context_to_running_functions_.erase(iter);
}
}

void PcscLiteClientRequestProcessor::BuildHandlerMap() {
AddHandlerToHandlerMap(
"pcsc_lite_version_number",
Expand Down Expand Up @@ -378,6 +430,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardReleaseContext(
status_log_severity_);
tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardReleaseContext",
s_card_context, *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand Down Expand Up @@ -413,6 +467,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardConnect(
tracer.AddPassedArg("dwPreferredProtocols",
DebugDumpSCardProtocols(preferred_protocols));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardConnect", s_card_context,
*this);

SCARDHANDLE s_card_handle;
DWORD active_protocol;
Expand Down Expand Up @@ -538,6 +594,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardReconnect(
tracer.AddPassedArg("dwInitialization",
DebugDumpSCardDisposition(initialization_action));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardReconnect",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -575,6 +634,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardDisconnect(
tracer.AddPassedArg("dwDisposition",
DebugDumpSCardDisposition(disposition_action));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardDisconnect",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = DisconnectCard(s_card_handle, disposition_action);

Expand Down Expand Up @@ -607,6 +669,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardBeginTransaction(
status_log_severity_);
tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardBeginTransaction",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -635,6 +700,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardEndTransaction(
tracer.AddPassedArg("dwDisposition",
DebugDumpSCardDisposition(disposition_action));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardEndTransaction",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand All @@ -660,6 +728,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardStatus(
status_log_severity_);
tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardStatus",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -735,6 +806,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardGetStatusChange(
: &pcsc_lite_reader_states[0],
pcsc_lite_reader_states.size()));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardGetStatusChange",
s_card_context, *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand Down Expand Up @@ -784,6 +857,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardControl(
tracer.AddPassedArg("bSendBuffer",
"<" + DebugDumpSCardBufferContents(data_to_send) + ">");
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardControl",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -825,6 +901,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardGetAttrib(
tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle));
tracer.AddPassedArg("dwAttrId", DebugDumpSCardAttributeId(attribute_id));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardGetAttrib",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -867,6 +946,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardSetAttrib(
tracer.AddPassedArg("dwAttrId", DebugDumpSCardAttributeId(attribute_id));
tracer.AddPassedArg("pbAttr", "<" + HexDumpBytes(attribute) + ">");
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardSetAttrib",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -916,6 +998,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardTransmit(
scard_response_protocol_information));
}
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard(
"SCardTransmit",
s_card_handles_registry_.FindContextByHandle(s_card_handle), *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsHandle(s_card_handle))
Expand Down Expand Up @@ -984,6 +1069,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardListReaders(
tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context));
tracer.AddPassedArg("mszGroups", Value::kNullTypeTitle);
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardListReaders", s_card_context,
*this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand Down Expand Up @@ -1020,6 +1107,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardListReaderGroups(
status_log_severity_);
tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardListReaderGroups",
s_card_context, *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand Down Expand Up @@ -1058,6 +1147,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardCancel(
status_log_severity_);
tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context));
tracer.LogEntrance();
// Note there's no `ScopedConcurrencyGuard`, since PC/SC API allows calling
// `SCardCancel()` from different threads.

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand All @@ -1083,6 +1174,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardIsValidContext(
status_log_severity_);
tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context));
tracer.LogEntrance();
ScopedConcurrencyGuard concurrency_guard("SCardIsValidContext",
s_card_context, *this);

LONG return_code = SCARD_S_SUCCESS;
if (!s_card_handles_registry_.ContainsContext(s_card_context))
Expand Down
Loading

0 comments on commit d640559

Please sign in to comment.