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

Prevent reception of unencrypted messages in an encrypted message exchange #8556

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

pan-apple
Copy link
Contributor

Problem

A malicious node can currently send unencrypted messages to an exchange that requires encrypted messages.

Change overview

Currently the only unencrypted messages a device should receive are session establishment messages.
This PR adds a flag to the exchange dispatch, which defines if encryption is mandatory for a message exchange. This flag is set to true by default for every exchange, except for session establishment message dispatch class.

Testing

Added a new unit test CheckUnencryptedMessageReceiveFailure that tries to send an unencrypted message to an exchange that requires encryption. The test expects that the message will be sent, but dropped by the receiver.

@pan-apple
Copy link
Contributor Author

@saurabhst do you have any review feedback?

@pan-apple
Copy link
Contributor Author

/rebase

@pan-apple pan-apple force-pushed the secure-message-check branch from a26adbd to a2ef424 Compare July 23, 2021 21:13
@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 762a204

File Section File VM
chip-qpg6100-lighting-example.out .text 52 52
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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'

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

sections,vmsize,filesize
.debug_info,0,982
.debug_loc,0,275
.debug_str,0,227
.strtab,0,207
.debug_line,0,140
.symtab,0,64
.debug_ranges,0,56
.debug_frame,0,52
.text,52,52
.debug_aranges,0,24
.shstrtab,0,1
[Unmapped],0,-52


@github-actions
Copy link

Size increase report for "esp32-example-build" from 762a204

File Section File VM
chip-temperature-measurement-app.elf .flash.text 100 100
chip-temperature-measurement-app.elf .flash.rodata 8 8
chip-lock-app.elf .flash.text 104 104
chip-lock-app.elf .flash.rodata 8 8
chip-all-clusters-app.elf .flash.text 44 44
chip-all-clusters-app.elf .flash.rodata 8 8
chip-shell.elf .flash.text 32 32
chip-shell.elf .flash.rodata 8 8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,991
.debug_str,0,230
.debug_line,0,218
.strtab,0,207
.debug_loc,0,193
.shstrtab,0,161
.flash.text,100,100
.xt.prop._ZNK4chip36SessionEstablishmentExchangeDispatch20IsEncryptionRequiredEv,0,76
.xt.prop._ZNK4chip9Messaging23ExchangeMessageDispatch20IsEncryptionRequiredEv,0,76
.debug_frame,0,72
.symtab,0,64
.debug_aranges,0,24
.debug_ranges,0,16
.flash.rodata,8,8
[Unmapped],0,-108

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_info,0,993
.debug_loc,0,311
.debug_line,0,226
.debug_str,0,226
.strtab,0,207
.shstrtab,0,157
.flash.text,104,104
.xt.prop._ZNK4chip36SessionEstablishmentExchangeDispatch20IsEncryptionRequiredEv,0,76
.xt.prop._ZNK4chip9Messaging23ExchangeMessageDispatch20IsEncryptionRequiredEv,0,76
.debug_frame,0,72
.symtab,0,64
.debug_aranges,0,24
.debug_ranges,0,16
.flash.rodata,8,8
[Unmapped],0,-112

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,934
.debug_str,0,228
.strtab,0,207
.debug_line,0,162
.debug_frame,0,48
.flash.text,44,44
.symtab,0,32
.debug_aranges,0,24
.flash.rodata,8,8
.riscv.attributes,0,-2
.shstrtab,0,-3
.debug_ranges,0,-8
[Unmapped],0,-52
.debug_loc,0,-102

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

sections,vmsize,filesize
.debug_info,0,571
.debug_loc,0,217
.debug_line,0,176
.debug_str,0,120
.strtab,0,102
.shstrtab,0,78
.xt.prop._ZNK4chip9Messaging23ExchangeMessageDispatch20IsEncryptionRequiredEv,0,76
.debug_frame,0,48
.flash.text,32,32
.symtab,0,32
.debug_aranges,0,16
.debug_ranges,0,8
.flash.rodata,8,8
[Unmapped],0,-40


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 762a204

File Section File VM
chip-lock.elf text 40 40
chip-lock.elf device_handles 8 8
chip-lock.elf rodata 8 8
chip-shell.elf text 32 32
chip-shell.elf rodata 8 4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,983
.debug_loc,0,360
.debug_str,0,228
.strtab,0,207
.debug_line,0,141
.symtab,0,64
.debug_frame,0,56
.debug_ranges,0,56
text,40,40
.debug_aranges,0,24
device_handles,8,8
rodata,8,8
.shstrtab,0,-3

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

sections,vmsize,filesize
.debug_info,0,563
.debug_loc,0,301
.debug_str,0,123
.debug_line,0,109
.strtab,0,102
.debug_ranges,0,48
.debug_frame,0,40
.symtab,0,32
text,32,32
.debug_aranges,0,16
rodata,4,8
.shstrtab,0,-2


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.

I think most of the reviewers agree that this is only a temporary solution. However, the prevailing thought seems to be this is fine to merge now and we can refine later.

I disagree. This does not address the fact that we have a model now that is essentially:

if (session is valid) {
  inject incoming message into X context without authentication
}

It is not sufficient just to verify that an incoming message belongs to one of the open sessions. That's a recipe for privilege escalation. For the security model to survive, each exchange must map to a specific session.

@pan-apple pan-apple requested a review from msandstedt July 27, 2021 23:22
@pan-apple
Copy link
Contributor Author

I think most of the reviewers agree that this is only a temporary solution. However, the prevailing thought seems to be this is fine to merge now and we can refine later.

I disagree. This does not address the fact that we have a model now that is essentially:

if (session is valid) {
  inject incoming message into X context without authentication
}

It is not sufficient just to verify that an incoming message belongs to one of the open sessions. That's a recipe for privilege escalation. For the security model to survive, each exchange must map to a specific session.

Please see #8556 (comment)

@bzbarsky-apple bzbarsky-apple merged commit 51a9262 into project-chip:master Jul 28, 2021
@pan-apple pan-apple deleted the secure-message-check branch July 28, 2021 15:37
@kghost kghost mentioned this pull request Jul 29, 2021
andy31415 added a commit to andy31415/connectedhomeip that referenced this pull request Jul 29, 2021
…sage exchange (project-chip#8556)"

This reverts commit 51a9262.

CI failure with core dump started on this PR.
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…hange (project-chip#8556)

* Prevent injection of unencrypted messages in an application message exchange

* fix test
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.

7 participants