Merge branch 'fix/SDK-3787_Avoid-cloning-nodes-with-zerokey' into 'de…

…velop'

SDK-3787. Avoid cloning a node with a zerokey

Closes SDK-3787

See merge request sdk/sdk!5410

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

include/mega/node.h

@@ -96,6 +96,8 @@ struct MEGA_API NewNode : public NodeCore
     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
@@ -217,6 +219,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<Node> , bool updateNodeCounters = true);
 
@@ -477,6 +485,19 @@ inline bool Node::keyApplied() const
     return nodekeydata.size() == size_t((type == FILENODE) ? FILENODEKEYLENGTH : FOLDERNODEKEYLENGTH);
 }
 
+inline bool Node::hasZeroKey() const
+{
+    return keyApplied() && SymmCipher::isZeroKey(reinterpret_cast<const byte*>(nodekeydata.data()), nodekeydata.size());
+}
+
+inline bool Node::hasZeroKey(const string& nodekeydata)
+{
+    return ((nodekeydata.size() == FILENODEKEYLENGTH) || (nodekeydata.size() == FOLDERNODEKEYLENGTH)) &&
+            SymmCipher::isZeroKey(reinterpret_cast<const byte*>(nodekeydata.data()), nodekeydata.size());
+}
+
+// END MEGA_API Node
+
 class NodeData
 {
 public:

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(!SymmCipher::isZeroKey(key, FILENODEKEYLENGTH));
                 arg("k", key, int(nn[i].nodekey.size()));
             }
             else
@@ -1255,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 (;;)
         {
@@ -1274,7 +1277,14 @@ 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_OK)
+                    {
+                        newNodeError = nn[arrayIndex].mError;
+                        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++;
                 }
             }
             else
@@ -1322,7 +1332,8 @@ bool CommandPutNodes::procresult(Result r, JSON& json)
         shared_ptr<Node> 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<error>(newNodeError) : API_ENOENT) : // Add last new node error if there is any, otherwise API_ENOENT
+                                            API_OK, nn, targetOverride);
         return true;
     }
     else
@@ -3806,6 +3817,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()));

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

src/megaapi_impl.cpp

@@ -8709,7 +8709,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)

src/node.cpp

@@ -1863,6 +1863,10 @@ std::unique_ptr<Node> 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)
 {
@@ -3110,6 +3114,12 @@ void LocalNode::queueClientUpload(shared_ptr<SyncUpload_inClient> upload, Versio
                 if (mc.treatAsIfFileDataEqual(*n, ext1, *upload, ext2))
                 {
                     cloneNode = n.get();
+                    if (cloneNode->hasZeroKey())
+                    {
+                        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;
+                    }
                     break;
                 }
             }

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<byte> zeroKey(SymmCipher::BLOCKSIZE, 0);
-    if (ckey && memcmp(ckey, zeroKey.data(), zeroKey.size()))
+    if (ckey && !SymmCipher::isZeroKey(ckey, SymmCipher::BLOCKSIZE))
     {
         memcpy(key, ckey, sizeof key);
         have_key = 1;

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<byte>, 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<byte[]>(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<byte> 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<byte*>(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<byte*>(key_test6.data()), FILENODEKEYLENGTH), true);
+}