Update the exchange match logic to align with the latest spec proposal #8441

yufengwangca · 2021-07-15T19:59:52Z

Problem

What is being fixed?

Currently, SecureSessionHandle is used to match exchange for the incoming message in addition to exchange id and node id, but an exchange is not tied to a secure session in the spec. Before app send an message or create an new exchange, there is no way to know the keyid of a SecureSessionHandle up front, right now, we temporarily hard coded the keyid to 0 to pass the exchange match check.

After discussion, we agree to update spec with the following changes.

reserve sessionId 0 for unsecured sessions
clearly specify the Session ID is one of the metadata associated with an Exchange.

Fixes Need a solution to get correct generated SecureSessionHandle for each new connection #4451

Change overview

Use sessionId 0 for unsecured session
Match incoming message's source Session ID with the Session ID associated with the exchange during the exchange match check.

Testing

How was this tested? (at least one bullet point required)

./chip-echo-responder
./chip-echo-requester
confirm the echo test pass with this change.
./chip-im-responder
./chip-im-initiator
confirm the im test pass with this change.

src/messaging/ExchangeContext.cpp

kghost · 2021-07-16T11:11:48Z

Discussed with Boris and Martin, actions should be taken on the spec side first.

yufengwangca · 2021-07-16T17:22:16Z

@turon could you confirm if the change in this PR aligned with your proposal in spec?

kghost · 2021-07-20T12:46:53Z

The new patch resolves my concern, but I still think comparing mSecureSession is the right way.

I'm considering remove all packetHeader and peerAddress arguments above transport layer, because it is not trust-able, and shouldn't be used above transport layer.

msandstedt · 2021-07-20T21:33:39Z

The new patch resolves my concern, but I still think comparing mSecureSession is the right way.

I'm considering remove all packetHeader and peerAddress arguments above transport layer, because it is not trust-able, and shouldn't be used above transport layer.

I agree. Does this block this PR, or do we want to write some TODO tickets?

When messages come in the door, we have:

session ID -> by definition authenticated
source ID -> part of plaintext nonce
destination ID -> plaintext authenticated

Obviously session ID is the primary key we should be using to recover internal context. Perhaps source ID and destination ID are also OK to pass to higher levels of the stack. But allowing higher level code to key on anything else seems like a good way to inadvertently create security vulnerabilities.

yufengwangca · 2021-07-20T22:42:56Z

The new patch resolves my concern, but I still think comparing mSecureSession is the right way.
I'm considering remove all packetHeader and peerAddress arguments above transport layer, because it is not trust-able, and shouldn't be used above transport layer.

I agree. Does this block this PR, or do we want to write some TODO tickets?

When messages come in the door, we have:

session ID -> by definition authenticated

source ID -> part of plaintext nonce

destination ID -> plaintext authenticated

Obviously session ID is the primary key we should be using to recover internal context. Perhaps source ID and destination ID are also OK to pass to higher levels of the stack. But allowing higher level code to key on anything else seems like a good way to inadvertently create security vulnerabilities.

