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);