Skip to content

Commit

Permalink
Fix handling of short discriminator in Linux BLE scan.
Browse files Browse the repository at this point in the history
A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes #21160
  • Loading branch information
bzbarsky-apple committed Jul 25, 2022
1 parent b647dfb commit 39ebc91
Show file tree
Hide file tree
Showing 36 changed files with 301 additions and 139 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ void SetupPayloadGenerateCommand::ConfigurePayload(SetupPayload & payload)
{
if (mDiscriminator.HasValue())
{
payload.discriminator = mDiscriminator.Value();
payload.discriminator.SetLongValue(mDiscriminator.Value());
}

if (mSetUpPINCode.HasValue())
Expand Down
29 changes: 19 additions & 10 deletions examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ CHIP_ERROR SetupPayloadParseCommand::Parse(std::string codeString, chip::SetupPa

CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)
{
ChipLogProgress(SetupPayload, "Version: %u", payload.version);
ChipLogProgress(SetupPayload, "VendorID: %u", payload.vendorID);
ChipLogProgress(SetupPayload, "ProductID: %u", payload.productID);
ChipLogProgress(SetupPayload, "Custom flow: %u (%s)", to_underlying(payload.commissioningFlow),
ChipLogProgress(SetupPayload, "Version: %u", payload.version);
ChipLogProgress(SetupPayload, "VendorID: %u", payload.vendorID);
ChipLogProgress(SetupPayload, "ProductID: %u", payload.productID);
ChipLogProgress(SetupPayload, "Custom flow: %u (%s)", to_underlying(payload.commissioningFlow),
CustomFlowString(payload.commissioningFlow));
{
StringBuilder<128> humanFlags;
Expand Down Expand Up @@ -106,15 +106,24 @@ CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)
humanFlags.Add("NONE");
}

ChipLogProgress(SetupPayload, "Capabilities: 0x%02X (%s)", payload.rendezvousInformation.Raw(), humanFlags.c_str());
ChipLogProgress(SetupPayload, "Capabilities: 0x%02X (%s)", payload.rendezvousInformation.Raw(), humanFlags.c_str());
}
ChipLogProgress(SetupPayload, "Discriminator: %u", payload.discriminator);
ChipLogProgress(SetupPayload, "Passcode: %u", payload.setUpPINCode);
if (payload.discriminator.IsShortDiscriminator())
{
ChipLogProgress(SetupPayload, "Short discriminator: %u (0x%x)", payload.discriminator.GetShortValue(),
payload.discriminator.GetShortValue());
}
else
{
ChipLogProgress(SetupPayload, "Long discriminator: %u (0x%x)", payload.discriminator.GetLongValue(),
payload.discriminator.GetLongValue());
}
ChipLogProgress(SetupPayload, "Passcode: %u", payload.setUpPINCode);

std::string serialNumber;
if (payload.getSerialNumber(serialNumber) == CHIP_NO_ERROR)
{
ChipLogProgress(SetupPayload, "SerialNumber: %s", serialNumber.c_str());
ChipLogProgress(SetupPayload, "SerialNumber: %s", serialNumber.c_str());
}

std::vector<OptionalQRCodeInfo> optionalVendorData = payload.getAllOptionalVendorData();
Expand All @@ -126,11 +135,11 @@ CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)

if (isTypeString)
{
ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,string value=%s", info.tag, info.data.c_str());
ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,string value=%s", info.tag, info.data.c_str());
}
else
{
ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,int value=%u", info.tag, info.int32);
ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,int value=%u", info.tag, info.int32);
}
}

Expand Down
6 changes: 3 additions & 3 deletions examples/platform/linux/CommissionableInit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov

if (options.discriminator.HasValue())
{
options.payload.discriminator = options.discriminator.Value();
options.payload.discriminator.SetLongValue(options.discriminator.Value());
}
else
{
Expand All @@ -72,7 +72,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
"given on command line. This is temporary and will disappear. Please update your scripts "
"to explicitly configure discriminator. ***",
static_cast<unsigned>(defaultTestDiscriminator));
options.payload.discriminator = defaultTestDiscriminator;
options.payload.discriminator.SetLongValue(defaultTestDiscriminator);
}

