Skip to content

Commit

Permalink
Fix use-after-free in AndroidDeviceControllerWrapper (#22280)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
tcarmelveilleux authored and pull[bot] committed Feb 13, 2024
1 parent 4716612 commit 1248672
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 4 deletions.
31 changes: 27 additions & 4 deletions src/controller/java/AndroidDeviceControllerWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@

#include <algorithm>
#include <memory>
#include <vector>

#include <string.h>

#include <lib/support/CodeUtils.h>
#include <lib/support/JniReferences.h>
Expand Down Expand Up @@ -224,9 +227,24 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(
JniByteArray jniIcac(env, intermediateCertificate);
JniByteArray jniNoc(env, nodeOperationalCertificate);

setupParams.controllerRCAC = jniRcac.byteSpan();
setupParams.controllerICAC = jniIcac.byteSpan();
setupParams.controllerNOC = jniNoc.byteSpan();
// Make copies of the cert that outlive the scope so that future factor init does not
// cause loss of scope from the JNI refs going away. Also, this keeps the certs
// handy for debugging commissioner init.
wrapper->mRcacCertificate = std::vector<uint8_t>(jniRcac.byteSpan().begin(), jniRcac.byteSpan().end());
if (!jniIcac.byteSpan().empty())
{
wrapper->mIcacCertificate = std::vector<uint8_t>(jniIcac.byteSpan().begin(), jniIcac.byteSpan().end());
}
else
{
wrapper->mIcacCertificate.clear();
}

wrapper->mNocCertificate = std::vector<uint8_t>(jniNoc.byteSpan().begin(), jniNoc.byteSpan().end());

setupParams.controllerRCAC = chip::ByteSpan(wrapper->mRcacCertificate.data(), wrapper->mRcacCertificate.size());
setupParams.controllerICAC = chip::ByteSpan(wrapper->mIcacCertificate.data(), wrapper->mIcacCertificate.size());
setupParams.controllerNOC = chip::ByteSpan(wrapper->mNocCertificate.data(), wrapper->mNocCertificate.size());
}
else
{
Expand Down Expand Up @@ -276,10 +294,12 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(
ChipLogByteSpan(Support, compressedFabricIdSpan);

chip::ByteSpan ipkSpan;
std::vector<uint8_t> ipkBuffer;
if (ipkEpochKey != nullptr)
{
JniByteArray jniIpk(env, ipkEpochKey);
ipkSpan = jniIpk.byteSpan();
ipkBuffer = std::vector<uint8_t>(jniIpk.byteSpan().begin(), jniIpk.byteSpan().end());
ipkSpan = chip::ByteSpan(ipkBuffer.data(), ipkBuffer.size());
}
else
{
Expand All @@ -288,6 +308,9 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(

*errInfoOnFailure = chip::Credentials::SetSingleIpkEpochKey(
&wrapper->mGroupDataProvider, wrapper->Controller()->GetFabricIndex(), ipkSpan, compressedFabricIdSpan);

memset(ipkBuffer.data(), 0, ipkBuffer.size());

if (*errInfoOnFailure != CHIP_NO_ERROR)
{
return nullptr;
Expand Down
5 changes: 5 additions & 0 deletions src/controller/java/AndroidDeviceControllerWrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <lib/support/JniReferences.h>

#include <memory>
#include <vector>

#include <jni.h>

Expand Down Expand Up @@ -178,6 +179,10 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel
jbyteArray operationalDatasetBytes = nullptr;
jbyte * operationalDataset = nullptr;

std::vector<uint8_t> mNocCertificate;
std::vector<uint8_t> mIcacCertificate;
std::vector<uint8_t> mRcacCertificate;

chip::Controller::AutoCommissioner mAutoCommissioner;

chip::Credentials::PartialDACVerifier mPartialDACVerifier;
Expand Down

0 comments on commit 1248672

Please sign in to comment.