Skip to content

Fix data corruption/mangling in DeviceCommissioner::SendOperationalCertificate #7309

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 2, 2021
Merged

Fix data corruption/mangling in DeviceCommissioner::SendOperationalCertificate #7309

merged 1 commit into from
Jun 2, 2021

Conversation

bluebin14
Copy link
Contributor

Problem

What is being fixed?

Change overview

What's in this PR

DeviceCommissioner::SendOperationalCertificate now correctly sends both the op cert and the ICA cert. Previously it was corrupting them both, thinking that TempZCL protocol needs to be supported which has a 255 byte limitation. TempZCL protocol is not involved here, the code uses Interaction Model exclusively which ends up in OperationalCredentialsCluster::AddOpCert which already does the right thing when the byte span is larger than 255 bytes.

Testing

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

  • tested manually by sending more than 255 bytes per certificate and verifying the following:
  • certs are not split (previously op cert was split)
  • both certs are sent (previously ica cert was ignored)

@bluebin14 bluebin14 mentioned this pull request Jun 2, 2021
Closed
@yunhanw-google yunhanw-google requested a review from pan-apple June 2, 2021 16:56
@yunhanw-google
Copy link
Contributor

@pan-apple please take a look, thanks

@pan-apple
Copy link
Contributor

@bluebin14 , thanks for putting up the PR. I think we should also update the code in the following function. That's the server side, which has the logic to reassemble the split certificate. Even though the current code will work, but eventually we'll send ICA certificates as well. That'll break the flow, as it'll try to combine the op cert and ICA cert buffers.

bool emberAfOperationalCredentialsClusterAddOpCertCallback(chip::app::Command * commandObj, chip::ByteSpan NOC,
                                                           chip::ByteSpan ICACertificate, chip::ByteSpan IPKValue,
                                                           chip::NodeId CaseAdminNode, uint16_t AdminVendorId)
```

@bluebin14
Copy link
Contributor Author

@pan-apple yes, right now the server side is not affected by this change even though the server indeed does strange things: if ica is used (eventually) then emberAfOperationalCredentialsClusterAddOpCertCallback combines the 2 certs into an unparseable tlv and passes the blob as 1 certificate to SetOperationalCert which will of course fail. It seems there are lot of todo's there. This here only removes the imaginary 255 byte limitation on the client side.

@andy31415 andy31415 merged commit bb8a6ef into project-chip:master Jun 2, 2021
@bluebin14 bluebin14 deleted the fix_opcert branch June 26, 2021 12:57
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@bluebin14
Fix data corruption/mangling in DeviceCommissioner::SendOperationalCe…
9da2d3c 
…rtificate (project-chip#7309)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andy31415 andy31415 andy31415 approved these changes

@saurabhst saurabhst saurabhst approved these changes

@Damian-Nordic Damian-Nordic Damian-Nordic approved these changes

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@pan-apple pan-apple Awaiting requested review from pan-apple

+1 more reviewer

@msandstedt msandstedt msandstedt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

controller review - approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix data corruption/mangling in DeviceCommissioner::SendOperationalCertificate

7 participants

@bluebin14 @yunhanw-google @pan-apple @andy31415 @msandstedt @saurabhst @Damian-Nordic