The spec change(CHIP-Specifications/connectedhomeip-spec#3588) has specified how should we identify an exchange.
The Exchange is then identified by the tuple {Session ID, Initiator Fabric ID, Initiator Node ID, Exchange ID}.

The logic in the PR is implemented against the spec except comparing the Initiator Fabric ID (Fabric logic is not completely ready in transport layer, will add this logic later to fully comply with the spec).

bzbarsky-apple · 2021-07-22T22:25:23Z

When messages come in the door, we have:
session ID -> by definition authenticated
source ID -> part of plaintext nonce
destination ID -> plaintext authenticated

Just as a minor correction: for unicast encrypted messages, spec does not send a source id and destination id at all. They are recoverable (and recovered) from the session id.

msandstedt · 2021-07-23T14:15:06Z

The new patch resolves my concern, but I still think comparing mSecureSession is the right way.
I'm considering remove all packetHeader and peerAddress arguments above transport layer, because it is not trust-able, and shouldn't be used above transport layer.

I agree. Does this block this PR, or do we want to write some TODO tickets?
When messages come in the door, we have:

session ID -> by definition authenticated

source ID -> part of plaintext nonce

destination ID -> plaintext authenticated

Obviously session ID is the primary key we should be using to recover internal context. Perhaps source ID and destination ID are also OK to pass to higher levels of the stack. But allowing higher level code to key on anything else seems like a good way to inadvertently create security vulnerabilities.

The spec change(CHIP-Specifications/connectedhomeip-spec#3588) has specified how should we identify an exchange.
The Exchange is then identified by the tuple {Session ID, Initiator Fabric ID, Initiator Node ID, Exchange ID}.

The logic in the PR is implemented against the spec except comparing the Initiator Fabric ID (Fabric logic is not completely ready in transport layer, will add this logic later to fully comply with the spec).

Yes, this needs to be addressed in (CHIP-Specifications/connectedhomeip-spec#3588) as well. What's proposed there doesn't make sense.

The session ID is an abstraction that, from the perspective of a recipient, can map to a subject. But the subject may or may not be a fabric+node identity. In the case of a PASE session, it isn't. If exchanges can exist over PASE, then keying on fabric+node identity doesn't work.

msandstedt

This code is attempting session match based upon the peer key ID. That's not guaranteed unique and not secure.

Session ID is the local key ID, and this is what should be used to verify session match.

stale · 2021-08-02T15:35:39Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

yufengwangca · 2021-08-02T16:17:24Z

Hold until https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/3588 is resolved

woody-apple · 2021-08-04T16:33:48Z

Removing SDK Discussion required, this doesn't require anything beyond the normal PR approval process.

kghost

It looks fine to me. I'd like to rename SecureSessionHandle to SessionHandle

yufengwangca · 2021-08-16T15:37:28Z

It looks fine to me. I'd like to rename SecureSessionHandle to SessionHandle

Thanks, that is already part of this change.

src/transport/SessionHandle.h

todo · 2021-08-18T21:37:02Z

Temporarily keep the old logic, check why only those two fields are used in comparison.

connectedhomeip/src/transport/SessionHandle.h

Lines 44 to 54 in e715539

    
               // TODO: Temporarily keep the old logic, check why only those two fields are used in comparison. 
        
               return mPeerNodeId == that.mPeerNodeId && mPeerKeyId == that.mPeerKeyId; 
        
           } 
        
           bool MatchIncomingSession(const SessionHandle & that) const 
        
           { 
        
               if (that.GetLocalKeyId().HasValue()) 
        
               { 
        
                   return mLocalKeyId == that.mLocalKeyId; 
        
               }

This comment was generated by todo based on a `TODO` comment in `e715539` in #8441. cc @yufengwangca.

github-actions · 2021-08-18T21:57:25Z

Size increase report for "gn_qpg-example-build" from 0bdeccb

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	216	216
chip-qpg6100-lighting-example.out	.bss	0	64
chip-qpg6100-lighting-example.out	.heap	0	-64

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,27374
.debug_loc,0,2575
.debug_line,0,618
.debug_ranges,0,544
.text,216,216
.bss,64,0
.debug_abbrev,0,43
.debug_frame,0,12
.shstrtab,0,-2
.heap,-64,0
.strtab,0,-138
[Unmapped],0,-216
.debug_str,0,-298

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

github-actions · 2021-08-18T22:35:16Z

Size increase report for "esp32-example-build" from 0bdeccb

File	Section	File	VM
chip-lock-app.elf	.flash.text	168	168
chip-lock-app.elf	.dram0.bss	0	64
chip-temperature-measurement-app.elf	.flash.text	268	268
chip-temperature-measurement-app.elf	.dram0.bss	0	64
chip-ipv6only-app.elf	.flash.text	172	172
chip-all-clusters-app.elf	.flash.text	168	168
chip-all-clusters-app.elf	.dram0.bss	0	64
chip-shell.elf	.flash.text	300	300
chip-shell.elf	.dram0.bss	0	72
chip-bridge-app.elf	.flash.text	224	224
chip-bridge-app.elf	.dram0.bss	0	64

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,10850
.debug_loc,0,2029
.debug_line,0,1137
.debug_ranges,0,800
.flash.text,168,168
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,140
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,88
.debug_abbrev,0,65
.dram0.bss,64,0
.debug_aranges,0,-8
.shstrtab,0,-11
.symtab,0,-16
.debug_frame,0,-24
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-88
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-140
[Unmapped],0,-168
.strtab,0,-189
.debug_str,0,-293

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,31975
.debug_loc,0,2272
.debug_line,0,1155
.debug_ranges,0,720
.flash.text,268,268
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,140
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,88
.dram0.bss,64,0
.debug_abbrev,0,57
.shstrtab,0,-11
.symtab,0,-16
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-88
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-140
.strtab,0,-189
[Unmapped],0,-268
.debug_str,0,-295

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
.flash.text,172,172
[Unmapped],0,-172

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,11513
.debug_loc,0,1887
.debug_ranges,0,496
.debug_line,0,315
.flash.text,168,168
.dram0.bss,64,0
.debug_abbrev,0,40
.debug_frame,0,4
.shstrtab,0,2
.riscv.attributes,0,-1
.strtab,0,-138
[Unmapped],0,-168
.debug_str,0,-294

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,9688
.debug_loc,0,2178
.debug_line,0,1190
.debug_ranges,0,768
.flash.text,300,300
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,140
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,88
.debug_abbrev,0,86
.dram0.bss,72,0
.shstrtab,0,-12
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-88
.strtab,0,-132
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-140
.debug_str,0,-258
[Unmapped],0,-300

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
.debug_info,0,35246
.debug_loc,0,2281
.debug_line,0,1119
.debug_ranges,0,800
.flash.text,224,224
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,140
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_13SessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,88
.debug_abbrev,0,65
.dram0.bss,64,0
.debug_aranges,0,-8
.shstrtab,0,-11
.symtab,0,-16
.debug_frame,0,-24
.xt.lit._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-88
.xt.prop._ZN4chip16BitMapObjectPoolINS_9Messaging15ExchangeContextELj8EE12CreateObjectIJPNS1_15ExchangeManagerEtRNS_19SecureSessionHandleEbRPNS1_16ExchangeDelegateEEEEPS2_DpOT_,0,-140
.strtab,0,-189
[Unmapped],0,-224
.debug_str,0,-295

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

github-actions · 2021-08-18T22:35:30Z

Size increase report for "nrfconnect-example-build" from 0bdeccb

File	Section	File	VM
chip-shell.elf	text	248	248
chip-shell.elf	bss	0	67
chip-shell.elf	[LOAD #3 [RW]]	0	29
chip-shell.elf	device_handles	8	8
chip-lock.elf	text	208	208
chip-lock.elf	bss	0	64

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,10675
.debug_loc,0,2524
.debug_line,0,817
.debug_ranges,0,568
text,248,248
bss,67,0
.debug_abbrev,0,64
[LOAD #3 [RW]],29,0
device_handles,8,8
.strtab,0,-132
.debug_str,0,-264

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,27361
.debug_loc,0,2732
.debug_line,0,624
.debug_ranges,0,552
text,208,208
bss,64,0
.debug_abbrev,0,43
.debug_frame,0,12
.shstrtab,0,-2
.strtab,0,-138
.debug_str,0,-300

project-chip#8441) * Update the exchange match logic to align with the latest spec proposal * Rename SecureSessionHandle to SessionHandle * Make mLocalKeyId and mPeerKeyId optional * Rename match to matchIncomingSession

boring-cyborg bot added the app label Jul 15, 2021

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs and msandstedt July 15, 2021 20:02

pullapprove bot added the review - pending label Jul 15, 2021

yufengwangca requested a review from kghost July 15, 2021 20:04

kghost suggested changes Jul 15, 2021

View reviewed changes

src/messaging/ExchangeContext.cpp Show resolved Hide resolved

yufengwangca changed the title ~~Stop using keyid to match exchange for incoming message~~ Update the exchange match logic to align with the latest spec proposal Jul 16, 2021

yufengwangca requested a review from turon July 16, 2021 17:20

yufengwangca requested a review from kghost July 16, 2021 17:22

woody-apple added the SDK Approved label Jul 20, 2021

woody-apple added SDK Discussion Required and removed SDK Approved labels Jul 22, 2021

msandstedt suggested changes Jul 26, 2021

View reviewed changes

stale bot added the stale Stale issue or PR label Aug 2, 2021

stale bot removed the stale Stale issue or PR label Aug 2, 2021

kghost approved these changes Aug 15, 2021

View reviewed changes

pan-apple self-requested a review August 16, 2021 20:35

pan-apple approved these changes Aug 16, 2021

View reviewed changes

yufengwangca requested review from woody-apple and jmartinez-silabs August 16, 2021 21:40

andy31415 reviewed Aug 18, 2021

View reviewed changes

src/transport/SessionHandle.h Outdated Show resolved Hide resolved

src/transport/SessionHandle.h Outdated Show resolved Hide resolved

yufengwangca added 4 commits August 18, 2021 14:38

Update the exchange match logic to align with the latest spec proposal

2990f90

Rename SecureSessionHandle to SessionHandle

2c49d50

Make mLocalKeyId and mPeerKeyId optional

d0a6623

Rename match to matchIncomingSession

f270ff8

andy31415 approved these changes Aug 19, 2021

View reviewed changes

jmartinez-silabs approved these changes Aug 19, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Aug 19, 2021

saurabhst approved these changes Aug 19, 2021

View reviewed changes

saurabhst merged commit 17a00b0 into project-chip:master Aug 19, 2021

yufengwangca deleted the pr/exchange/session branch August 19, 2021 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the exchange match logic to align with the latest spec proposal #8441

Update the exchange match logic to align with the latest spec proposal #8441

yufengwangca commented Jul 15, 2021 •

edited

Loading

kghost commented Jul 16, 2021

yufengwangca commented Jul 16, 2021

kghost commented Jul 20, 2021 •

edited

Loading

msandstedt commented Jul 20, 2021

yufengwangca commented Jul 20, 2021

bzbarsky-apple commented Jul 22, 2021

msandstedt commented Jul 23, 2021

msandstedt left a comment

stale bot commented Aug 2, 2021

yufengwangca commented Aug 2, 2021

woody-apple commented Aug 4, 2021

kghost left a comment

yufengwangca commented Aug 16, 2021

todo bot commented Aug 18, 2021

github-actions bot commented Aug 18, 2021

github-actions bot commented Aug 18, 2021

github-actions bot commented Aug 18, 2021

Update the exchange match logic to align with the latest spec proposal #8441

Update the exchange match logic to align with the latest spec proposal #8441

Conversation

yufengwangca commented Jul 15, 2021 • edited Loading

Problem

Change overview

Testing

kghost commented Jul 16, 2021

yufengwangca commented Jul 16, 2021

kghost commented Jul 20, 2021 • edited Loading

msandstedt commented Jul 20, 2021

yufengwangca commented Jul 20, 2021

bzbarsky-apple commented Jul 22, 2021

msandstedt commented Jul 23, 2021

msandstedt left a comment

Choose a reason for hiding this comment

stale bot commented Aug 2, 2021

yufengwangca commented Aug 2, 2021

woody-apple commented Aug 4, 2021

kghost left a comment

Choose a reason for hiding this comment

yufengwangca commented Aug 16, 2021

todo bot commented Aug 18, 2021

Temporarily keep the old logic, check why only those two fields are used in comparison.

This comment was generated by todo based on a TODO comment in e715539 in #8441. cc @yufengwangca.

github-actions bot commented Aug 18, 2021

github-actions bot commented Aug 18, 2021

github-actions bot commented Aug 18, 2021

yufengwangca commented Jul 15, 2021 •

edited

Loading

kghost commented Jul 20, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `e715539` in #8441. cc @yufengwangca.