Fix escaping in Linux app storage (#20289)

* Fix escaping in Linux app storage

- Linux apps (like all-clusters-app) use a different storage backend
  than chip-tool, but the backend eventually goes to the same INI
  format.
- The Linux app did not properly manage keys with `=` and `\n` in the
  key, when attempting reload of value.

Fixes #20188

This PR:

- Adds the same escaping to Linux apps as for chip-tool
- Adds unescaping
- Adds unit tests for escaping and unescaping
- Adds debug dumping of all keys to chip-tool storage init

Testing done:

- Added unit tests pass
- Existing unit tests pass
- Cert tests pass
- Manual validation of restart with colliding keys in INI backend
  no longer shows collision after change

* Fix lint

* Fix CI

* Address review comment

Author: tcarmelveilleux

examples/chip-tool/config/PersistentStorage.cpp

@@ -18,8 +18,7 @@
 #include "PersistentStorage.h"
 
 #include <lib/core/CHIPEncoding.h>
-#include <lib/support/Base64.h>
-#include <protocols/secure_channel/PASESession.h>
+#include <lib/support/IniEscaping.h>
 
 #include <fstream>
 #include <memory>
@@ -30,6 +29,7 @@ using Sections = std::map<String, Section>;
 
 using namespace ::chip;
 using namespace ::chip::Controller;
+using namespace ::chip::IniEscaping;
 using namespace ::chip::Logging;
 
 constexpr const char kDefaultSectionName[]  = "Default";
@@ -48,60 +48,6 @@ std::string GetFilename(const char * name)
     return "/tmp/chip_tool_config." + std::string(name) + ".ini";
 }
 
-namespace {
-
-std::string EscapeKey(const std::string & key)
-{
-    std::string escapedKey;
-    escapedKey.reserve(key.size());
-
-    for (char c : key)
-    {
-        // Replace spaces, non-printable chars, `=` and the escape itself with hex-escaped (C-style) characters.
-        if ((c <= 0x20) || (c == '=') || (c == '\\') || (c >= 0x7F))
-        {
-            char escaped[5] = { 0 };
-            snprintf(escaped, sizeof(escaped), "\\x%02x", (static_cast<unsigned>(c) & 0xff));
-            escapedKey += escaped;
-        }
-        else
-        {
-            escapedKey += c;
-        }
-    }
-
-    return escapedKey;
-}
-
-std::string StringToBase64(const std::string & value)
-{
-    std::unique_ptr<char[]> buffer(new char[BASE64_ENCODED_LEN(value.length())]);
-
-    uint32_t len =
-        chip::Base64Encode32(reinterpret_cast<const uint8_t *>(value.data()), static_cast<uint32_t>(value.length()), buffer.get());
-    if (len == UINT32_MAX)
-    {
-        return "";
-    }
-
-    return std::string(buffer.get(), len);
-}
-
-std::string Base64ToString(const std::string & b64Value)
-{
-    std::unique_ptr<uint8_t[]> buffer(new uint8_t[BASE64_MAX_DECODED_LEN(b64Value.length())]);
-
-    uint32_t len = chip::Base64Decode32(b64Value.data(), static_cast<uint32_t>(b64Value.length()), buffer.get());
-    if (len == UINT32_MAX)
-    {
-        return "";
-    }
-
-    return std::string(reinterpret_cast<const char *>(buffer.get()), len);
-}
-
-} // namespace
-
 CHIP_ERROR PersistentStorage::Init(const char * name)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -118,6 +64,12 @@ CHIP_ERROR PersistentStorage::Init(const char * name)
     mName = name;
     mConfig.parse(ifs);
     ifs.close();
+
+    // To audit the contents at init, uncomment the following:
+#if 0
+    DumpKeys();
+#endif
+
 exit:
     return err;
 }
@@ -189,6 +141,24 @@ bool PersistentStorage::SyncDoesKeyExist(const char * key)
     return (it != section.end());
 }
 
