Skip to content

Commit

Permalink
Fix unsafe use of strncpy in OpenThread dns-sd code. (#13392)
Browse files Browse the repository at this point in the history
For the three uses where we are copying data of some computed size
into a buffer, add checks that the size is not too big for the buffer
(and in particular that we will be able to null-terminate after
copying).

For the one use where we are copying the entire buffer between two
identical-sized buffers:

1) Assert that the target buffer us not smaller than the source buffer.

2) Use CopyString to ensure null-termination even if the source is not
null-terminated.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Mar 12, 2022
1 parent 688d5fb commit 2173106
Showing 1 changed file with 16 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@

#include <app/AttributeAccessInterface.h>
#include <lib/core/CHIPEncoding.h>
#include <lib/support/CHIPMemString.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/FixedBufferAllocator.h>
#include <lib/support/ThreadOperationalDataset.h>
Expand Down Expand Up @@ -2073,6 +2074,10 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::FromOtDnsRespons

// Extract from the <hostname>.<domain-name>. the <hostname> part.
size_t substringSize = strchr(serviceInfo.mHostNameBuffer, '.') - serviceInfo.mHostNameBuffer;
if (substringSize >= ArraySize(mdnsService.mHostName))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
strncpy(mdnsService.mHostName, serviceInfo.mHostNameBuffer, substringSize);
// Append string terminating character.
mdnsService.mHostName[substringSize] = '\0';
Expand All @@ -2082,6 +2087,10 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::FromOtDnsRespons

// Extract from the <type>.<protocol>.<domain-name>. the <type> part.
substringSize = strchr(serviceType, '.') - serviceType;
if (substringSize >= ArraySize(mdnsService.mType))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
strncpy(mdnsService.mType, serviceType, substringSize);
// Append string terminating character.
mdnsService.mType[substringSize] = '\0';
Expand All @@ -2093,6 +2102,10 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::FromOtDnsRespons
return CHIP_ERROR_INVALID_ARGUMENT;

substringSize = strchr(protocolSubstringStart, '.') - protocolSubstringStart;
if (substringSize >= ArraySize(protocol))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
strncpy(protocol, protocolSubstringStart, substringSize);
// Append string terminating character.
protocol[substringSize] = '\0';
Expand Down Expand Up @@ -2211,7 +2224,9 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnDnsBrowseResult(otEr
// Invoke callback for every service one by one instead of for the whole list due to large memory size needed to
// allocate on
// stack.
strncpy(dnsResult->mMdnsService.mName, serviceName, sizeof(serviceName));
static_assert(ArraySize(dnsResult->mMdnsService.mName) >= ArraySize(serviceName),
"The target buffer must be big enough");
Platform::CopyString(dnsResult->mMdnsService.mName, serviceName);
DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowse, reinterpret_cast<intptr_t>(dnsResult));
wasAnythingBrowsed = true;
}
Expand Down

0 comments on commit 2173106

Please sign in to comment.