// Default to minimum PBKDF iterations
Expand All @@ -84,7 +84,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
ChipLogError(Support, "PASE PBKDF iterations set to %u", static_cast<unsigned>(spake2pIterationCount));

return provider.Init(options.spake2pVerifier, options.spake2pSalt, spake2pIterationCount, setupPasscode,
options.payload.discriminator);
options.payload.discriminator.GetLongValue());
}

CHIP_ERROR InitConfigurationManager(ConfigurationManagerImpl & configManager, LinuxDeviceOptions & options)
Expand Down
2 changes: 1 addition & 1 deletion examples/tv-casting-app/linux/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
ChipLogError(Support, "PASE PBKDF iterations set to %u", static_cast<unsigned>(spake2pIterationCount));

return provider.Init(options.spake2pVerifier, options.spake2pSalt, spake2pIterationCount, setupPasscode,
options.payload.discriminator);
options.payload.discriminator.GetLongValue());
}

// To hold SPAKE2+ verifier, discriminator, passcode
Expand Down
4 changes: 3 additions & 1 deletion src/app/server/OnboardingCodesUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,15 @@ CHIP_ERROR GetPayloadContents(chip::PayloadContents & aPayload, chip::Rendezvous
#endif
}

err = GetCommissionableDataProvider()->GetSetupDiscriminator(aPayload.discriminator);
uint16_t discriminator = 0;
err = GetCommissionableDataProvider()->GetSetupDiscriminator(discriminator);
if (err != CHIP_NO_ERROR)
{
ChipLogError(AppServer, "GetCommissionableDataProvider()->GetSetupDiscriminator() failed: %" CHIP_ERROR_FORMAT,
err.Format());
return err;
}
aPayload.discriminator.SetLongValue(discriminator);

err = chip::DeviceLayer::GetDeviceInstanceInfoProvider()->GetVendorId(aPayload.vendorID);
if (err != CHIP_NO_ERROR)
Expand Down
5 changes: 3 additions & 2 deletions src/ble/BleConnectionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <ble/BleError.h>

#include <lib/support/DLLUtil.h>
#include <lib/support/SetupDiscriminator.h>

namespace chip {
namespace Ble {
Expand All @@ -51,8 +52,8 @@ class DLL_EXPORT BleConnectionDelegate
OnConnectionErrorFunct OnConnectionError;

// Call this function to delegate the connection steps required to get a BLE_CONNECTION_OBJECT
// out of a peripheral discriminator.
virtual void NewConnection(BleLayer * bleLayer, void * appState, uint16_t connDiscriminator) = 0;
// out of a peripheral that matches the given discriminator.
virtual void NewConnection(BleLayer * bleLayer, void * appState, const SetupDiscriminator & connDiscriminator) = 0;

// Call this function to stop the connection
virtual CHIP_ERROR CancelConnection() = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/ble/BleLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ CHIP_ERROR BleLayer::CancelBleIncompleteConnection()
return err;
}

CHIP_ERROR BleLayer::NewBleConnectionByDiscriminator(uint16_t connDiscriminator, void * appState,
CHIP_ERROR BleLayer::NewBleConnectionByDiscriminator(const SetupDiscriminator & connDiscriminator, void * appState,
BleConnectionDelegate::OnConnectionCompleteFunct onSuccess,
BleConnectionDelegate::OnConnectionErrorFunct onError)
{
Expand Down
3 changes: 2 additions & 1 deletion src/ble/BleLayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@

#include <ble/BleConfig.h>

#include <lib/support/SetupDiscriminator.h>
#include <system/SystemLayer.h>
#include <system/SystemPacketBuffer.h>

Expand Down Expand Up @@ -245,7 +246,7 @@ class DLL_EXPORT BleLayer
void Shutdown();

CHIP_ERROR CancelBleIncompleteConnection();
CHIP_ERROR NewBleConnectionByDiscriminator(uint16_t connDiscriminator, void * appState = nullptr,
CHIP_ERROR NewBleConnectionByDiscriminator(const SetupDiscriminator & connDiscriminator, void * appState = nullptr,
BleConnectionDelegate::OnConnectionCompleteFunct onSuccess = OnConnectionComplete,
BleConnectionDelegate::OnConnectionErrorFunct onError = OnConnectionError);
CHIP_ERROR NewBleConnectionByObject(BLE_CONNECTION_OBJECT connObj);
Expand Down
4 changes: 3 additions & 1 deletion src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,9 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
}
else if (params.HasDiscriminator())
{
SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(params.GetDiscriminator()));
SetupDiscriminator discriminator;
discriminator.SetLongValue(params.GetDiscriminator());
SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(discriminator));
}
else
{
Expand Down
6 changes: 3 additions & 3 deletions src/controller/CommissioningWindowOpener.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer);
}