+void PersistentStorage::DumpKeys() const
+{
+#if CHIP_PROGRESS_LOGGING
+    for (const auto & section : mConfig.sections)
+    {
+        const std::string & sectionName = section.first;
+        const auto & sectionContent     = section.second;
+
+        ChipLogProgress(chipTool, "[%s]", sectionName.c_str());
+        for (const auto & entry : sectionContent)
+        {
+            const std::string & keyName = entry.first;
+            ChipLogProgress(chipTool, "  => %s", UnescapeKey(keyName).c_str());
+        }
+    }
+#endif // CHIP_PROGRESS_LOGGING
+}
+
 CHIP_ERROR PersistentStorage::SyncClearAll()
 {
     ChipLogProgress(chipTool, "Clearing %s storage", kDefaultSectionName);

examples/chip-tool/config/PersistentStorage.h

@@ -34,6 +34,8 @@ class PersistentStorage : public chip::PersistentStorageDelegate
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override;
     bool SyncDoesKeyExist(const char * key) override;
 
+    void DumpKeys() const;
+
     uint16_t GetListenPort();
     chip::Logging::LogCategory GetLoggingLevel();
 

scripts/tools/check_includes_config.py

@@ -106,6 +106,8 @@
     # Not intended for embedded clients (#11705).
     'src/app/ClusterStateCache.h': {'list', 'map', 'set', 'vector', 'queue'},
     'src/app/BufferedReadCallback.h': {'vector'},
+    'src/lib/support/IniEscaping.cpp': {'string'},
+    'src/lib/support/IniEscaping.h': {'string'},
 
     # Itself in DENY.
     'src/lib/support/CHIPListUtils.h': {'set'},

src/lib/support/BUILD.gn

@@ -107,6 +107,8 @@ static_library("support") {
     "FibonacciUtils.h",
     "FixedBufferAllocator.cpp",
     "FixedBufferAllocator.h",
+    "IniEscaping.cpp",
+    "IniEscaping.h",
     "Iterators.h",
     "LifetimePersistedCounter.h",
     "ObjectLifeCycle.h",

src/lib/support/IniEscaping.cpp

@@ -0,0 +1,148 @@
+/*
+ *
+ *    Copyright (c) 2022 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <memory>
+#include <string>
+
+#include "IniEscaping.h"
+#include <lib/support/Base64.h>
+#include <lib/support/BytesToHex.h>
+
+namespace chip {
+namespace IniEscaping {
+
+namespace {
+
+constexpr size_t kEscapeChunkSize = 4; // "\x12" --> 4 chars
+
+constexpr bool NeedsEscape(char c)
+{
+    return (c <= 0x20) || (c == '=') || (c == '\\') || (c >= 0x7F);
+}
+
+constexpr bool IsLowercaseHex(char c)
+{
+    return ((c >= '0') && (c <= '9')) || ((c >= 'a') && (c <= 'f'));
+}
+
+bool IsValidEscape(const std::string & s)
+{
+    return (s.size() >= kEscapeChunkSize) && (s[0] == '\\') && (s[1] == 'x') && IsLowercaseHex(s[2]) && IsLowercaseHex(s[3]);
+}
+
+} // namespace
+
+std::string EscapeKey(const std::string & key)
+{
+    std::string escapedKey;
+    escapedKey.reserve(key.size());
+
+    for (char c : key)
+    {
+        // Replace spaces, non-printable chars, `=` and the escape itself with hex-escaped (C-style) characters.
+        if (NeedsEscape(c))
+        {
+            char escaped[kEscapeChunkSize + 1] = { 0 };
+            snprintf(escaped, sizeof(escaped), "\\x%02x", (static_cast<unsigned>(c) & 0xff));
+            escapedKey += escaped;
+        }
+        else
+        {
+            escapedKey += c;
+        }
+    }
+
+    return escapedKey;
+}
+
+std::string UnescapeKey(const std::string & key)
+{
+    std::string unescaped;
+    unescaped.reserve(key.size());
+
+    size_t idx       = 0;
+    size_t remaining = key.size();
+    while (remaining > 0)
+    {
+        char c = key[idx];
+        if (c == '\\')
+        {
+            // Don't process invalid escapes.
+            if (remaining < kEscapeChunkSize)
+            {
+                return "";
+            }
+
+            auto escapeChunk = key.substr(idx, kEscapeChunkSize);
+            if (!IsValidEscape(escapeChunk))
+            {
+                return "";
+            }
+
+            // We validated format, now extract the last two chars as hex
+            auto hexDigits   = escapeChunk.substr(2, 2);
+            uint8_t charByte = 0;
+            if ((chip::Encoding::HexToBytes(hexDigits.data(), 2, &charByte, 1) != 1) || !NeedsEscape(static_cast<char>(charByte)))
+            {
+                return "";
+            }
+
+            unescaped += static_cast<char>(charByte);
+            idx += kEscapeChunkSize;
+        }
+        else
+        {
+            unescaped += c;
+            idx += 1;
+        }
+
+        remaining = key.size() - idx;
+    }
+
+    return unescaped;
+}
+
+std::string StringToBase64(const std::string & value)
+{
+    std::unique_ptr<char[]> buffer(new char[BASE64_ENCODED_LEN(value.length())]);
+
+    uint32_t len =
+        chip::Base64Encode32(reinterpret_cast<const uint8_t *>(value.data()), static_cast<uint32_t>(value.length()), buffer.get());
+    if (len == UINT32_MAX)
+    {
+        return "";
+    }
+
+    return std::string(buffer.get(), len);
+}
+
+std::string Base64ToString(const std::string & b64Value)
+{
+    std::unique_ptr<uint8_t[]> buffer(new uint8_t[BASE64_MAX_DECODED_LEN(b64Value.length())]);
+
+    uint32_t len = chip::Base64Decode32(b64Value.data(), static_cast<uint32_t>(b64Value.length()), buffer.get());
+    if (len == UINT32_MAX)
+    {
+        return "";
+    }
+
+    return std::string(reinterpret_cast<const char *>(buffer.get()), len);
+}
+
+} // namespace IniEscaping
+} // namespace chip

src/lib/support/IniEscaping.h

@@ -0,0 +1,69 @@
+/*
+ *
+ *    Copyright (c) 2022 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <string>
+
+namespace chip {
+namespace IniEscaping {
+
+/**
+ * @brief Escape a storage key to be INI-safe.
+ *
+ * All characters <= 0x20, >= 0x7F and `\` and `=` are
+ * escaped as `\xYY` where `YY` is a 2-digit lowercase hex value.
+ *
+ * @param key - key to escape
+ * @return the escaped key
+ */
+std::string EscapeKey(const std::string & key);
+
+/**
+ * @brief Unescape a storage key escaped by `EscapeKey`
+ *
+ * If any character not expected to be escaped is found, or
+ * if any escape sequences are partial, or if uppercase hex is seen
+ * in an escape sequence, the empty string is returned.
+ *
+ * @param key - key to unescape
+ * @return the original key that was provided to EscapeKey or empty string on error.
+ */
+std::string UnescapeKey(const std::string & escapedKey);
+
+/**
+ * @brief Takes an octet string passed into a std::string and converts it to base64
+ *
+ * There may be `\0` characters in the data of std::string.
+ *
+ * @param value - Value to convert to base64.
+ * @return the base64 encoding of the `value` input
+ */
+std::string StringToBase64(const std::string & value);
+
+/**
+ * @brief Takes a base64 buffer and converts it to an octet string buffer
+ *        within a std::string.
+ *
+ * @param b64Value - Buffer of base64 to decode
+ * @return an std::string with the bytes, or empty string on decoding errors
+ */
+std::string Base64ToString(const std::string & b64Value);
+
+} // namespace IniEscaping
+} // namespace chip

src/lib/support/tests/BUILD.gn

@@ -34,6 +34,7 @@ chip_test_suite("tests") {
     "TestErrorStr.cpp",
     "TestFixedBufferAllocator.cpp",
     "TestFold.cpp",
+    "TestIniEscaping.cpp",
     "TestIntrusiveList.cpp",
     "TestOwnerOf.cpp",
     "TestPersistedCounter.cpp",