From c42fb18d59efb5f7c77a1804cc85b3af337fb07a Mon Sep 17 00:00:00 2001 From: Mounir IDRASSI Date: Wed, 14 Oct 2026 12:44:23 +0200 Subject: [PATCH] Security tokens: wipe APDU buffers throughout their lifetime EMV keyfile cleanup did not cover APDU response storage, temporary copies, or transport buffers on exceptions. Wipe responses on clear, replacement, destruction and growth, avoid redundant response and PAN copies, and install transport cleanup guards before use. Add standalone checks for erasure before release, aliasing, allocation failures, exception unwinding and const TLV input. --- Tests/response_apdu_test.cpp | 158 +++++++++++++++++++++++++++++++++++ src/Common/EMVCard.cpp | 42 +++++----- src/Common/ResponseAPDU.cpp | 105 +++++++++++++++-------- src/Common/ResponseAPDU.h | 11 ++- src/Common/SCardReader.cpp | 6 +- src/Common/TLVParser.cpp | 4 +- src/Common/TLVParser.h | 4 +- 7 files changed, 261 insertions(+), 69 deletions(-) create mode 100644 Tests/response_apdu_test.cpp diff --git a/Tests/response_apdu_test.cpp b/Tests/response_apdu_test.cpp new file mode 100644 index 00000000..9e82d05c --- /dev/null +++ b/Tests/response_apdu_test.cpp @@ -0,0 +1,158 @@ +// Standalone cleanup checks using synthetic data; no smart card is required. +// c++ -std=c++11 -O2 -Isrc Tests/response_apdu_test.cpp \ +// src/Common/ResponseAPDU.cpp src/Common/TLVParser.cpp -o /tmp/response_apdu_test +#include "Common/ResponseAPDU.h" +#include "Common/TLVParser.h" +#include +#include +#include +#include +#include + +using namespace VeraCrypt; + +static const void *watched = NULL; +static size_t watchedSize = 0; +static unsigned int wipesChecked = 0; +static bool failNextAllocation = false; + +static void Require(bool condition, const char *message) +{ + if (!condition) + { + std::fprintf(stderr, "FAIL: %s\n", message); + std::abort(); + } +} + +void *operator new(size_t size) +{ + if (failNextAllocation) + { + failNextAllocation = false; + throw std::bad_alloc(); + } + void *p = std::malloc(size ? size : 1); + if (!p) throw std::bad_alloc(); + return p; +} + +void operator delete(void *p) noexcept +{ + if (p && p == watched) + { + // Inspect while storage is still allocated, before forwarding to free(). + const unsigned char *bytes = static_cast(p); + for (size_t i = 0; i < watchedSize; ++i) + Require(bytes[i] == 0, "response storage released without erasure"); + watched = NULL; + ++wipesChecked; + } + std::free(p); +} + +#if __cplusplus >= 201402L +void operator delete(void *p, size_t) noexcept { ::operator delete(p); } +#endif + +static void Watch(const ResponseAPDU& response) +{ + Require(watched == NULL && !response.getData().empty(), "invalid cleanup observation"); + watched = response.getData().data(); + watchedSize = response.getData().size(); +} + +int main() +{ + static_assert(std::is_same().getData()), + const vector&>::value, "getData must not copy response bytes"); + const vector original(37, 0x5a); + const vector suffix(7, 0x23); + + { + ResponseAPDU response(original, 0x9000); + Watch(response); + response.clear(); + Require(response.getData().empty() && response.getSW() == 0, "clear changed semantics"); + response.appendData(suffix); + Require(response.getData() == suffix, "reuse after clear"); + // The original tail must also have been wiped, even though size is now smaller. + } + { + ResponseAPDU response(original, 0x9000); + const vector extra(response.getData().capacity() + 1, 0x23); + Watch(response); + response.appendData(extra); + Require(watched == NULL, "growth did not release old storage"); + Require(response.getData().size() == original.size() + extra.size(), "append size"); + for (size_t i = 0; i < response.getData().size(); ++i) + Require(response.getData()[i] == (i < original.size() ? 0x5a : 0x23), "append contents"); + Watch(response); + } + { + ResponseAPDU response(original, 0x9000); + response.appendData(response.getData()); + Require(response.getData() == vector(original.size() * 2, 0x5a), "aliased append"); + response.clear(); + response.appendData(suffix); + response.appendData(response.getData()); + Require(response.getData() == vector(suffix.size() * 2, 0x23), "aliased append with capacity"); + Watch(response); + } + { + ResponseAPDU source(original, 0x9000), target(suffix, 0x6a82); + Watch(target); + target = source; + Require(watched == NULL && target.getData() == original && target.getSW() == 0x9000, "copy assignment"); + ResponseAPDU copy(target); + target.clear(); + Require(copy.getData() == original, "copy must own its storage"); + ResponseAPDU& alias = copy; + copy = alias; + Require(copy.getData() == original, "self assignment"); + Watch(copy); + } + { + ResponseAPDU source(original, 0x9000), target(suffix, 0x6a82); + vector encoded = source.getBytes(); + Watch(target); + target.setBytes(encoded); + Require(watched == NULL && target.getData() == original && target.getSW() == 0x9000, "response round trip"); + ResponseAPDU alias(encoded, 0); + alias.setBytes(alias.getData()); + Require(alias.getData() == original && alias.getSW() == 0x9000, "aliased setBytes"); + Watch(target); + target.setBytes(vector(1, 0)); + Require(watched == NULL && target.getData().empty() && target.getSW() == 0, "short response"); + } + { + ResponseAPDU response(original, 0x9000), target(suffix, 0x6a82); + const vector extra(response.getData().capacity() + 1, 0x23); + failNextAllocation = true; + try { response.appendData(extra); Require(false, "expected allocation failure"); } + catch (const std::bad_alloc&) {} + Require(response.getData() == original && response.getSW() == 0x9000, "failed growth changed response"); + failNextAllocation = true; + try { target = response; Require(false, "expected allocation failure"); } + catch (const std::bad_alloc&) {} + Require(target.getData() == suffix && target.getSW() == 0x6a82, "failed assignment changed response"); + Watch(response); + } + try + { + ResponseAPDU response(original, 0x9000); + Watch(response); + throw 1; + } + catch (int) {} + { + const uint8 encoded[] = { 0x5a, 0x02, 0x12, 0x34 }; + ResponseAPDU response(vector(encoded, encoded + sizeof(encoded)), 0x9000); + shared_ptr node = TLVParser::TLV_Parse(response.getData().data(), response.getData().size()); + response.clear(); + Require(node->Tag == 0x5a && node->Value->size() == 2 + && (*node->Value)[0] == 0x12 && (*node->Value)[1] == 0x34, "parser retained borrowed storage"); + } + Require(watched == NULL && wipesChecked == 10, "missing cleanup observations"); + std::puts("PASS: response ownership, erasure before release, replacement, growth, aliasing, allocation failure, unwinding and const TLV input"); +} diff --git a/src/Common/EMVCard.cpp b/src/Common/EMVCard.cpp index 30f7f5f9..70725003 100644 --- a/src/Common/EMVCard.cpp +++ b/src/Common/EMVCard.cpp @@ -151,7 +151,6 @@ namespace VeraCrypt bool hasBeenReset = false; CommandAPDU command; ResponseAPDU response; - vector responseData; shared_ptr rootNode; shared_ptr fciNode; shared_ptr dfNameNode; @@ -218,8 +217,7 @@ namespace VeraCrypt } if (response.getSW() == SW_NO_ERROR && response.getData().size() > 0) { - responseData = response.getData(); - rootNode = TLVParser::TLV_Parse(responseData.data(), responseData.size()); + rootNode = TLVParser::TLV_Parse(response.getData().data(), response.getData().size()); fciNode = TLVParser::TLV_Find(rootNode, EMV_FCI_TAG); if (fciNode && fciNode->Subs->size() >= 2) { @@ -261,11 +259,9 @@ namespace VeraCrypt m_reader->ApduProcessData(command, response); if (response.getSW() == SW_NO_ERROR && response.getData().size() > 0) { - responseData = response.getData(); - try { - templateNode = TLVParser::TLV_Parse(responseData.data(), responseData.size()); + templateNode = TLVParser::TLV_Parse(response.getData().data(), response.getData().size()); if (templateNode && templateNode->Tag == EMV_TEMPLATE_TAG && templateNode->Subs->size() >= 1) { for (size_t i = 0; i < templateNode->Subs->size(); i++) @@ -342,10 +338,15 @@ namespace VeraCrypt shared_ptr issuerPublicKeyCertNode; CommandAPDU command; ResponseAPDU response; - vector responseData; + if (!iccCert.empty()) + burn(iccCert.data(), iccCert.size()); iccCert.clear(); + if (!issuerCert.empty()) + burn(issuerCert.data(), issuerCert.size()); issuerCert.clear(); + if (!cplcData.empty()) + burn(cplcData.data(), cplcData.size()); cplcData.clear(); if (m_iccCert.size() && m_issuerCert.size() && m_cplcData.size()) @@ -395,11 +396,9 @@ namespace VeraCrypt m_reader->ApduProcessData(command, response); if (response.getSW() == SW_NO_ERROR && response.getData().size() > 0) { - responseData = response.getData(); - try { - rootNode = TLVParser::TLV_Parse(responseData.data(), responseData.size()); + rootNode = TLVParser::TLV_Parse(response.getData().data(), response.getData().size()); } catch(TLVException) { @@ -410,6 +409,8 @@ namespace VeraCrypt if (iccPublicKeyCertNode && iccPublicKeyCertNode->Value->size() > 0) { iccFound = true; + if (!iccCert.empty()) + burn(iccCert.data(), iccCert.size()); iccCert = *iccPublicKeyCertNode->Value.get(); } @@ -417,6 +418,8 @@ namespace VeraCrypt if (issuerPublicKeyCertNode && issuerPublicKeyCertNode->Value->size() > 0) { issuerFound = true; + if (!issuerCert.empty()) + burn(issuerCert.data(), issuerCert.size()); issuerCert = *issuerPublicKeyCertNode->Value.get(); } } @@ -456,12 +459,12 @@ namespace VeraCrypt bool panFound = false; bool aidSelected = false; vector EMVCardAid; - vector panData; + uint8 panData[2] = { 0 }; + finally_do_arg (uint8 *, panData, { burn (finally_arg, 2); }); shared_ptr rootNode; shared_ptr panNode; CommandAPDU command; ResponseAPDU response; - vector responseData; lastPANDigits = L""; @@ -501,11 +504,9 @@ namespace VeraCrypt m_reader->ApduProcessData(command, response); if (response.getSW() == SW_NO_ERROR && response.getData().size() > 0) { - responseData = response.getData(); - try { - rootNode = TLVParser::TLV_Parse(responseData.data(), responseData.size()); + rootNode = TLVParser::TLV_Parse(response.getData().data(), response.getData().size()); } catch(TLVException) { @@ -516,10 +517,10 @@ namespace VeraCrypt if (panNode && panNode->Value->size() >= 8) { panFound = true; - panData = *panNode->Value.get(); - panData = vector(panData.rbegin(), panData.rbegin() + 2); // only interested in last digits - std::swap(panData[0], panData[1]); - lastPANDigits = ArrayToHexWideString(panData.data(), (int) panData.size()); + // Only copy the last digits, leaving the full PAN in the wiping TLV node. + panData[0] = (*panNode->Value)[panNode->Value->size() - 2]; + panData[1] = panNode->Value->back(); + lastPANDigits = ArrayToHexWideString(panData, (int) sizeof(panData)); } } } @@ -528,9 +529,6 @@ namespace VeraCrypt } } - if (panData.size()) - burn(panData.data(), panData.size()); - if (!aidSelected) throw EMVSelectAIDFailed(); diff --git a/src/Common/ResponseAPDU.cpp b/src/Common/ResponseAPDU.cpp index 3fe3b6de..84692942 100644 --- a/src/Common/ResponseAPDU.cpp +++ b/src/Common/ResponseAPDU.cpp @@ -1,33 +1,18 @@ #include "ResponseAPDU.h" +#include "Tcdefs.h" +#include #include using namespace std; namespace VeraCrypt { - uint16 BytesToUInt16(const vector& buff) - { - uint16 value = 0; - for (uint16 i = 0; i < buff.size(); i++) - { - value <<= 8; - value |= (uint16)buff.at(i); - } - - return value; - } - - void AppendData (vector& buffer, const uint8* pbData, size_t cbData) - { - size_t orgSize = buffer.size (); - buffer.resize (orgSize + cbData); - memcpy (buffer.data () + orgSize, pbData, cbData); - } - /*********************************************************************************/ void ResponseAPDU::clear() { + if (!m_data.empty()) + burn(m_data.data(), m_data.size()); m_data.clear(); m_SW = 0; } @@ -36,10 +21,28 @@ namespace VeraCrypt { } - ResponseAPDU::ResponseAPDU(const vector& data, uint16 SW) + ResponseAPDU::ResponseAPDU(const vector& data, uint16 SW) : m_data(data), m_SW(SW) { - m_data = data; - m_SW = SW; + } + + ResponseAPDU::ResponseAPDU(const ResponseAPDU& other) : m_data(other.m_data), m_SW(other.m_SW) + { + } + + ResponseAPDU& ResponseAPDU::operator=(const ResponseAPDU& other) + { + if (this != &other) + { + ResponseAPDU replacement(other); + m_data.swap(replacement.m_data); + m_SW = other.m_SW; + } + return *this; + } + + ResponseAPDU::~ResponseAPDU() + { + clear(); } uint32 ResponseAPDU::getNr() @@ -47,7 +50,7 @@ namespace VeraCrypt return (uint32)m_data.size(); } - const vector ResponseAPDU::getData() + const vector& ResponseAPDU::getData() const { return m_data; } @@ -67,13 +70,17 @@ namespace VeraCrypt return m_SW; } - const vector ResponseAPDU::getBytes() + vector ResponseAPDU::getBytes() { vector apdu; - - AppendData(apdu, m_data.data(), m_data.size()); - apdu.push_back((uint8)getSW1()); - apdu.push_back((uint8)getSW2()); + if (m_data.size() > apdu.max_size() - 2) + throw length_error("APDU response too large"); + // Allocate the complete output before copying sensitive data. + apdu.resize(m_data.size() + 2); + if (!m_data.empty()) + memcpy(apdu.data(), m_data.data(), m_data.size()); + apdu[m_data.size()] = (uint8)getSW1(); + apdu[m_data.size() + 1] = (uint8)getSW2(); return apdu; } @@ -85,7 +92,34 @@ namespace VeraCrypt void ResponseAPDU::appendData(const uint8* data, size_t dataLen) { - AppendData(m_data, data, dataLen); + if (dataLen == 0) + return; + if (dataLen > m_data.max_size() - m_data.size()) + throw length_error("APDU response too large"); + + size_t oldSize = m_data.size(); + size_t newSize = oldSize + dataLen; + if (newSize > m_data.capacity()) + { + // vector growth would otherwise release the previous contents without wiping. + vector replacement; + size_t capacity = m_data.capacity(); + if (capacity <= m_data.max_size() / 2 && capacity * 2 > newSize) + replacement.reserve(capacity * 2); + replacement.resize(newSize); + if (oldSize > 0) + memcpy(replacement.data(), m_data.data(), oldSize); + memcpy(replacement.data() + oldSize, data, dataLen); + // Copy the appended bytes before erasing, as data may refer to m_data. + if (oldSize > 0) + burn(m_data.data(), oldSize); + m_data.swap(replacement); + } + else + { + m_data.resize(newSize); + memcpy(m_data.data() + oldSize, data, dataLen); + } } void ResponseAPDU::setSW(uint16 SW) @@ -95,17 +129,14 @@ namespace VeraCrypt void ResponseAPDU::setBytes(const vector& bytes) { - clear(); + ResponseAPDU replacement; if (bytes.size() >= 2) { - vector SWBytes; - m_data.resize(bytes.size() - 2); - SWBytes.resize(2); - - memcpy(m_data.data(), bytes.data(), bytes.size() - 2); - memcpy(SWBytes.data(), bytes.data() + bytes.size() - 2, 2); - m_SW = BytesToUInt16(SWBytes); + replacement.appendData(bytes.data(), bytes.size() - 2); + replacement.m_SW = (static_cast(bytes[bytes.size() - 2]) << 8) | bytes.back(); } + m_data.swap(replacement.m_data); + m_SW = replacement.m_SW; } } diff --git a/src/Common/ResponseAPDU.h b/src/Common/ResponseAPDU.h index f279afca..ed847cab 100644 --- a/src/Common/ResponseAPDU.h +++ b/src/Common/ResponseAPDU.h @@ -20,10 +20,14 @@ namespace VeraCrypt ResponseAPDU(); ResponseAPDU(const vector& data, uint16 SW); + ResponseAPDU(const ResponseAPDU& other); + ResponseAPDU& operator=(const ResponseAPDU& other); + ~ResponseAPDU(); uint32 getNr(); - const vector getData(); + // The returned data remains owned by this response and is invalidated by mutation. + const vector& getData() const; uint8 getSW1(); @@ -31,7 +35,8 @@ namespace VeraCrypt uint16 getSW(); - const vector getBytes(); + // The caller owns this serialized copy and is responsible for erasing it. + vector getBytes(); void setSW(uint16 SW); void setBytes(const vector& bytes); @@ -41,4 +46,4 @@ namespace VeraCrypt }; }; -#endif // TC_HEADER_Common_ResponseAPDU \ No newline at end of file +#endif // TC_HEADER_Common_ResponseAPDU diff --git a/src/Common/SCardReader.cpp b/src/Common/SCardReader.cpp index 6c5f654a..67bf31e5 100644 --- a/src/Common/SCardReader.cpp +++ b/src/Common/SCardReader.cpp @@ -1,5 +1,6 @@ #include "SCardReader.h" #include "PCSCException.h" +#include "Platform/Finally.h" #include @@ -399,7 +400,9 @@ namespace VeraCrypt size_t indexOfLcData = 0; vector pbSendBuffer; + finally_do_arg (vector *, &pbSendBuffer, { if (!finally_arg->empty()) burn (finally_arg->data(), finally_arg->size()); }); vector pbRecvBuffer; + finally_do_arg (vector *, &pbRecvBuffer, { if (!finally_arg->empty()) burn (finally_arg->data(), finally_arg->size()); }); DWORD cbSendLength = 0; DWORD cbRecvLength = 0; @@ -639,9 +642,6 @@ namespace VeraCrypt end: - burn(pbSendBuffer.data(), pbSendBuffer.size()); - burn(pbRecvBuffer.data(), pbRecvBuffer.size()); - if (lRet != SCARD_S_SUCCESS) throw PCSCException(lRet); } diff --git a/src/Common/TLVParser.cpp b/src/Common/TLVParser.cpp index 4a9ce5ee..60ead288 100644 --- a/src/Common/TLVParser.cpp +++ b/src/Common/TLVParser.cpp @@ -46,7 +46,7 @@ namespace VeraCrypt } /* Parsing one TLV node */ - shared_ptr TLVParser::TLV_Parse_One(uint8* buf, size_t size) + shared_ptr TLVParser::TLV_Parse_One(const uint8* buf, size_t size) { size_t index = 0; size_t i = 0; @@ -205,7 +205,7 @@ namespace VeraCrypt } /* Parsing TLV from a buffer and constructing TLV structure */ - shared_ptr TLVParser::TLV_Parse(uint8* buf, size_t size) + shared_ptr TLVParser::TLV_Parse(const uint8* buf, size_t size) { shared_ptr node = TLV_Parse_One(buf, size); TLV_Parse_Sub(node); diff --git a/src/Common/TLVParser.h b/src/Common/TLVParser.h index 2fc4a27c..f082277d 100644 --- a/src/Common/TLVParser.h +++ b/src/Common/TLVParser.h @@ -40,7 +40,7 @@ namespace VeraCrypt static uint16 CheckBit(uint8 value, int bit); /* Parsing one TLV node */ - static shared_ptr TLV_Parse_One(uint8* buf, size_t size); + static shared_ptr TLV_Parse_One(const uint8* buf, size_t size); /* Parsing all TLV nodes */ static int TLV_Parse_SubNodes(shared_ptr parent); @@ -54,7 +54,7 @@ namespace VeraCrypt public: /* Parsing TLV from a buffer and constructing TLV structure */ - static shared_ptr TLV_Parse(uint8* buf, size_t size); + static shared_ptr TLV_Parse(const uint8* buf, size_t size); /* Finding a TLV node with a particular tag */ static shared_ptr TLV_Find(shared_ptr node, uint16 tag);