From 82ea1cf35a2b10bb7a2cfbaa0d9bac98fe5bb182 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 a59a7e1f5..4ade67d23 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 ef5639bfbbeb7d83bd66e3faf3e88e4aa1e05a6e 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 4ade67d23..14d53bef9 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 e70c08b543f1af569a59c672f707128cc31d341d 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 14d53bef9..1d77fda79 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 06898263f6ef8dc4294a8c7554023320c27b957c 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 1d77fda79..d2ce4f5bf 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 6da2ed4232d3b3ecde7a04621c554f66de558fab 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 +++++-- 1 file changed, 5 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 d2ce4f5bf..fd0786068 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; } -- cgit v1.2.3