From 07a97c0f6310da216f033ee4cd0fed7be005cf19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Mon, 25 Mar 2024 22:03:11 +0000 Subject: [PATCH 01/10] SDK-3787. Avoid cloning a node with a zerokey Context: When putting a new node, the nodekey is obfuscated with the client's master key, generating a new 32 bytes key which is sent to the API. Issue: The API will reject this key if the first 16 bytes are equal to the last 16 bytes, which means that the key is either a zero key or was generated with a zero key (i.e., the transferkey used to produce the filekey/nodekey was a zero key). Notice that either if the nodekey is a zerokey (all bytes are zero) or it was generated with a zerokey (in this case, its first 16 bytes will be equal to the last 16 bytes), the obfuscated key which is sent to the API will also suffer from this condition: the first 16 bytes will be equal to the last 16 bytes. How this issue is currently handled: When this happens, we will receive an API_EKEY (-14) error for that node, and the transfer will be restarted. When the transfer is restarted, a new fresh transfer key will be generated, changing the final filekey and the nodekey which will be used. This is, no matter whether the zerokey was the transfer key, the file key or the node key, all of them will be substituted after the new transfer. What is not currently handled: If the node was already cached and a new upload detects that node and consider it as a valid node, this node will be cloned for the putnodes. If the nodekey of that node was a zerokey (for example, if the serialized data was corrupted or empty), this will generate an infinite loop with no solution. __________________ Solution: This branch adds the following code to avoid the above: 1) Add logic to check whether a key is a zero key. This can be used for 16-byte generator keys and for 32-byte file keys. 2) Check if a node to be cloned has a zero key: discard it if it is the case. 3) Add asserts and logs to check the validity of the nodekey on CommandPutNodes. --- include/mega/node.h | 63 +++++++++++++++++++++++++++++--------------- include/mega/utils.h | 6 +++++ src/commands.cpp | 22 +++++++++++++++- src/node.cpp | 6 +++++ src/share.cpp | 3 +-- src/utils.cpp | 30 +++++++++++++++++++++ 6 files changed, 106 insertions(+), 24 deletions(-) diff --git a/include/mega/node.h b/include/mega/node.h index 629dcd4de1..41847770a8 100644 --- a/include/mega/node.h +++ b/include/mega/node.h @@ -77,27 +77,6 @@ struct CloudNode bool isIgnoreFile() const; }; -// new node for putnodes() -struct MEGA_API NewNode : public NodeCore -{ - string nodekey; - - newnodesource_t source = NEW_NODE; - - NodeHandle ovhandle; - UploadHandle uploadhandle; - UploadToken uploadtoken; - - std::unique_ptr fileattributes; - - // versioning used for this new node, forced at server's side regardless the account's value - VersioningOption mVersioningOption = NoVersioning; - bool added = false; // set true when the actionpacket arrives - bool canChangeVault = false; - handle mAddedHandle = UNDEF; // updated as actionpacket arrives - error mError = API_OK; // per-node error (updated in cs response) -}; - struct MEGA_API PublicLink { handle ph; @@ -217,6 +196,12 @@ struct MEGA_API Node : public NodeCore, FileFingerprint // check if the key is present and is the correct size for this node bool keyApplied() const; + // Check whether the node key is a zero key or was generated by a zero key (so it is a bad key and it will be rejected by the API). + bool hasZeroKey() const; + + // Static version of the above for related node classes. + static bool hasZeroKey(const string& nodekeydata); + // change parent node association. updateNodeCounters is false when called from NodeManager::unserializeNode bool setparent(std::shared_ptr , bool updateNodeCounters = true); @@ -476,6 +461,42 @@ inline bool Node::keyApplied() const return nodekeydata.size() == size_t((type == FILENODE) ? FILENODEKEYLENGTH : FOLDERNODEKEYLENGTH); } +inline bool Node::hasZeroKey() const +{ + return keyApplied() && isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); +} + +inline bool Node::hasZeroKey(const string& nodekeydata) +{ + return ((nodekeydata.size() == FILENODEKEYLENGTH) || (nodekeydata.size() == FOLDERNODEKEYLENGTH)) && + isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); +} + +// END MEGA_API Node + +// new node for putnodes() +struct MEGA_API NewNode : public NodeCore +{ + string nodekey; + + newnodesource_t source = NEW_NODE; + + NodeHandle ovhandle; + UploadHandle uploadhandle; + UploadToken uploadtoken; + + std::unique_ptr fileattributes; + + // versioning used for this new node, forced at server's side regardless the account's value + VersioningOption mVersioningOption = NoVersioning; + bool added = false; // set true when the actionpacket arrives + bool canChangeVault = false; + handle mAddedHandle = UNDEF; // updated as actionpacket arrives + error mError = API_OK; // per-node error (updated in cs response) + + bool hasZeroKey() const { return Node::hasZeroKey(nodekey); } +}; + class NodeData { public: diff --git a/include/mega/utils.h b/include/mega/utils.h index af3d45c6b0..020e863f9c 100644 --- a/include/mega/utils.h +++ b/include/mega/utils.h @@ -1082,6 +1082,12 @@ bool is_digit(unsigned int ch); // Get the current process ID unsigned long getCurrentPid(); +// Check whether a key is a zerokey +// This applies to keys whose length is: +// SymmCipher:BLOCKSIZE (16) (used for generator keys, ex: Transfer::transferkey; also folder node keys and general node keys) +// FILENODEKEYLENGTH (32 or 2*SymmCipher::BLOCKSIZE) -> AES32 file/node keys +bool isZeroKey(const byte* key, size_t keySize); + } // namespace mega #endif // MEGA_UTILS_H diff --git a/src/commands.cpp b/src/commands.cpp index 72040bcd21..2952d79083 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -1160,9 +1160,11 @@ CommandPutNodes::CommandPutNodes(MegaClient* client, NodeHandle th, if (!client->loggedIntoWritableFolder()) { + assert(!nn[i].hasZeroKey()); // Add this assert here to avoid extra checks in production -> we will have a check and logs if this fails within CommandPutNode::procresult if (nn[i].nodekey.size() <= sizeof key) { client->key.ecb_encrypt((byte*)nn[i].nodekey.data(), key, nn[i].nodekey.size()); + assert(!isZeroKey(key, FILENODEKEYLENGTH)); arg("k", key, int(nn[i].nodekey.size())); } else @@ -1274,7 +1276,24 @@ bool CommandPutNodes::procresult(Result r, JSON& json) assert(arrayIndex < nn.size()); if (arrayIndex < nn.size()) { - nn[arrayIndex++].mError = error(json.getint()); + nn[arrayIndex].mError = error(json.getint()); + if (nn[arrayIndex].mError == API_EKEY) + { + // Check if the error was due to a zerokey. + // We check this here to avoid a lot of unnecessary checks within CommandPutNodes constructor, where we send the key. + // For the constructor we just add asserts for the debug mode. Here we can add checks and logs. + if (nn[arrayIndex].hasZeroKey()) + { + LOG_err << "[CommandPutNodes] New Node failed with API_EKEY has a zerokey!!!!" << " [index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + assert(false && "New Node which failed with API_EKEY has a zerokey!!!!"); + // sendevent? The API already sends an event for this scenario + } + else + { + LOG_warn << "[CommandPutNodes] New Node failed with API_EKEY !!" << " [index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + } + } + arrayIndex++; } } else @@ -3793,6 +3812,7 @@ CommandNodeKeyUpdate::CommandNodeKeyUpdate(MegaClient* client, handle_vector* v) if ((n = client->nodebyhandle(h))) { client->key.ecb_encrypt((byte*)n->nodekey().data(), nodekey, n->nodekey().size()); + assert(!n->hasZeroKey()); element(h, MegaClient::NODEHANDLE); element(nodekey, int(n->nodekey().size())); diff --git a/src/node.cpp b/src/node.cpp index bb3b2a3857..b319534e33 100644 --- a/src/node.cpp +++ b/src/node.cpp @@ -3107,6 +3107,12 @@ void LocalNode::queueClientUpload(shared_ptr upload, Versio if (mc.treatAsIfFileDataEqual(*n, ext1, *upload, ext2)) { cloneNode = n.get(); + if (cloneNode->hasZeroKey()) + { + LOG_warn << "Clone node key is a zero key!!!!!!!!! -> avoiding cloning node to regenerate the key [cloneNode path = '" << cloneNode->displaypath() << "', sourceLocalname = '" << upload->sourceLocalname << "']"; + assert(false && "Clone node key is a zero key!!!!!!!!!"); + cloneNode = nullptr; + } break; } } diff --git a/src/share.cpp b/src/share.cpp index 6447815abf..eaab5ccc57 100644 --- a/src/share.cpp +++ b/src/share.cpp @@ -92,8 +92,7 @@ NewShare::NewShare(handle ch, int coutgoing, handle cpeer, accesslevel_t caccess upgrade_pending_to_full = cupgrade_pending_to_full; remove_key = okremoved; - static const std::vector zeroKey(SymmCipher::BLOCKSIZE, 0); - if (ckey && memcmp(ckey, zeroKey.data(), zeroKey.size())) + if (ckey && !isZeroKey(ckey, SymmCipher::BLOCKSIZE)) { memcpy(key, ckey, sizeof key); have_key = 1; diff --git a/src/utils.cpp b/src/utils.cpp index b27e92961c..d85621df15 100644 --- a/src/utils.cpp +++ b/src/utils.cpp @@ -3346,5 +3346,35 @@ unsigned long getCurrentPid() #endif } +bool isZeroKey(const byte* key, size_t keySize) +{ + if (!key) + { + // Invalid key pointer, consider it non-zero + LOG_warn << "[isZeroKey] invalid key pointer"; + assert(false && "[isZeroKey] invalid key pointer"); + return false; + } + + if (keySize == FILENODEKEYLENGTH) // 32 (filekey, nodekey, etc) + { + assert(FILENODEKEYLENGTH == SymmCipher::BLOCKSIZE * 2); + // Check if the lower 16 bytes (0-15) are equal to the higher 16 bytes (16-31) + // This will be true either if the key is all zeros or it was generated with a 16-byte zero key (for example, the transferkey was a zerokey). + return std::memcmp(&key[0], &key[SymmCipher::BLOCKSIZE], SymmCipher::BLOCKSIZE) == 0; + } + else if (keySize == SymmCipher::BLOCKSIZE) // 16 (transfer key, client master key, etc) + { + // Check if all bytes are zero (zerokey) + byte zeroKey[SymmCipher::BLOCKSIZE] = {}; + return std::memcmp(&key[0], zeroKey, SymmCipher::BLOCKSIZE) == 0; + } + + // Invalid key size, consider it non-zero + LOG_warn << "[isZeroKey] used a keySize(" << keySize << ") different from 32 and 16 -> function will return false"; + assert(false && "isZeroKey used a keySize different from 32 and 16"); + return false; +} + } // namespace mega From 4d70af51757f9155fd49f0e879f3c920bd8ef7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 00:45:37 +0000 Subject: [PATCH 02/10] Use node error returned from API instead of API_ENOENT for transfer failure on CommandPutNodes When putnodes fail, we receive an API response whose param "f" is empty, and the transfer fails with API_ENOENT (not found). However, when there is a specific node failure (for example, API_EKEY -14 for a node using a zero key or a bad key), it would be more approppriate to use the specific error for the transfer failure. Otherwise, the error returned can be misleading:while it is true that the put node cannot be found (it is empty because the operation didn't work), that's a generic error which doesn't give information on the specific problem leading to an empty response for putnodes. --- src/commands.cpp | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index 2952d79083..287053aedc 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -1257,6 +1257,7 @@ bool CommandPutNodes::procresult(Result r, JSON& json) // If the first three nodes failed, the response would be e.g. [-9,-9,-9]. Success is [] // If the second and third node failed, the response would change to {"1":-9,"2":-9}. + Error newNodeError(API_OK); unsigned arrayIndex = 0; for (;;) { @@ -1277,20 +1278,25 @@ bool CommandPutNodes::procresult(Result r, JSON& json) if (arrayIndex < nn.size()) { nn[arrayIndex].mError = error(json.getint()); - if (nn[arrayIndex].mError == API_EKEY) + if (nn[arrayIndex].mError != API_OK) { - // Check if the error was due to a zerokey. - // We check this here to avoid a lot of unnecessary checks within CommandPutNodes constructor, where we send the key. - // For the constructor we just add asserts for the debug mode. Here we can add checks and logs. - if (nn[arrayIndex].hasZeroKey()) + newNodeError = nn[arrayIndex].mError; + LOG_debug << "[CommandPutNodes] New Node failed with " << newNodeError << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + if (nn[arrayIndex].mError == API_EKEY) { - LOG_err << "[CommandPutNodes] New Node failed with API_EKEY has a zerokey!!!!" << " [index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; - assert(false && "New Node which failed with API_EKEY has a zerokey!!!!"); - // sendevent? The API already sends an event for this scenario - } - else - { - LOG_warn << "[CommandPutNodes] New Node failed with API_EKEY !!" << " [index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + // Check if the error was due to a zerokey. + // We check this here to avoid a lot of unnecessary checks within CommandPutNodes constructor, where we send the key. + // For the constructor we just add asserts for the debug mode. Here we can add checks and logs. + if (nn[arrayIndex].hasZeroKey()) + { + LOG_err << "[CommandPutNodes] New Node failed with API_EKEY has a zerokey!!!!" << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + assert(false && "New Node which failed with API_EKEY has a zerokey!!!!"); + // sendevent? The API already sends an event for this scenario + } + else + { + LOG_warn << "[CommandPutNodes] New Node failed with API_EKEY !!" << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + } } } arrayIndex++; @@ -1341,7 +1347,8 @@ bool CommandPutNodes::procresult(Result r, JSON& json) shared_ptr tempNode = !nn.empty() ? client->nodebyhandle(nn.front().mAddedHandle) : nullptr; bool targetOverride = (tempNode.get() && NodeHandle().set6byte(tempNode->parenthandle) != targethandle); - performAppCallback(emptyResponse ? API_ENOENT : API_OK, nn, targetOverride); + performAppCallback(emptyResponse ? ((newNodeError != API_OK) ? static_cast(newNodeError) : API_ENOENT) : // Add last new node error if there is any, otherwise API_ENOENT + API_OK, nn, targetOverride); return true; } else From 8fdff7e1bc17a8c2b7f7e8164cfdd46f73a22a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 05:15:24 +0000 Subject: [PATCH 03/10] Adapt std::memcmp to use directly the pointer rather than the reference to the first object --- src/utils.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils.cpp b/src/utils.cpp index d85621df15..d1ad415092 100644 --- a/src/utils.cpp +++ b/src/utils.cpp @@ -3361,13 +3361,13 @@ bool isZeroKey(const byte* key, size_t keySize) assert(FILENODEKEYLENGTH == SymmCipher::BLOCKSIZE * 2); // Check if the lower 16 bytes (0-15) are equal to the higher 16 bytes (16-31) // This will be true either if the key is all zeros or it was generated with a 16-byte zero key (for example, the transferkey was a zerokey). - return std::memcmp(&key[0], &key[SymmCipher::BLOCKSIZE], SymmCipher::BLOCKSIZE) == 0; + return std::memcmp(key /*key[0-15]*/, key + SymmCipher::BLOCKSIZE /*key[16-31]*/, SymmCipher::BLOCKSIZE) == 0; } else if (keySize == SymmCipher::BLOCKSIZE) // 16 (transfer key, client master key, etc) { // Check if all bytes are zero (zerokey) - byte zeroKey[SymmCipher::BLOCKSIZE] = {}; - return std::memcmp(&key[0], zeroKey, SymmCipher::BLOCKSIZE) == 0; + static const byte zeroKey[SymmCipher::BLOCKSIZE] = {}; + return std::memcmp(key, zeroKey, SymmCipher::BLOCKSIZE) == 0; } // Invalid key size, consider it non-zero From dcc3acf892f22ced75489887039af0ccc0eb0a5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 13:46:18 +0000 Subject: [PATCH 04/10] Move isZeroKey method from utils to cryptopp module --- include/mega/crypto/cryptopp.h | 13 +++++++++++++ include/mega/node.h | 4 ++-- include/mega/utils.h | 6 ------ src/commands.cpp | 2 +- src/crypto/cryptopp.cpp | 30 ++++++++++++++++++++++++++++++ src/share.cpp | 2 +- src/utils.cpp | 30 ------------------------------ 7 files changed, 47 insertions(+), 40 deletions(-) diff --git a/include/mega/crypto/cryptopp.h b/include/mega/crypto/cryptopp.h index 83f5ce085f..57ddc906e4 100644 --- a/include/mega/crypto/cryptopp.h +++ b/include/mega/crypto/cryptopp.h @@ -434,6 +434,19 @@ class MEGA_API SymmCipher static void incblock(byte*, unsigned = BLOCKSIZE); + /** + * @brief Check whether a key is a zerokey, or was generated with a zerokey + * + * This applies to keys whose length is: + * SymmCipher:BLOCKSIZE (16) (used for generator keys, ex: Transfer::transferkey; also folder node keys and general node keys) + * FILENODEKEYLENGTH (32 or 2*SymmCipher::BLOCKSIZE) -> AES32 file/node keys + * + * @param key Encryption/Decryption key + * @param keySize Encryption/Decryption key length (expected BLOCKSIZE or BLOCKSIZE*2) + * @return true if the key is a zero key or was generated with a zero key + */ + static bool isZeroKey(const byte* key, size_t keySize); + SymmCipher() { } SymmCipher(const SymmCipher& ref); SymmCipher& operator=(const SymmCipher& ref); diff --git a/include/mega/node.h b/include/mega/node.h index 41847770a8..4430905ace 100644 --- a/include/mega/node.h +++ b/include/mega/node.h @@ -463,13 +463,13 @@ inline bool Node::keyApplied() const inline bool Node::hasZeroKey() const { - return keyApplied() && isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); + return keyApplied() && SymmCipher::isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); } inline bool Node::hasZeroKey(const string& nodekeydata) { return ((nodekeydata.size() == FILENODEKEYLENGTH) || (nodekeydata.size() == FOLDERNODEKEYLENGTH)) && - isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); + SymmCipher::isZeroKey(reinterpret_cast(nodekeydata.data()), nodekeydata.size()); } // END MEGA_API Node diff --git a/include/mega/utils.h b/include/mega/utils.h index 020e863f9c..af3d45c6b0 100644 --- a/include/mega/utils.h +++ b/include/mega/utils.h @@ -1082,12 +1082,6 @@ bool is_digit(unsigned int ch); // Get the current process ID unsigned long getCurrentPid(); -// Check whether a key is a zerokey -// This applies to keys whose length is: -// SymmCipher:BLOCKSIZE (16) (used for generator keys, ex: Transfer::transferkey; also folder node keys and general node keys) -// FILENODEKEYLENGTH (32 or 2*SymmCipher::BLOCKSIZE) -> AES32 file/node keys -bool isZeroKey(const byte* key, size_t keySize); - } // namespace mega #endif // MEGA_UTILS_H diff --git a/src/commands.cpp b/src/commands.cpp index 287053aedc..0b72086c8d 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -1164,7 +1164,7 @@ CommandPutNodes::CommandPutNodes(MegaClient* client, NodeHandle th, if (nn[i].nodekey.size() <= sizeof key) { client->key.ecb_encrypt((byte*)nn[i].nodekey.data(), key, nn[i].nodekey.size()); - assert(!isZeroKey(key, FILENODEKEYLENGTH)); + assert(!SymmCipher::isZeroKey(key, FILENODEKEYLENGTH)); arg("k", key, int(nn[i].nodekey.size())); } else diff --git a/src/crypto/cryptopp.cpp b/src/crypto/cryptopp.cpp index f4d67e34f5..aed65ae3d6 100644 --- a/src/crypto/cryptopp.cpp +++ b/src/crypto/cryptopp.cpp @@ -639,6 +639,36 @@ void SymmCipher::incblock(byte* dst, unsigned len) } } +bool SymmCipher::isZeroKey(const byte* key, size_t keySize) +{ + if (!key) + { + // Invalid key pointer, consider it non-zero + LOG_warn << "[SymmCipher::isZeroKey] invalid key pointer"; + assert(false && "[SymmCipher::isZeroKey] invalid key pointer"); + return false; + } + + if (keySize == FILENODEKEYLENGTH) // 32 (filekey, nodekey, etc) + { + static_assert(FILENODEKEYLENGTH == SymmCipher::BLOCKSIZE * 2); + // Check if the lower 16 bytes (0-15) are equal to the higher 16 bytes (16-31) + // This will be true either if the key is all zeros or it was generated with a 16-byte zero key (for example, the transferkey was a zerokey). + return std::memcmp(key /*key[0-15]*/, key + SymmCipher::BLOCKSIZE /*key[16-31]*/, SymmCipher::BLOCKSIZE) == 0; + } + else if (keySize == SymmCipher::BLOCKSIZE) // 16 (transfer key, client master key, etc) + { + // Check if all bytes are zero (zerokey) + static const byte zeroKey[SymmCipher::BLOCKSIZE] = {}; + return std::memcmp(key, zeroKey, SymmCipher::BLOCKSIZE) == 0; + } + + // Invalid key size, consider it non-zero + LOG_warn << "[SymmCipher::isZeroKey] used a keySize(" << keySize << ") different from 32 and 16 -> function will return false"; + assert(false && "SymmCipher::isZeroKey used a keySize different from 32 and 16"); + return false; +} + SymmCipher::SymmCipher(const SymmCipher &ref) { setkey(ref.key); diff --git a/src/share.cpp b/src/share.cpp index eaab5ccc57..8e85005b41 100644 --- a/src/share.cpp +++ b/src/share.cpp @@ -92,7 +92,7 @@ NewShare::NewShare(handle ch, int coutgoing, handle cpeer, accesslevel_t caccess upgrade_pending_to_full = cupgrade_pending_to_full; remove_key = okremoved; - if (ckey && !isZeroKey(ckey, SymmCipher::BLOCKSIZE)) + if (ckey && !SymmCipher::isZeroKey(ckey, SymmCipher::BLOCKSIZE)) { memcpy(key, ckey, sizeof key); have_key = 1; diff --git a/src/utils.cpp b/src/utils.cpp index d1ad415092..b27e92961c 100644 --- a/src/utils.cpp +++ b/src/utils.cpp @@ -3346,35 +3346,5 @@ unsigned long getCurrentPid() #endif } -bool isZeroKey(const byte* key, size_t keySize) -{ - if (!key) - { - // Invalid key pointer, consider it non-zero - LOG_warn << "[isZeroKey] invalid key pointer"; - assert(false && "[isZeroKey] invalid key pointer"); - return false; - } - - if (keySize == FILENODEKEYLENGTH) // 32 (filekey, nodekey, etc) - { - assert(FILENODEKEYLENGTH == SymmCipher::BLOCKSIZE * 2); - // Check if the lower 16 bytes (0-15) are equal to the higher 16 bytes (16-31) - // This will be true either if the key is all zeros or it was generated with a 16-byte zero key (for example, the transferkey was a zerokey). - return std::memcmp(key /*key[0-15]*/, key + SymmCipher::BLOCKSIZE /*key[16-31]*/, SymmCipher::BLOCKSIZE) == 0; - } - else if (keySize == SymmCipher::BLOCKSIZE) // 16 (transfer key, client master key, etc) - { - // Check if all bytes are zero (zerokey) - static const byte zeroKey[SymmCipher::BLOCKSIZE] = {}; - return std::memcmp(key, zeroKey, SymmCipher::BLOCKSIZE) == 0; - } - - // Invalid key size, consider it non-zero - LOG_warn << "[isZeroKey] used a keySize(" << keySize << ") different from 32 and 16 -> function will return false"; - assert(false && "isZeroKey used a keySize different from 32 and 16"); - return false; -} - } // namespace mega From 7ff1fe3647a36902b86c946d1e11a7a20923d775 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 13:53:22 +0000 Subject: [PATCH 05/10] Undo NewNode move --- include/mega/node.h | 46 ++++++++++++++++++++++----------------------- src/node.cpp | 4 ++++ 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/include/mega/node.h b/include/mega/node.h index 4430905ace..47129090b2 100644 --- a/include/mega/node.h +++ b/include/mega/node.h @@ -77,6 +77,29 @@ struct CloudNode bool isIgnoreFile() const; }; +// new node for putnodes() +struct MEGA_API NewNode : public NodeCore +{ + string nodekey; + + newnodesource_t source = NEW_NODE; + + NodeHandle ovhandle; + UploadHandle uploadhandle; + UploadToken uploadtoken; + + std::unique_ptr fileattributes; + + // versioning used for this new node, forced at server's side regardless the account's value + VersioningOption mVersioningOption = NoVersioning; + bool added = false; // set true when the actionpacket arrives + bool canChangeVault = false; + handle mAddedHandle = UNDEF; // updated as actionpacket arrives + error mError = API_OK; // per-node error (updated in cs response) + + bool hasZeroKey() const; +}; + struct MEGA_API PublicLink { handle ph; @@ -474,29 +497,6 @@ inline bool Node::hasZeroKey(const string& nodekeydata) // END MEGA_API Node -// new node for putnodes() -struct MEGA_API NewNode : public NodeCore -{ - string nodekey; - - newnodesource_t source = NEW_NODE; - - NodeHandle ovhandle; - UploadHandle uploadhandle; - UploadToken uploadtoken; - - std::unique_ptr fileattributes; - - // versioning used for this new node, forced at server's side regardless the account's value - VersioningOption mVersioningOption = NoVersioning; - bool added = false; // set true when the actionpacket arrives - bool canChangeVault = false; - handle mAddedHandle = UNDEF; // updated as actionpacket arrives - error mError = API_OK; // per-node error (updated in cs response) - - bool hasZeroKey() const { return Node::hasZeroKey(nodekey); } -}; - class NodeData { public: diff --git a/src/node.cpp b/src/node.cpp index b319534e33..dc69c31f4a 100644 --- a/src/node.cpp +++ b/src/node.cpp @@ -1860,6 +1860,10 @@ std::unique_ptr NodeData::createNode(MegaClient& client, bool fromOldCache return n; } +bool NewNode::hasZeroKey() const +{ + return Node::hasZeroKey(nodekey); +} PublicLink::PublicLink(handle ph, m_time_t cts, m_time_t ets, bool takendown, const char *authKey) { From 68fef1adde1631eb9ee0cb33193317cbebcb0a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 14:04:01 +0000 Subject: [PATCH 06/10] Simplify new node error checks on CommandPutNodes::procresult --- src/commands.cpp | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index 0b72086c8d..c944476a0d 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -1281,23 +1281,8 @@ bool CommandPutNodes::procresult(Result r, JSON& json) if (nn[arrayIndex].mError != API_OK) { newNodeError = nn[arrayIndex].mError; - LOG_debug << "[CommandPutNodes] New Node failed with " << newNodeError << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; - if (nn[arrayIndex].mError == API_EKEY) - { - // Check if the error was due to a zerokey. - // We check this here to avoid a lot of unnecessary checks within CommandPutNodes constructor, where we send the key. - // For the constructor we just add asserts for the debug mode. Here we can add checks and logs. - if (nn[arrayIndex].hasZeroKey()) - { - LOG_err << "[CommandPutNodes] New Node failed with API_EKEY has a zerokey!!!!" << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; - assert(false && "New Node which failed with API_EKEY has a zerokey!!!!"); - // sendevent? The API already sends an event for this scenario - } - else - { - LOG_warn << "[CommandPutNodes] New Node failed with API_EKEY !!" << " [newnode index = " << arrayIndex << ", handle = " << nn[arrayIndex].nodehandle << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; - } - } + LOG_debug << "[CommandPutNodes] New Node failed with " << newNodeError << " [newnode index = " << arrayIndex << ", NodeHandle = " << nn[arrayIndex].nodeHandle() << "]"; + assert(((nn[arrayIndex].mError != API_EKEY) || !nn[arrayIndex].hasZeroKey()) && "New Node which failed with API_EKEY has a zerokey!!!!"); } arrayIndex++; } From 9b9dc28739b9057692fb9492abed2889db5e896c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 14:09:16 +0000 Subject: [PATCH 07/10] Send event for a cloned node with a zero key --- src/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.cpp b/src/node.cpp index dc69c31f4a..c2685b1a95 100644 --- a/src/node.cpp +++ b/src/node.cpp @@ -3114,7 +3114,7 @@ void LocalNode::queueClientUpload(shared_ptr upload, Versio if (cloneNode->hasZeroKey()) { LOG_warn << "Clone node key is a zero key!!!!!!!!! -> avoiding cloning node to regenerate the key [cloneNode path = '" << cloneNode->displaypath() << "', sourceLocalname = '" << upload->sourceLocalname << "']"; - assert(false && "Clone node key is a zero key!!!!!!!!!"); + mc.sendevent(99486, "Node has a zerokey"); cloneNode = nullptr; } break; From 8f2eaa46f4549ab358cd73d969b1d4f207acd3dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 15:15:12 +0000 Subject: [PATCH 08/10] Add unit tests for SymmCipher::isZeroKey Test SymmCipher::isZeroKey Test whether a key is a zerokey or generated with a zerokey Use different data structures (byte[], byte*, std::vector, std::string) 1) Test a 16-byte key all zeros - isZeroKey should be true 2) Test a 16-byte key all ones - isZeroKey should be false 3) Test a 32-byte key all zeros - isZeroKey should be true 4) Test a 32-byte key all ones - isZeroKey should be true 5) Test a 32-byte key half zeros, half ones - isZeroKey should be false 6) Test a 32-byte key: "0123456789ABCDEF0123456789ABCDEF" - isZeroKey should be true --- tests/unit/Crypto_test.cpp | 44 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/unit/Crypto_test.cpp b/tests/unit/Crypto_test.cpp index f48fa5b29a..53e704722c 100644 --- a/tests/unit/Crypto_test.cpp +++ b/tests/unit/Crypto_test.cpp @@ -349,3 +349,47 @@ TEST(Crypto, SymmCipher_xorblock_block_unaligned) ASSERT_EQ(memcmp(dest, result, sizeof(dest)), 0); } + +// Test SymmCipher::isZeroKey +// +// Test whether a key is a zerokey or generated with a zerokey +// Use different data structures (byte[], byte*, std::vector, std::string) +// +// 1) Test a 16-byte key all zeros - isZeroKey should be true +// 2) Test a 16-byte key all ones - isZeroKey should be false +// 3) Test a 32-byte key all zeros - isZeroKey should be true +// 4) Test a 32-byte key all ones - isZeroKey should be true +// 5) Test a 32-byte key half zeros, half ones - isZeroKey should be false +// 6) Test a 32-byte key: "0123456789ABCDEF0123456789ABCDEF" - isZeroKey should be true +TEST(Crypto, SymmCipher_isZeroKey) +{ + // 1) Test a 16-byte key all zeros - isZeroKey should be true + byte key_test1[SymmCipher::BLOCKSIZE] = {}; + ASSERT_EQ(SymmCipher::isZeroKey(key_test1, SymmCipher::BLOCKSIZE), true); + + // 2) Test a 16-byte key all ones - isZeroKey should be false + auto key_test2 = std::make_unique(SymmCipher::BLOCKSIZE); + std::memset(key_test2.get(), 1, SymmCipher::BLOCKSIZE); + ASSERT_EQ(SymmCipher::isZeroKey(key_test2.get(), SymmCipher::BLOCKSIZE), false); + + // 3) Test a 32-byte key all zeros - isZeroKey should be true + std::vector key_test3(FILENODEKEYLENGTH, 0); + ASSERT_EQ(SymmCipher::isZeroKey(key_test3.data(), FILENODEKEYLENGTH), true); + + // 4) Test a 32-byte key all ones - isZeroKey should be true + std::string key_test4(FILENODEKEYLENGTH, 1); + ASSERT_EQ(SymmCipher::isZeroKey(reinterpret_cast(key_test4.data()), FILENODEKEYLENGTH), true); + + // 5) Test a 32-byte key half zeros, half ones - isZeroKey should be false + byte key_test5[FILENODEKEYLENGTH]; + std::memset(key_test5, 0, SymmCipher::BLOCKSIZE); + std::memset(key_test5 + SymmCipher::BLOCKSIZE, 1, SymmCipher::BLOCKSIZE); + ASSERT_EQ(SymmCipher::isZeroKey(key_test5, FILENODEKEYLENGTH), false); + + // 6) Test a 32-byte key: "0123456789ABCDEF0123456789ABCDEF" - isZeroKey should be true + std::string key_test6; + key_test6.resize(FILENODEKEYLENGTH); + key_test6.replace(0, SymmCipher::BLOCKSIZE, "0123456789ABCDEF"); + key_test6.replace(SymmCipher::BLOCKSIZE, SymmCipher::BLOCKSIZE, "0123456789ABCDEF"); + ASSERT_EQ(SymmCipher::isZeroKey(reinterpret_cast(key_test6.data()), FILENODEKEYLENGTH), true); +} From dde8c1623b796e47db0771a9b0c5441696e13e15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Tue, 26 Mar 2024 18:06:39 +0000 Subject: [PATCH 09/10] Force upload if the existing node has a zerokey For a regular upload, a previous node or a node with the same name is compared to see if the actual upload could be avoided and we could just clone the node. That also need a check, and force the upload if the base node has a zerokey. In that case, MegaApiImpl::hasToForceUpload should return true. Also, an event should be send, similar to what we already do within LocalNode::queueClientUpload for syncs. --- src/megaapi_impl.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/megaapi_impl.cpp b/src/megaapi_impl.cpp index 26841d8f63..5e03bfd606 100644 --- a/src/megaapi_impl.cpp +++ b/src/megaapi_impl.cpp @@ -8674,7 +8674,18 @@ bool MegaApiImpl::hasToForceUpload(const Node &node, const MegaTransferPrivate & bool canForceUpload = transfer.isForceNewUpload(); bool isPdf = name.find(".pdf") != string::npos; - return canForceUpload && (isMedia || isPdf) && !(hasPreview && hasThumbnail); + if (canForceUpload && (isMedia || isPdf) && !(hasPreview && hasThumbnail)) + { + return true; + } + if (node.hasZeroKey()) + { + // If the node has a zerokey, we need to discard it, regardless other conditions. + LOG_warn << "[MegaApiImpl::hasToForceUpload] Node has a zerokey, forcing a new upload..." << " [handle = " << node.nodeHandle() << "]"; + client->sendevent(99486, "Node has a zerokey"); + return true; + } + return false; } void MegaApiImpl::moveTransferUp(int transferTag, MegaRequestListener *listener) From f03d34d5194ec5527dfb1f7f3c1fbf972443d639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Manuel=20Garc=C3=ADa=20Amicis?= Date: Mon, 1 Apr 2024 13:45:32 +0100 Subject: [PATCH 10/10] Adjust log message --- src/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.cpp b/src/node.cpp index c2685b1a95..75654c6429 100644 --- a/src/node.cpp +++ b/src/node.cpp @@ -3113,7 +3113,7 @@ void LocalNode::queueClientUpload(shared_ptr upload, Versio cloneNode = n.get(); if (cloneNode->hasZeroKey()) { - LOG_warn << "Clone node key is a zero key!!!!!!!!! -> avoiding cloning node to regenerate the key [cloneNode path = '" << cloneNode->displaypath() << "', sourceLocalname = '" << upload->sourceLocalname << "']"; + LOG_warn << "Clone node key is a zero key!! Avoid cloning node to generate a new key [cloneNode path = '" << cloneNode->displaypath() << "', sourceLocalname = '" << upload->sourceLocalname << "']"; mc.sendevent(99486, "Node has a zerokey"); cloneNode = nullptr; }