Make RandUtils.h use DRBG_get_bytes() (#10465)

* Make RandUtils.h use DRBG_get_bytes() - Move RandUtils.* from lib/support to src/crypto to break dependency cycle w/ CHIPCryptoPAL.h - Make RandUtils functions use DRBG_get_bytes(), which avoids issues of `srand()` improperly initialized or dependencies on `rand()` in core SDK code. All random numbers in core protocols are supposed, by spec, to be generated using a crypto-based DRBG - Fix needless inclusions of RandUtils.h - Move all uses of `GetRandU*` to the new location/version - Fix minor usage of snprintf() for large hex strings in DnsSd to use BytesToHex(). Testing done: unit tests still pass. Cert tests still pass. * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]>
project-chip · Oct 14, 2021 · 267951c · 267951c
1 parent f0cc672
commit 267951c
Show file tree

Hide file tree

Showing 24 changed files with 105 additions and 172 deletions.
diff --git a/examples/bridge-app/linux/main.cpp b/examples/bridge-app/linux/main.cpp
@@ -33,7 +33,6 @@
 #include <credentials/examples/DeviceAttestationCredsExample.h>
 #include <lib/core/CHIPError.h>
 #include <lib/support/CHIPMem.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/ZclString.h>
 #include <setup_payload/QRCodeSetupPayloadGenerator.h>
 #include <setup_payload/SetupPayload.h>

diff --git a/examples/ota-provider-app/linux/main.cpp b/examples/ota-provider-app/linux/main.cpp
@@ -28,7 +28,6 @@
 #include <lib/core/CHIPError.h>
 #include <lib/support/CHIPArgParser.hpp>
 #include <lib/support/CHIPMem.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 
 #include <ota-provider-common/BdxOtaSender.h>

diff --git a/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp b/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp
@@ -23,9 +23,9 @@
 #include <app/CommandPathParams.h>
 #include <app/clusters/ota-provider/ota-provider-delegate.h>
 #include <app/util/af.h>
+#include <crypto/RandUtils.h>
 #include <lib/core/CHIPTLV.h>
 #include <lib/support/CHIPMemString.h>
-#include <lib/support/RandUtils.h>
 #include <protocols/secure_channel/PASESession.h> // For chip::kTestDeviceNodeId
 
 #include <string.h>
@@ -56,7 +56,7 @@ void GenerateUpdateToken(uint8_t * buf, size_t bufSize)
 {
     for (size_t i = 0; i < bufSize; ++i)
     {
-        buf[i] = chip::GetRandU8();
+        buf[i] = chip::Crypto::GetRandU8();
     }
 }
 

diff --git a/examples/ota-requestor-app/linux/main.cpp b/examples/ota-requestor-app/linux/main.cpp
@@ -28,7 +28,6 @@
 #include <lib/support/CHIPArgParser.hpp>
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/Span.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <messaging/ExchangeDelegate.h>

diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp
@@ -33,7 +33,6 @@
 #include <credentials/examples/DeviceAttestationVerifierExample.h>
 
 #include <lib/support/CHIPMem.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/ScopedBuffer.h>
 #include <setup_payload/QRCodeSetupPayloadGenerator.h>
 #include <setup_payload/SetupPayload.h>

diff --git a/examples/shell/shell_common/cmd_misc.cpp b/examples/shell/shell_common/cmd_misc.cpp
@@ -17,11 +17,11 @@
 
 #include <lib/shell/Engine.h>
 
+#include <crypto/RandUtils.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/support/Base64.h>
 #include <lib/support/CHIPArgParser.hpp>
 #include <lib/support/CodeUtils.h>
-#include <lib/support/RandUtils.h>
 
 #include <inttypes.h>
 #include <stdarg.h>
@@ -56,7 +56,7 @@ CHIP_ERROR cmd_log(int argc, char ** argv)
 
 CHIP_ERROR cmd_rand(int argc, char ** argv)
 {
-    streamer_printf(streamer_get(), "%d\n\r", GetRandU8());
+    streamer_printf(streamer_get(), "%d\n\r", static_cast<int>(chip::Crypto::GetRandU8()));
     return CHIP_NO_ERROR;
 }
 