mSetupPayload.version = 0;
mSetupPayload.discriminator = discriminator;
mSetupPayload.version = 0;
mSetupPayload.discriminator.SetLongValue(discriminator);
mSetupPayload.rendezvousInformation = RendezvousInformationFlags(RendezvousInformationFlag::kOnNetwork);

mCommissioningWindowCallback = callback;
Expand Down Expand Up @@ -142,7 +142,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Operationa
AdministratorCommissioning::Commands::OpenCommissioningWindow::Type request;
request.commissioningTimeout = mCommissioningWindowTimeout.count();
request.PAKEVerifier = serializedVerifierSpan;
request.discriminator = mSetupPayload.discriminator;
request.discriminator = mSetupPayload.discriminator.GetLongValue();
request.iterations = mPBKDFIterations;
request.salt = mPBKDFSalt;

Expand Down
15 changes: 11 additions & 4 deletions src/controller/SetUpCodePairer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,17 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(SetupPayload & payload)
{
ChipLogProgress(Controller, "Starting commissioning discovery over DNS-SD");

currentFilter.type = payload.isShortDiscriminator ? Dnssd::DiscoveryFilterType::kShortDiscriminator
: Dnssd::DiscoveryFilterType::kLongDiscriminator;
currentFilter.code =
payload.isShortDiscriminator ? static_cast<uint16_t>((payload.discriminator >> 8) & 0x0F) : payload.discriminator;
auto & discriminator = payload.discriminator;
if (discriminator.IsShortDiscriminator())
{
currentFilter.type = Dnssd::DiscoveryFilterType::kShortDiscriminator;
currentFilter.code = discriminator.GetShortValue();
}
else
{
currentFilter.type = Dnssd::DiscoveryFilterType::kLongDiscriminator;
currentFilter.code = discriminator.GetLongValue();
}
// Handle possibly-sync callbacks.
mWaitingForDiscovery[kIPTransport] = true;
CHIP_ERROR err = mCommissioner->DiscoverCommissionableNodes(currentFilter);
Expand Down
10 changes: 5 additions & 5 deletions src/controller/python/chip/setup_payload/Generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ extern "C" ChipError::StorageType pychip_SetupPayload_PrintOnboardingCodes(uint3
SetupPayload payload;
RendezvousInformationFlags rendezvousFlags = RendezvousInformationFlag::kNone;

payload.version = version;
payload.setUpPINCode = passcode;
payload.vendorID = vendorId;
payload.productID = productId;
payload.discriminator = discriminator;
payload.version = version;
payload.setUpPINCode = passcode;
payload.vendorID = vendorId;
payload.productID = productId;
payload.discriminator.SetLongValue(discriminator);
payload.rendezvousInformation = rendezvousFlags.SetRaw(capabilities);

switch (customFlow)
Expand Down
9 changes: 8 additions & 1 deletion src/controller/python/chip/setup_payload/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,14 @@ void YieldSetupPayloadAttributes(const SetupPayload & payload, AttributeVisitor
attrVisitor("ProductID", std::to_string(payload.productID).c_str());
attrVisitor("CommissioningFlow", std::to_string(static_cast<uint8_t>(payload.commissioningFlow)).c_str());
attrVisitor("RendezvousInformation", std::to_string(payload.rendezvousInformation.Raw()).c_str());
attrVisitor("Discriminator", std::to_string(payload.discriminator).c_str());
if (payload.discriminator.IsShortDiscriminator())
{
attrVisitor("Short discriminator", std::to_string(payload.discriminator.GetShortValue()).c_str());
}
else
{
attrVisitor("Long discriminator", std::to_string(payload.discriminator.GetLongValue()).c_str());
}
attrVisitor("SetUpPINCode", std::to_string(payload.setUpPINCode).c_str());

std::string serialNumber;
Expand Down
1 change: 1 addition & 0 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ static_library("support") {
"SafeInt.h",
"SerializableIntegerSet.cpp",
"SerializableIntegerSet.h",
"SetupDiscriminator.h",
"SortUtils.h",
"StateMachine.h",
"ThreadOperationalDataset.cpp",
Expand Down
111 changes: 111 additions & 0 deletions src/lib/support/SetupDiscriminator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
*
* Copyright (c) 2022 Project CHIP Authors
*
* 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 defines the SetupDiscriminator type, which is used by
* low-level code (e.g. BLE) in addition to setup payload code.
*/

#pragma once

#include <lib/support/CodeUtils.h>

#include <cstdint>

namespace chip {

class SetupDiscriminator
{
public:
constexpr SetupDiscriminator() : mDiscriminator(0), mIsShortDiscriminator(0) {}

// See section 5.1.2. QR Code in the Matter specification
static constexpr int kLongBits = 12;

// See section 5.1.3. Manual Pairing Code in the Matter specification
static constexpr int kShortBits = 4;

void SetShortValue(uint8_t discriminator)
{
VerifyOrDie(discriminator == (discriminator & kShortMask));
mDiscriminator = (discriminator & kShortMask);
mIsShortDiscriminator = true;
}

void SetLongValue(uint16_t discriminator)
{
VerifyOrDie(discriminator == (discriminator & kLongMask));
mDiscriminator = (discriminator & kLongMask);
mIsShortDiscriminator = false;
}

bool IsShortDiscriminator() const { return mIsShortDiscriminator; }

uint8_t GetShortValue() const
{
if (IsShortDiscriminator())
{
return static_cast<uint8_t>(mDiscriminator);
}

return LongToShortValue(mDiscriminator);
}

uint16_t GetLongValue() const
{
VerifyOrDie(!IsShortDiscriminator());
return mDiscriminator;
}

bool MatchesLongDiscriminator(uint16_t discriminator) const
{
if (!IsShortDiscriminator())
{
return mDiscriminator == discriminator;
}

return mDiscriminator == LongToShortValue(discriminator);
}

bool operator==(const SetupDiscriminator & other) const
{
return mIsShortDiscriminator == other.mIsShortDiscriminator && mDiscriminator == other.mDiscriminator;
}

private:
static_assert(kLongBits == 12, "Unexpected field length");

static constexpr uint16_t kLongMask = (1 << kLongBits) - 1;
static constexpr uint8_t kShortMask = (1 << kShortBits) - 1;

static uint8_t LongToShortValue(uint16_t longValue)
{
// Short value consists of the high bits of the long value.
constexpr int kLongToShortShift = kLongBits - kShortBits;
return static_cast<uint8_t>(longValue >> kLongToShortShift);
}

// If long discriminator, all 12 bits are used. If short discriminator,
// only the low kManualSetupDiscriminatorFieldLengthInBits bits are used, to
// store the value of the short discriminator (which contains only the high
// bits of the complete 12-bit discriminator).
uint16_t mDiscriminator : 12;
uint16_t mIsShortDiscriminator : 1;
};

} // namespace chip
2 changes: 1 addition & 1 deletion src/platform/Darwin/BleConnectionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Internal {
class BleConnectionDelegateImpl : public Ble::BleConnectionDelegate
{
public:
virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, const uint16_t connDiscriminator);
virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, const SetupDiscriminator & connDiscriminator);
virtual CHIP_ERROR CancelConnection();
};

Expand Down
Loading

0 comments on commit 39ebc91

Please sign in to comment.