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.

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

Commissioner/Controller init on Android may fail or crash due to use-after-free #22279

Closed
tcarmelveilleux opened this issue Aug 30, 2022 · 1 comment · Fixed by #22280
Closed

Commissioner/Controller init on Android may fail or crash due to use-after-free #22279

tcarmelveilleux opened this issue Aug 30, 2022 · 1 comment · Fixed by #22280
Assignees
@tcarmelveilleux
Labels
android crash security V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Use-after-free of JNI references in AndroidDeviceControllerWrapper can cause failure to init or crashes

Some JNI references, the ones for operational certs and IPK, have incorrect scoping when initializing the controller wrapper. Depending on system state, use after-free can arise and was observed.

This problem was found during testing with non-default Operational Credentials Issuer.

Behavior varied between versions of Android and platform. In the best case, it was an error on init, other times it was crashes.

In some cases, no crash or error occured, but certificates were subtly wrong, and found out only at time of use.

In large scale testing, this was seen during ~3% of commissioning runs.

Proposed Solution

  • Fix the lifetime issue found by code audit
@tcarmelveilleux tcarmelveilleux self-assigned this Aug 30, 2022
@andy31415
Copy link
Contributor

Patch acceptable - this is platform-specific and gatekept by platform owners (in this case Android)

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Aug 30, 2022
@tcarmelveilleux
Fix use-after-fre in AndroidDeviceControllerWrapper
f2d8ea3 
- Some JNI array refererences we used beyond their live scope in init
  of the `AndroidDeviceControllerWrapper`, causing either crashes or
  errors, in probabilistic situations (not easily reproducible, but
  seen in larger scale testing)

Fixes: project-chip#22279

This PR:
- Ensures a copy exists of the originally "passed by JNI reference" data,
  up to the point it is no longer used, for NOC, ICAC, RCAC and IPK.

Testing done:
- Integrations test pass
- Testing done also in field trials by some participants
@tcarmelveilleux tcarmelveilleux mentioned this issue Aug 30, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Aug 31, 2022
@tcarmelveilleux
Fix use-after-free in AndroidDeviceControllerWrapper (#22280)
a2d3f39 
* Fix use-after-fre in AndroidDeviceControllerWrapper

- Some JNI array refererences we used beyond their live scope in init
  of the `AndroidDeviceControllerWrapper`, causing either crashes or
  errors, in probabilistic situations (not easily reproducible, but
  seen in larger scale testing)

Fixes: #22279

This PR:
- Ensures a copy exists of the originally "passed by JNI reference" data,
  up to the point it is no longer used, for NOC, ICAC, RCAC and IPK.

Testing done:
- Integrations test pass
- Testing done also in field trials by some participants

* Restyled
isiu-apple pushed a commit to isiu-apple/connectedhomeip that referenced this issue Sep 16, 2022
@tcarmelveilleux @isiu-apple
Fix use-after-free in AndroidDeviceControllerWrapper (project-chip#22280
988fc77 
)

* Fix use-after-fre in AndroidDeviceControllerWrapper

- Some JNI array refererences we used beyond their live scope in init
  of the `AndroidDeviceControllerWrapper`, causing either crashes or
  errors, in probabilistic situations (not easily reproducible, but
  seen in larger scale testing)

Fixes: project-chip#22279

This PR:
- Ensures a copy exists of the originally "passed by JNI reference" data,
  up to the point it is no longer used, for NOC, ICAC, RCAC and IPK.

Testing done:
- Integrations test pass
- Testing done also in field trials by some participants

* Restyled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
android crash security V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix use-after-free in AndroidDeviceControllerWrapper tcarmelveilleux/connectedhomeip
2 participants
@tcarmelveilleux @andy31415