diff --git a/examples/shell/standalone/main.cpp b/examples/shell/standalone/main.cpp
@@ -21,7 +21,6 @@
 #include <lib/support/Base64.h>
 #include <lib/support/CHIPArgParser.hpp>
 #include <lib/support/CodeUtils.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 
 #include <ChipShellCollection.h>

diff --git a/src/crypto/BUILD.gn b/src/crypto/BUILD.gn
@@ -58,6 +58,8 @@ static_library("crypto") {
   sources = [
     "CHIPCryptoPAL.cpp",
     "CHIPCryptoPAL.h",
+    "RandUtils.cpp",
+    "RandUtils.h",
   ]
 
   cflags = [ "-Wconversion" ]

diff --git a/src/crypto/RandUtils.cpp b/src/crypto/RandUtils.cpp
@@ -0,0 +1,69 @@
+/*
+ *
+ *    Copyright (c) 2020 Project CHIP Authors
+ *    Copyright (c) 2013-2017 Nest Labs, Inc.
+ *
+ *    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.
+ */
+
+/**
+ *    @file
+ *      This file implements utility functions for deriving random integers.
+ *
+ *  @note These utility functions do not generate cryptographically strong
+ *        random number. To get cryptographically strong random data use
+ *        chip::Crypto::DRBG_get_bytes().
+ *
+ */
+
+#include "RandUtils.h"
+
+#include <stdint.h>
+#include <stdlib.h>
+
+#include <crypto/CHIPCryptoPAL.h>
+#include <lib/support/CodeUtils.h>
+
+namespace chip {
+namespace Crypto {
+
+uint64_t GetRandU64()
+{
+    uint64_t tmp = 0;
+    VerifyOrDie(CHIP_NO_ERROR == DRBG_get_bytes(reinterpret_cast<uint8_t *>(&tmp), sizeof(tmp)));
+    return tmp;
+}
+
+uint32_t GetRandU32()
+{
+    uint32_t tmp = 0;
+    VerifyOrDie(CHIP_NO_ERROR == DRBG_get_bytes(reinterpret_cast<uint8_t *>(&tmp), sizeof(tmp)));
+    return tmp;
+}
+
+uint16_t GetRandU16()
+{
+    uint16_t tmp = 0;
+    VerifyOrDie(CHIP_NO_ERROR == DRBG_get_bytes(reinterpret_cast<uint8_t *>(&tmp), sizeof(tmp)));
+    return tmp;
+}
+
+uint8_t GetRandU8()
+{
+    uint8_t tmp = 0;
+    VerifyOrDie(CHIP_NO_ERROR == DRBG_get_bytes(&tmp, sizeof(tmp)));
+    return tmp;
+}
+
+} // namespace Crypto
+} // namespace chip
diff --git a/src/lib/support/RandUtils.h → src/crypto/RandUtils.h b/src/lib/support/RandUtils.h → src/crypto/RandUtils.h
@@ -16,21 +16,12 @@
  *    limitations under the License.
  */
 
-/**
- *    @file
- *      This file defines utility functions for deriving random integers.
- *
- *  @note These utility functions do not generate cryptographically strong
- *        random number. To get cryptographically strong random data use
- *        chip::Crypto::DRBG_get_bytes().
- *
- */
-
 #pragma once
 
 #include <stdint.h>
 
 namespace chip {
+namespace Crypto {
 
 /**
  *  This function generates 64-bit unsigned random number.
@@ -64,4 +55,5 @@ extern uint16_t GetRandU16();
  */
 extern uint8_t GetRandU8();
 
+} // namespace Crypto
 } // namespace chip
diff --git a/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp b/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp
@@ -27,6 +27,8 @@
 
 #if CHIP_DEVICE_CONFIG_ENABLE_SOFTWARE_UPDATE_MANAGER
 
+#include <crypto/RandUtils.h>
+
 #include <lib/core/CHIPCore.h>
 #include <platform/ConnectivityManager.h>
 #include <platform/PlatformManager.h>
@@ -35,7 +37,6 @@
 #include <platform/internal/GenericSoftwareUpdateManagerImpl.h>
 
 #include <lib/support/FibonacciUtils.h>
-#include <lib/support/RandUtils.h>
 
 #include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
@@ -724,7 +725,7 @@ uint32_t GenericSoftwareUpdateManagerImpl<ImplClass>::GetNextWaitTimeInterval()
 template <class ImplClass>
 uint32_t GenericSoftwareUpdateManagerImpl<ImplClass>::ComputeNextScheduledWaitTimeInterval(void)
 {
-    uint32_t timeOutMsecs = (mMinWaitTimeMs + (GetRandU32() % (mMaxWaitTimeMs - mMinWaitTimeMs)));
+    uint32_t timeOutMsecs = (mMinWaitTimeMs + (chip::Crypto::GetRandU32() % (mMaxWaitTimeMs - mMinWaitTimeMs)));
 
     ChipLogProgress(DeviceLayer, "Next Scheduled Software Update Check in %ums", timeOutMsecs);
 
@@ -1044,7 +1045,7 @@ void GenericSoftwareUpdateManagerImpl<ImplClass>::DefaultRetryPolicyCallback(voi
     if (maxWaitTimeInMsec != 0)
     {
         minWaitTimeInMsec = (CHIP_DEVICE_CONFIG_SWU_MIN_WAIT_TIME_INTERVAL_PERCENT_PER_STEP * maxWaitTimeInMsec) / 100;
-        waitTimeInMsec    = minWaitTimeInMsec + (GetRandU32() % (maxWaitTimeInMsec - minWaitTimeInMsec));
+        waitTimeInMsec    = minWaitTimeInMsec + (chip::Crypto::GetRandU32() % (maxWaitTimeInMsec - minWaitTimeInMsec));
 
         ChipLogDetail(DeviceLayer,
                       "Computing swu retry policy: attempts %" PRIu32 ", max wait time %" PRIu32 " ms, selected wait time %" PRIu32

diff --git a/src/lib/core/tests/TestCHIPTLV.cpp b/src/lib/core/tests/TestCHIPTLV.cpp
@@ -35,13 +35,13 @@
 
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/ScopedBuffer.h>
 #include <lib/support/UnitTestRegistration.h>
 #include <lib/support/UnitTestUtils.h>
 
 #include <system/TLVPacketBufferBackingStore.h>
 
+#include <stdlib.h>
 #include <string.h>
 
 using namespace chip;
@@ -4081,7 +4081,7 @@ static void TLVReaderFuzzTest(nlTestSuite * inSuite, void * inContext)
             {
                 uint8_t fuzzMask = sFixedFuzzMask;
                 while (fuzzMask == 0)
-                    fuzzMask = GetRandU8();
+                    fuzzMask = static_cast<uint8_t>(rand() & 0xFF);
 
                 fuzzedData[i] ^= fuzzMask;
             }

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -23,6 +23,7 @@
 #include "MinimalMdnsServer.h"
 #include "ServiceNaming.h"
 
+#include <crypto/RandUtils.h>
 #include <lib/dnssd/Advertiser_ImplMinimalMdnsAllocator.h>
 #include <lib/dnssd/minimal_mdns/ResponseSender.h>
 #include <lib/dnssd/minimal_mdns/Server.h>
@@ -32,8 +33,8 @@
 #include <lib/dnssd/minimal_mdns/responders/QueryResponder.h>
 #include <lib/dnssd/minimal_mdns/responders/Srv.h>
 #include <lib/dnssd/minimal_mdns/responders/Txt.h>
+#include <lib/support/BytesToHex.h>
 #include <lib/support/CHIPMem.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/StringBuilder.h>
 
 // Enable detailed mDNS logging for received queries
@@ -231,8 +232,7 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
     QueryResponderAllocator<kMaxOperationalRecords> * FindEmptyOperationalAllocator();
 
     ResponseSender mResponseSender;
-    uint32_t mCommissionInstanceName1;
-    uint32_t mCommissionInstanceName2;
+    uint8_t mCommissionableInstanceName[sizeof(uint64_t)];
 
     // current request handling
     const chip::Inet::IPPacketInfo * mCurrentSource = nullptr;
@@ -280,8 +280,9 @@ CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::InetLayer * inetLayer)
 {
     GlobalMinimalMdnsServer::Server().Shutdown();
 
-    mCommissionInstanceName1 = GetRandU32();
-    mCommissionInstanceName2 = GetRandU32();
+    uint64_t random_instance_name = chip::Crypto::GetRandU64();
+    memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName));
+
     // Re-set the server in the response sender in case this has been swapped in the
     // GlobalMinimalMdnsServer (used for testing).
     mResponseSender.SetServer(&GlobalMinimalMdnsServer::Server());
@@ -442,12 +443,9 @@ CHIP_ERROR AdvertiserMinMdns::GetCommissionableInstanceName(char * instanceName,
     {
         return CHIP_ERROR_NO_MEMORY;
     }
-    size_t len = snprintf(instanceName, maxLength, ChipLogFormatX64, mCommissionInstanceName1, mCommissionInstanceName2);
-    if (len >= maxLength)
-    {
-        return CHIP_ERROR_NO_MEMORY;
-    }
-    return CHIP_NO_ERROR;
+
+    return chip::Encoding::BytesToUppercaseHexString(&mCommissionableInstanceName[0], sizeof(mCommissionableInstanceName),
+                                                     instanceName, maxLength);
 }
 
 CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters & params)

diff --git a/src/lib/dnssd/BUILD.gn b/src/lib/dnssd/BUILD.gn
@@ -23,6 +23,7 @@ source_set("platform_header") {
 static_library("dnssd") {
   public_deps = [
     ":platform_header",
+    "${chip_root}/src/crypto",
     "${chip_root}/src/lib/core",
     "${chip_root}/src/lib/support",
   ]

diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp
@@ -19,6 +19,7 @@
 
 #include <inttypes.h>
 
+#include <crypto/RandUtils.h>
 #include <lib/core/CHIPConfig.h>
 #include <lib/core/CHIPSafeCasts.h>
 #include <lib/dnssd/DnssdCache.h>
@@ -28,7 +29,6 @@
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/ErrorStr.h>
-#include <lib/support/RandUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/CHIPDeviceConfig.h>
 #include <platform/CHIPDeviceLayer.h>
@@ -47,7 +47,9 @@ CHIP_ERROR DiscoveryImplPlatform::InitImpl()
 {
     ReturnErrorCodeIf(mDnssdInitialized, CHIP_NO_ERROR);
     ReturnErrorOnFailure(ChipDnssdInit(HandleDnssdInit, HandleDnssdError, this));
-    mCommissionInstanceName = GetRandU64();
+
+    uint64_t random_instance_name = chip::Crypto::GetRandU64();
+    memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName));
 
     return CHIP_NO_ERROR;
 }
@@ -116,13 +118,9 @@ CHIP_ERROR DiscoveryImplPlatform::GetCommissionableInstanceName(char * instanceN
     {
         return CHIP_ERROR_NO_MEMORY;
     }
-    size_t len = snprintf(instanceName, maxLength, "%08" PRIX32 "%08" PRIX32, static_cast<uint32_t>(mCommissionInstanceName >> 32),
-                          static_cast<uint32_t>(mCommissionInstanceName));
-    if (len >= maxLength)
-    {
-        return CHIP_ERROR_NO_MEMORY;
-    }
-    return CHIP_NO_ERROR;
+
+    return chip::Encoding::BytesToUppercaseHexString(&mCommissionableInstanceName[0], sizeof(mCommissionableInstanceName),
+                                                     instanceName, maxLength);
 }
 
 template <class Derived, size_t N_idle, size_t N_active, size_t N_tcp>

diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h
@@ -81,7 +81,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
     bool mIsOperationalPublishing        = false;
     bool mIsCommissionableNodePublishing = false;
     bool mIsCommissionerPublishing       = false;
-    uint64_t mCommissionInstanceName;
+    uint8_t mCommissionableInstanceName[sizeof(uint64_t)];
 
     bool mDnssdInitialized               = false;
     ResolverDelegate * mResolverDelegate = nullptr;

diff --git a/src/lib/shell/commands/Base64.cpp b/src/lib/shell/commands/Base64.cpp
@@ -28,7 +28,6 @@
 #include <lib/support/Base64.h>
 #include <lib/support/CHIPArgParser.hpp>
 #include <lib/support/CodeUtils.h>
-#include <lib/support/RandUtils.h>
 
 chip::Shell::Engine sShellBase64Commands;
 

diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn
@@ -88,8 +88,6 @@ static_library("support") {
     "Pool.h",
     "PrivateHeap.cpp",
     "PrivateHeap.h",
-    "RandUtils.cpp",
-    "RandUtils.h",
     "ReferenceCountedHandle.h",
     "SafeInt.h",
     "SerializableIntegerSet.cpp",