From b7c947fed32468867ca2fc191b786060fbce88b3 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 13 Oct 2018 08:08:41 -0400 Subject: key_manager: Remove unnecessary seek in DeriveSDSeed() Given the file is opened a few lines above and no operations are done, other than check if the file is in a valid state, the read/write pointer will always be at the beginning of the file. --- src/core/crypto/key_manager.cpp | 1 - 1 file changed, 1 deletion(-) (limited to 'src/core/crypto/key_manager.cpp') diff --git a/src/core/crypto/key_manager.cpp b/src/core/crypto/key_manager.cpp index a59a7e1f57..4ade67d236 100644 --- a/src/core/crypto/key_manager.cpp +++ b/src/core/crypto/key_manager.cpp @@ -152,7 +152,6 @@ boost::optional DeriveSDSeed() { if (!sd_private.IsOpen()) return boost::none; - sd_private.Seek(0, SEEK_SET); std::array private_seed{}; if (sd_private.ReadBytes(private_seed.data(), private_seed.size()) != 0x10) return boost::none; -- cgit v1.2.3 From 214d206020a38ff5abeff2bd145e4fcb86bd71da Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 13 Oct 2018 08:12:00 -0400 Subject: key_manager: Don't assume file seeks and reads will always succeed Given the filesystem should always be assumed to be volatile, we should check and bail out if a seek operation isn't successful. This'll prevent potentially writing/returning garbage data from the function in rare cases. This also allows removing a check to see if an offset is within the bounds of a file before perfoming a seek operation. If a seek is attempted beyond the end of a file, it will fail, so this essentially combines two checks into one in one place. --- src/core/crypto/key_manager.cpp | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) (limited to 'src/core/crypto/key_manager.cpp') diff --git a/src/core/crypto/key_manager.cpp b/src/core/crypto/key_manager.cpp index 4ade67d236..14d53bef9a 100644 --- a/src/core/crypto/key_manager.cpp +++ b/src/core/crypto/key_manager.cpp @@ -147,30 +147,38 @@ boost::optional DeriveSDSeed() { "rb+"); if (!save_43.IsOpen()) return boost::none; + const FileUtil::IOFile sd_private( FileUtil::GetUserPath(FileUtil::UserPath::SDMCDir) + "/Nintendo/Contents/private", "rb+"); if (!sd_private.IsOpen()) return boost::none; std::array private_seed{}; - if (sd_private.ReadBytes(private_seed.data(), private_seed.size()) != 0x10) + if (sd_private.ReadBytes(private_seed.data(), private_seed.size()) != private_seed.size()) { return boost::none; + } std::array buffer{}; std::size_t offset = 0; for (; offset + 0x10 < save_43.GetSize(); ++offset) { - save_43.Seek(offset, SEEK_SET); + if (!save_43.Seek(offset, SEEK_SET)) { + return boost::none; + } + save_43.ReadBytes(buffer.data(), buffer.size()); - if (buffer == private_seed) + if (buffer == private_seed) { break; + } } - if (offset + 0x10 >= save_43.GetSize()) + if (!save_43.Seek(offset + 0x10, SEEK_SET)) { return boost::none; + } Key128 seed{}; - save_43.Seek(offset + 0x10, SEEK_SET); - save_43.ReadBytes(seed.data(), seed.size()); + if (save_43.ReadBytes(seed.data(), seed.size()) != seed.size()) { + return boost::none; + } return seed; } @@ -233,7 +241,9 @@ std::vector GetTicketblob(const FileUtil::IOFile& ticket_save) { return {}; std::vector buffer(ticket_save.GetSize()); - ticket_save.ReadBytes(buffer.data(), buffer.size()); + if (ticket_save.ReadBytes(buffer.data(), buffer.size()) != buffer.size()) { + return {}; + } std::vector out; u32 magic{}; -- cgit v1.2.3 From 5323ae4af4d443b9123b4ec1f00f24da5cde477c Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 13 Oct 2018 08:14:38 -0400 Subject: key_manager: Brace long conditional body If a conditional (or it's body) travels more than one line, it should be braced. --- src/core/crypto/key_manager.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/core/crypto/key_manager.cpp') diff --git a/src/core/crypto/key_manager.cpp b/src/core/crypto/key_manager.cpp index 14d53bef9a..1d77fda79e 100644 --- a/src/core/crypto/key_manager.cpp +++ b/src/core/crypto/key_manager.cpp @@ -308,10 +308,11 @@ boost::optional> ParseTicket(const TicketRaw& ticket, std::memcpy(&cert_authority, ticket.data() + 0x140, sizeof(cert_authority)); if (cert_authority == 0) return boost::none; - if (cert_authority != Common::MakeMagic('R', 'o', 'o', 't')) + if (cert_authority != Common::MakeMagic('R', 'o', 'o', 't')) { LOG_INFO(Crypto, "Attempting to parse ticket with non-standard certificate authority {:08X}.", cert_authority); + } Key128 rights_id; std::memcpy(rights_id.data(), ticket.data() + 0x2A0, sizeof(Key128)); -- cgit v1.2.3 From f50401aa7f4eff35db6690afe635d7f615bbcf97 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 13 Oct 2018 08:28:15 -0400 Subject: key_manager: Use std::vector's insert() instead of std::copy with a back_inserter If the data is unconditionally being appended to the back of a std::vector, we can just directly insert it there without the need to insert all of the elements one-by-one with a std::back_inserter. --- src/core/crypto/key_manager.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/core/crypto/key_manager.cpp') diff --git a/src/core/crypto/key_manager.cpp b/src/core/crypto/key_manager.cpp index 1d77fda79e..d2ce4f5bf5 100644 --- a/src/core/crypto/key_manager.cpp +++ b/src/core/crypto/key_manager.cpp @@ -881,9 +881,9 @@ void KeyManager::DeriveETicket(PartitionDataManager& data) { "/system/save/80000000000000e2", "rb+"); + const auto blob2 = GetTicketblob(save2); auto res = GetTicketblob(save1); - const auto res2 = GetTicketblob(save2); - std::copy(res2.begin(), res2.end(), std::back_inserter(res)); + res.insert(res.end(), blob2.begin(), blob2.end()); for (const auto& raw : res) { const auto pair = ParseTicket(raw, rsa_key); -- cgit v1.2.3 From e21cda8767466211fdc41fb15ea05f3eb26d9d6a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 13 Oct 2018 09:13:19 -0400 Subject: key_manager/partition_data_manager: Silence truncation compiler warnings --- src/core/crypto/key_manager.cpp | 7 +++++-- src/core/crypto/key_manager.h | 2 +- src/core/crypto/partition_data_manager.cpp | 12 +++++++----- src/core/crypto/partition_data_manager.h | 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) (limited to 'src/core/crypto/key_manager.cpp') diff --git a/src/core/crypto/key_manager.cpp b/src/core/crypto/key_manager.cpp index d2ce4f5bf5..fd07860684 100644 --- a/src/core/crypto/key_manager.cpp +++ b/src/core/crypto/key_manager.cpp @@ -98,7 +98,7 @@ std::array DecryptKeyblob(const std::array& encrypted_keyblob, return keyblob; } -void KeyManager::DeriveGeneralPurposeKeys(u8 crypto_revision) { +void KeyManager::DeriveGeneralPurposeKeys(std::size_t crypto_revision) { const auto kek_generation_source = GetKey(S128KeyType::Source, static_cast(SourceKeyType::AESKekGeneration)); const auto key_generation_source = @@ -270,6 +270,9 @@ static std::array operator^(const std::array& lhs, template static std::array MGF1(const std::array& seed) { + // Avoids truncation overflow within the loop below. + static_assert(target_size <= 0xFF); + std::array seed_exp{}; std::memcpy(seed_exp.data(), seed.data(), in_size); @@ -277,7 +280,7 @@ static std::array MGF1(const std::array& seed) { size_t i = 0; while (out.size() < target_size) { out.resize(out.size() + 0x20); - seed_exp[in_size + 3] = i; + seed_exp[in_size + 3] = static_cast(i); mbedtls_sha256(seed_exp.data(), seed_exp.size(), out.data() + out.size() - 0x20, 0); ++i; } diff --git a/src/core/crypto/key_manager.h b/src/core/crypto/key_manager.h index a41abbdfcd..cccb3c0aec 100644 --- a/src/core/crypto/key_manager.h +++ b/src/core/crypto/key_manager.h @@ -175,7 +175,7 @@ private: void WriteKeyToFile(KeyCategory category, std::string_view keyname, const std::array& key); - void DeriveGeneralPurposeKeys(u8 crypto_revision); + void DeriveGeneralPurposeKeys(std::size_t crypto_revision); void SetKeyWrapped(S128KeyType id, Key128 key, u64 field1 = 0, u64 field2 = 0); void SetKeyWrapped(S256KeyType id, Key256 key, u64 field1 = 0, u64 field2 = 0); diff --git a/src/core/crypto/partition_data_manager.cpp b/src/core/crypto/partition_data_manager.cpp index bef8cdaf00..51d89508bd 100644 --- a/src/core/crypto/partition_data_manager.cpp +++ b/src/core/crypto/partition_data_manager.cpp @@ -332,7 +332,8 @@ FileSys::VirtualFile PartitionDataManager::GetBoot0Raw() const { return boot0; } -PartitionDataManager::EncryptedKeyBlob PartitionDataManager::GetEncryptedKeyblob(u8 index) const { +PartitionDataManager::EncryptedKeyBlob PartitionDataManager::GetEncryptedKeyblob( + std::size_t index) const { if (HasBoot0() && index < NUM_ENCRYPTED_KEYBLOBS) return GetEncryptedKeyblobs()[index]; return {}; @@ -389,7 +390,7 @@ std::array PartitionDataManager::GetKeyblobMACKeySource() const { return FindKeyFromHex(package1_decrypted_bytes, source_hashes[0]); } -std::array PartitionDataManager::GetKeyblobKeySource(u8 revision) const { +std::array PartitionDataManager::GetKeyblobKeySource(std::size_t revision) const { if (keyblob_source_hashes[revision] == SHA256Hash{}) { LOG_WARNING(Crypto, "No keyblob source hash for crypto revision {:02X}! Cannot derive keys...", @@ -456,11 +457,12 @@ void PartitionDataManager::DecryptPackage2(std::array, 0x20> if (file->ReadObject(&header) != sizeof(Package2Header)) return; - u8 revision = 0xFF; + std::size_t revision = 0xFF; if (header.magic != Common::MakeMagic('P', 'K', '2', '1')) { - for (size_t i = 0; i < package2_keys.size(); ++i) { - if (AttemptDecrypt(package2_keys[i], header)) + for (std::size_t i = 0; i < package2_keys.size(); ++i) { + if (AttemptDecrypt(package2_keys[i], header)) { revision = i; + } } } diff --git a/src/core/crypto/partition_data_manager.h b/src/core/crypto/partition_data_manager.h index 7c9c4410a9..9e448f7205 100644 --- a/src/core/crypto/partition_data_manager.h +++ b/src/core/crypto/partition_data_manager.h @@ -34,7 +34,7 @@ public: // BOOT0 bool HasBoot0() const; FileSys::VirtualFile GetBoot0Raw() const; - EncryptedKeyBlob GetEncryptedKeyblob(u8 index) const; + EncryptedKeyBlob GetEncryptedKeyblob(std::size_t index) const; EncryptedKeyBlobs GetEncryptedKeyblobs() const; std::vector GetSecureMonitor() const; std::array GetPackage2KeySource() const; @@ -46,7 +46,7 @@ public: std::vector GetPackage1Decrypted() const; std::array GetMasterKeySource() const; std::array GetKeyblobMACKeySource() const; - std::array GetKeyblobKeySource(u8 revision) const; + std::array GetKeyblobKeySource(std::size_t revision) const; // Fuses bool HasFuses() const; -- cgit v1.2.3