Skip to content

Commit 8c42518

Browse files
sifmelcarasifmelcara
authored
Add uid to OnTransactCb argument (grpc#27328)
* Add uid to OnTransactCb argument This will allow callback functions to check if current RPC call conforms to the security policy.
1 parent 49f1f43 commit 8c42518

File tree

11 files changed

+45
-33
lines changed

11 files changed

+45
-33
lines changed

‎src/core/ext/transport/binder/server/binder_server.cc

+5-4
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,8 @@ class BinderServerListener : public Server::ListenerInterface {
9999
void Start(Server* /*server*/,
100100
const std::vector<grpc_pollset*>* /*pollsets*/) override {
101101
tx_receiver_ = factory_(
102-
[this](transaction_code_t code, grpc_binder::ReadableParcel* parcel) {
103-
return OnSetupTransport(code, parcel);
104-
});
102+
[this](transaction_code_t code, grpc_binder::ReadableParcel* parcel,
103+
int uid) { return OnSetupTransport(code, parcel, uid); });
105104
endpoint_binder_ = tx_receiver_->GetRawBinder();
106105
grpc_add_endpoint_binder(addr_, endpoint_binder_);
107106
}
@@ -127,12 +126,14 @@ class BinderServerListener : public Server::ListenerInterface {
127126

128127
private:
129128
absl::Status OnSetupTransport(transaction_code_t code,
130-
grpc_binder::ReadableParcel* parcel) {
129+
grpc_binder::ReadableParcel* parcel, int uid) {
131130
grpc_core::ExecCtx exec_ctx;
132131
if (grpc_binder::BinderTransportTxCode(code) !=
133132
grpc_binder::BinderTransportTxCode::SETUP_TRANSPORT) {
134133
return absl::InvalidArgumentError("Not a SETUP_TRANSPORT request");
135134
}
135+
// TODO(mingcl): Verify security policy here
136+
gpr_log(GPR_ERROR, "calling uid = %d", uid);
136137
int version;
137138
absl::Status status = parcel->ReadInt32(&version);
138139
if (!status.ok()) {

‎src/core/ext/transport/binder/wire_format/binder.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ class ReadableParcel {
7777
class TransactionReceiver : public HasRawBinder {
7878
public:
7979
using OnTransactCb =
80-
std::function<absl::Status(transaction_code_t, ReadableParcel*)>;
80+
std::function<absl::Status(transaction_code_t, ReadableParcel*, int uid)>;
8181

8282
~TransactionReceiver() override = default;
8383
};

‎src/core/ext/transport/binder/wire_format/binder_android.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,8 @@ binder_status_t f_onTransact(AIBinder* binder, transaction_code_t code,
7878
std::unique_ptr<ReadableParcel> output =
7979
absl::make_unique<ReadableParcelAndroid>(in);
8080
// The lock should be released "after" the callback finishes.
81-
absl::Status status = (*callback)(code, output.get());
81+
absl::Status status =
82+
(*callback)(code, output.get(), AIBinder_getCallingUid());
8283
if (status.ok()) {
8384
return STATUS_OK;
8485
} else {

‎src/core/ext/transport/binder/wire_format/wire_reader_impl.cc

+8-3
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,9 @@ void WireReaderImpl::SendSetupTransport(Binder* binder) {
120120
// callback owns a Ref() when it's being invoked.
121121
tx_receiver_ = binder->ConstructTxReceiver(
122122
/*wire_reader_ref=*/Ref(),
123-
[this](transaction_code_t code, ReadableParcel* readable_parcel) {
124-
return this->ProcessTransaction(code, readable_parcel);
123+
[this](transaction_code_t code, ReadableParcel* readable_parcel,
124+
int uid) {
125+
return this->ProcessTransaction(code, readable_parcel, uid);
125126
});
126127

127128
gpr_log(GPR_INFO, "tx_receiver = %p", tx_receiver_->GetRawBinder());
@@ -141,7 +142,8 @@ std::unique_ptr<Binder> WireReaderImpl::RecvSetupTransport() {
141142
}
142143

143144
absl::Status WireReaderImpl::ProcessTransaction(transaction_code_t code,
144-
ReadableParcel* parcel) {
145+
ReadableParcel* parcel,
146+
int uid) {
145147
gpr_log(GPR_INFO, __func__);
146148
gpr_log(GPR_INFO, "tx code = %u", code);
147149
if (code >= static_cast<unsigned>(kFirstCallId)) {
@@ -166,12 +168,15 @@ absl::Status WireReaderImpl::ProcessTransaction(transaction_code_t code,
166168
return absl::InvalidArgumentError("Transports not connected yet");
167169
}
168170

171+
// TODO(mingcl): Verify security policy for every incoming call
172+
169173
switch (BinderTransportTxCode(code)) {
170174
case BinderTransportTxCode::SETUP_TRANSPORT: {
171175
if (recvd_setup_transport_) {
172176
return absl::InvalidArgumentError(
173177
"Already received a SETUP_TRANSPORT request");
174178
}
179+
gpr_log(GPR_ERROR, "calling uid = %d", uid);
175180
recvd_setup_transport_ = true;
176181
int version;
177182
RETURN_IF_ERROR(parcel->ReadInt32(&version));

‎src/core/ext/transport/binder/wire_format/wire_reader_impl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class WireReaderImpl : public WireReader {
6767
std::unique_ptr<Binder> binder) override;
6868

6969
absl::Status ProcessTransaction(transaction_code_t code,
70-
ReadableParcel* parcel);
70+
ReadableParcel* parcel, int uid);
7171

7272
/// Send SETUP_TRANSPORT request through \p binder.
7373
///

‎test/core/transport/binder/end2end/fake_binder.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,8 @@ class PersistentFakeTransactionReceiver {
183183
std::unique_ptr<FakeBinderTunnel> tunnel);
184184

185185
absl::Status Receive(BinderTransportTxCode tx_code, ReadableParcel* parcel) {
186-
return callback_(static_cast<transaction_code_t>(tx_code), parcel);
186+
return callback_(static_cast<transaction_code_t>(tx_code), parcel,
187+
/*uid=*/0);
187188
}
188189

189190
private:

‎test/core/transport/binder/end2end/fake_binder_test.cc

+18-16
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,8 @@ TEST_P(FakeBinderTest, SendInt32) {
4747
int called = 0;
4848
std::unique_ptr<Binder> sender;
4949
std::unique_ptr<TransactionReceiver> tx_receiver;
50-
std::tie(sender, tx_receiver) =
51-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
50+
std::tie(sender, tx_receiver) = NewBinderPair(
51+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
5252
EXPECT_EQ(tx_code, kTxCode);
5353
int value = 0;
5454
EXPECT_TRUE(parcel->ReadInt32(&value).ok());
@@ -72,8 +72,8 @@ TEST_P(FakeBinderTest, SendString) {
7272
int called = 0;
7373
std::unique_ptr<Binder> sender;
7474
std::unique_ptr<TransactionReceiver> tx_receiver;
75-
std::tie(sender, tx_receiver) =
76-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
75+
std::tie(sender, tx_receiver) = NewBinderPair(
76+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
7777
EXPECT_EQ(tx_code, kTxCode);
7878
std::string value;
7979
EXPECT_TRUE(parcel->ReadString(&value).ok());
@@ -97,8 +97,8 @@ TEST_P(FakeBinderTest, SendByteArray) {
9797
int called = 0;
9898
std::unique_ptr<Binder> sender;
9999
std::unique_ptr<TransactionReceiver> tx_receiver;
100-
std::tie(sender, tx_receiver) =
101-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
100+
std::tie(sender, tx_receiver) = NewBinderPair(
101+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
102102
EXPECT_EQ(tx_code, kTxCode);
103103
std::string value;
104104
EXPECT_TRUE(parcel->ReadByteArray(&value).ok());
@@ -127,8 +127,8 @@ TEST_P(FakeBinderTest, SendMultipleItems) {
127127
int called = 0;
128128
std::unique_ptr<Binder> sender;
129129
std::unique_ptr<TransactionReceiver> tx_receiver;
130-
std::tie(sender, tx_receiver) =
131-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
130+
std::tie(sender, tx_receiver) = NewBinderPair(
131+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
132132
int value_result;
133133
EXPECT_EQ(tx_code, kTxCode);
134134
EXPECT_TRUE(parcel->ReadInt32(&value_result).ok());
@@ -163,8 +163,8 @@ TEST_P(FakeBinderTest, SendBinder) {
163163
int called = 0;
164164
std::unique_ptr<Binder> sender;
165165
std::unique_ptr<TransactionReceiver> tx_receiver;
166-
std::tie(sender, tx_receiver) =
167-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
166+
std::tie(sender, tx_receiver) = NewBinderPair(
167+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
168168
EXPECT_EQ(tx_code, kTxCode);
169169
std::unique_ptr<Binder> binder;
170170
EXPECT_TRUE(parcel->ReadBinder(&binder).ok());
@@ -179,7 +179,8 @@ TEST_P(FakeBinderTest, SendBinder) {
179179
int called2 = 0;
180180
std::unique_ptr<TransactionReceiver> tx_receiver2 =
181181
absl::make_unique<FakeTransactionReceiver>(
182-
nullptr, [&](transaction_code_t tx_code, ReadableParcel* parcel) {
182+
nullptr,
183+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
183184
int value;
184185
EXPECT_TRUE(parcel->ReadInt32(&value).ok());
185186
EXPECT_EQ(value, kValue);
@@ -204,8 +205,8 @@ TEST_P(FakeBinderTest, SendTransactionAfterDestruction) {
204205
int called = 0;
205206
{
206207
std::unique_ptr<TransactionReceiver> tx_receiver;
207-
std::tie(sender, tx_receiver) =
208-
NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) {
208+
std::tie(sender, tx_receiver) = NewBinderPair(
209+
[&](transaction_code_t tx_code, ReadableParcel* parcel, int /*uid*/) {
209210
EXPECT_EQ(tx_code, kTxCode);
210211
int value;
211212
EXPECT_TRUE(parcel->ReadInt32(&value).ok());
@@ -275,9 +276,10 @@ TEST_P(FakeBinderTest, StressTest) {
275276
std::unique_ptr<TransactionReceiver> tx_receiver;
276277
int expected_tx_code = th_arg->tx_code;
277278
std::vector<std::vector<int>>* cnt = th_arg->global_cnts;
278-
std::tie(binder, tx_receiver) = NewBinderPair(
279-
[tid, p, cnt, expected_tx_code](transaction_code_t tx_code,
280-
ReadableParcel* parcel) mutable {
279+
std::tie(binder, tx_receiver) =
280+
NewBinderPair([tid, p, cnt, expected_tx_code](
281+
transaction_code_t tx_code, ReadableParcel* parcel,
282+
int /*uid*/) mutable {
281283
EXPECT_EQ(tx_code, expected_tx_code);
282284
int value;
283285
EXPECT_TRUE(parcel->ReadInt32(&value).ok());

‎test/core/transport/binder/end2end/fuzzers/client_fuzzer.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ void FuzzingLoop(
187187
/*is_setup_transport=*/true);
188188
callback(static_cast<transaction_code_t>(
189189
grpc_binder::BinderTransportTxCode::SETUP_TRANSPORT),
190-
parcel.get())
190+
parcel.get(), /*uid=*/0)
191191
.IgnoreError();
192192
}
193193
while (data_provider.remaining_bytes() > 0) {
@@ -198,7 +198,7 @@ void FuzzingLoop(
198198
absl::make_unique<ReadableParcelForFuzzing>(
199199
&data_provider,
200200
/*is_setup_transport=*/false);
201-
callback(tx_code, parcel.get()).IgnoreError();
201+
callback(tx_code, parcel.get(), /*uid=*/0).IgnoreError();
202202
}
203203
wire_reader_ref = nullptr;
204204
}

‎test/core/transport/binder/end2end/testing_channel_create.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ class ServerSetupTransportHelper {
3535
: wire_reader_(absl::make_unique<WireReaderImpl>(
3636
/*transport_stream_receiver=*/nullptr, /*is_client=*/false)) {
3737
std::tie(endpoint_binder_, tx_receiver_) = NewBinderPair(
38-
[this](transaction_code_t tx_code, ReadableParcel* parcel) {
39-
return this->wire_reader_->ProcessTransaction(tx_code, parcel);
38+
[this](transaction_code_t tx_code, ReadableParcel* parcel, int uid) {
39+
return this->wire_reader_->ProcessTransaction(tx_code, parcel, uid);
4040
});
4141
}
4242
std::unique_ptr<Binder> WaitForClientBinder() {

‎test/core/transport/binder/mock_objects.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ class MockTransactionReceiver : public TransactionReceiver {
7777
explicit MockTransactionReceiver(OnTransactCb transact_cb,
7878
BinderTransportTxCode code,
7979
ReadableParcel* output) {
80-
transact_cb(static_cast<transaction_code_t>(code), output).IgnoreError();
80+
transact_cb(static_cast<transaction_code_t>(code), output, /*uid=*/0)
81+
.IgnoreError();
8182
}
8283

8384
MOCK_METHOD(void*, GetRawBinder, (), (override));

‎test/core/transport/binder/wire_reader_test.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,8 @@ class WireReaderTest : public ::testing::Test {
7575
template <typename T>
7676
absl::Status CallProcessTransaction(T tx_code) {
7777
return wire_reader_.ProcessTransaction(
78-
static_cast<transaction_code_t>(tx_code), &mock_readable_parcel_);
78+
static_cast<transaction_code_t>(tx_code), &mock_readable_parcel_,
79+
/*uid=*/0);
7980
}
8081

8182
std::shared_ptr<StrictMock<MockTransportStreamReceiver>>

0 commit comments

Comments
 (0)