Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Aug 19, 2021
Merged

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

merged 4 commits into from
Aug 19, 2021

Conversation

yufengwangca
Copy link
Contributor

@yufengwangca yufengwangca commented Jul 15, 2021

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.

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

Change overview

  1. Use sessionId 0 for unsecured session
  2. 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)

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

@kghost
Copy link
Contributor

kghost commented Jul 16, 2021

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

@yufengwangca 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 yufengwangca requested a review from turon July 16, 2021 17:20
@yufengwangca
Copy link
Contributor Author

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

@yufengwangca yufengwangca requested a review from kghost July 16, 2021 17:22
@kghost
Copy link
Contributor

kghost commented Jul 20, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

@msandstedt msandstedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

stale bot commented Aug 2, 2021

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.

@stale stale bot added the stale Stale issue or PR label Aug 2, 2021
@yufengwangca
Copy link
Contributor Author

@stale stale bot removed the stale Stale issue or PR label Aug 2, 2021
@woody-apple
Copy link
Contributor

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

Copy link
Contributor

@kghost kghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yufengwangca
Copy link
Contributor Author

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

Thanks, that is already part of this change.

@pan-apple pan-apple self-requested a review August 16, 2021 20:35
src/transport/SessionHandle.h Outdated Show resolved Hide resolved
src/transport/SessionHandle.h Outdated Show resolved Hide resolved
@todo
Copy link

todo bot commented Aug 18, 2021

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

// 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
Copy link

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
Copy link

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
Copy link

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


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a solution to get correct generated SecureSessionHandle for each new connection
9 participants