Skip to content

Commit 67ca31a

Browse files
xaionaro@dx.centerxaionaro@dx.center
authored andcommitted
fix: 11 semantic integrity bugs in ACL, JNI proxy, and generators
ACL security fixes: - ApproveRequest/DenyRequest now require status='pending', preventing re-approval of denied requests or re-denial of approved ones - RevokeClient deletes orphaned pending_requests, preventing re-granting access to revoked clients via stale pending requests - Reject empty CommonName in Register to prevent shared identity JNI proxy correctness fixes: - Guard GetByteArrayData against zero-length arrays (panic on &data[0]) - Close all pending callback channels on Proxy stream exit, preventing permanent JVM thread deadlock on in-flight callbacks - Return InvalidArgument from SetField/SetStaticField when value is nil instead of silently no-oping - Release voidDetector global JNI ref on stream close (leak per stream) - Release callback argument handles after response received (global ref leak) - Remove TOCTOU pre-check in ReleaseHandle; Release is already atomic Also fix m.Error→m.HasError field name in grpcgen server/client builders (introduced by prior subagent, caught by test).
1 parent 1a9826a commit 67ca31a

9 files changed

Lines changed: 84 additions & 17 deletions

File tree

grpc/server/acl/store.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,9 @@ func (s *Store) RevokeClient(clientID string) error {
154154
if _, err := tx.Exec(`DELETE FROM grants WHERE client_id = ?`, clientID); err != nil {
155155
return fmt.Errorf("deleting grants for client %q: %w", clientID, err)
156156
}
157+
if _, err := tx.Exec(`DELETE FROM pending_requests WHERE client_id = ?`, clientID); err != nil {
158+
return fmt.Errorf("deleting pending requests for client %q: %w", clientID, err)
159+
}
157160
if _, err := tx.Exec(`DELETE FROM clients WHERE client_id = ?`, clientID); err != nil {
158161
return fmt.Errorf("deleting client %q: %w", clientID, err)
159162
}
@@ -347,11 +350,11 @@ func (s *Store) ApproveRequest(
347350

348351
var clientID, methodsJSON string
349352
err = tx.QueryRow(
350-
`SELECT client_id, methods FROM pending_requests WHERE id = ?`,
353+
`SELECT client_id, methods FROM pending_requests WHERE id = ? AND status = 'pending'`,
351354
requestID,
352355
).Scan(&clientID, &methodsJSON)
353356
if err != nil {
354-
return fmt.Errorf("looking up request %d: %w", requestID, err)
357+
return fmt.Errorf("looking up pending request %d: %w", requestID, err)
355358
}
356359

357360
var methods []string
@@ -384,7 +387,7 @@ func (s *Store) ApproveRequest(
384387
// DenyRequest marks the request as 'denied' without creating any grants.
385388
func (s *Store) DenyRequest(requestID int64) error {
386389
_, err := s.db.Exec(
387-
`UPDATE pending_requests SET status = 'denied' WHERE id = ?`,
390+
`UPDATE pending_requests SET status = 'denied' WHERE id = ? AND status = 'pending'`,
388391
requestID,
389392
)
390393
if err != nil {

grpc/server/auth_service.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,14 @@ func (s *AuthServiceServer) Register(
3939
return nil, status.Errorf(codes.Internal, "parse signed cert: %v", err)
4040
}
4141

42+
clientID := cert.Subject.CommonName
43+
if clientID == "" {
44+
return nil, status.Errorf(codes.InvalidArgument, "CSR must have a non-empty CommonName")
45+
}
46+
4247
fingerprint := fmt.Sprintf("sha256:%x", sha256.Sum256(cert.Raw))
4348

44-
if err := s.Store.RegisterClient(cert.Subject.CommonName, string(certPEM), fingerprint); err != nil {
49+
if err := s.Store.RegisterClient(clientID, string(certPEM), fingerprint); err != nil {
4550
return nil, status.Errorf(codes.Internal, "register client: %v", err)
4651
}
4752

grpc/server/jni_raw/proxy_stream.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,20 @@ func (s *Server) Proxy(stream pb.JNIService_ProxyServer) error {
7777
}
7878
}
7979

80+
// Release callback argument handles when the callback is done,
81+
// regardless of which exit path is taken (send error, void
82+
// fire-and-forget, or normal response).
83+
defer func() {
84+
for _, h := range argHandles {
85+
if h != 0 {
86+
_ = s.VM.Do(func(env *jni.Env) error {
87+
s.Handles.Release(env, h)
88+
return nil
89+
})
90+
}
91+
}
92+
}()
93+
8094
// Send callback event to client.
8195
event := &pb.ProxyServerMessage{
8296
Msg: &pb.ProxyServerMessage_Callback{
@@ -275,6 +289,24 @@ func (s *Server) Proxy(stream pb.JNIService_ProxyServer) error {
275289
}
276290
}
277291
defer proxyCleanup()
292+
defer func() {
293+
// Release the voidDetector's cached JNI global ref (Void.TYPE).
294+
_ = s.VM.Do(func(env *jni.Env) error {
295+
detector.close(env)
296+
return nil
297+
})
298+
}()
299+
300+
// Close all pending callback channels when the stream ends so that
301+
// blocked callback goroutines do not deadlock.
302+
defer func() {
303+
pendingMu.Lock()
304+
for id, ch := range pending {
305+
close(ch)
306+
delete(pending, id)
307+
}
308+
pendingMu.Unlock()
309+
}()
278310

279311
// 7. proxyHandle was set inside the VM.Do callbacks above (local JNI
280312
// refs are thread-local — must be stored before VM.Do returns).

grpc/server/jni_raw/server.go

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ func (s *Server) requireObject(handle int64) (*jni.Object, error) {
4949
return obj, nil
5050
}
5151

52+
// requireClass retrieves a handle and casts it to *jni.Class.
53+
// The caller is responsible for ensuring the handle refers to a JNI class object.
54+
// Passing a non-class handle causes undefined JVM behavior.
5255
func (s *Server) requireClass(handle int64) (*jni.Class, error) {
5356
obj, err := s.requireObject(handle)
5457
if err != nil {
@@ -1014,7 +1017,9 @@ func (s *Server) GetByteArrayData(_ context.Context, req *pb.GetByteArrayDataReq
10141017
byteArr := (*jni.ByteArray)(unsafe.Pointer(obj))
10151018
length := env.GetArrayLength(arr)
10161019
data = make([]byte, length)
1017-
env.GetByteArrayRegion(byteArr, 0, length, unsafe.Pointer(&data[0]))
1020+
if length > 0 {
1021+
env.GetByteArrayRegion(byteArr, 0, length, unsafe.Pointer(&data[0]))
1022+
}
10181023
return nil
10191024
}); err != nil {
10201025
return nil, status.Errorf(codes.Internal, "%v", err)
@@ -1046,10 +1051,13 @@ func (s *Server) SetField(_ context.Context, req *pb.SetFieldValueRequest) (*pb.
10461051
if err != nil {
10471052
return nil, err
10481053
}
1054+
val := req.GetValue()
1055+
if val == nil || val.GetValue() == nil {
1056+
return nil, status.Errorf(codes.InvalidArgument, "value must not be nil")
1057+
}
10491058
if err := s.withEnv(func(env *jni.Env) error {
10501059
fid := fieldID(req.GetFieldId())
1051-
s.setFieldValue(env, obj, fid, req.GetValue())
1052-
return nil
1060+
return s.setFieldValue(env, obj, fid, val)
10531061
}); err != nil {
10541062
return nil, status.Errorf(codes.Internal, "%v", err)
10551063
}
@@ -1078,10 +1086,13 @@ func (s *Server) SetStaticField(_ context.Context, req *pb.SetStaticFieldValueRe
10781086
if err != nil {
10791087
return nil, err
10801088
}
1089+
val := req.GetValue()
1090+
if val == nil || val.GetValue() == nil {
1091+
return nil, status.Errorf(codes.InvalidArgument, "value must not be nil")
1092+
}
10811093
if err := s.withEnv(func(env *jni.Env) error {
10821094
fid := fieldID(req.GetFieldId())
1083-
s.setStaticFieldValue(env, cls, fid, req.GetValue())
1084-
return nil
1095+
return s.setStaticFieldValue(env, cls, fid, val)
10851096
}); err != nil {
10861097
return nil, status.Errorf(codes.Internal, "%v", err)
10871098
}
@@ -1128,7 +1139,10 @@ func (s *Server) setFieldValue(
11281139
obj *jni.Object,
11291140
fid jni.FieldID,
11301141
val *pb.JValue,
1131-
) {
1142+
) error {
1143+
if val == nil || val.GetValue() == nil {
1144+
return fmt.Errorf("value must not be nil")
1145+
}
11321146
switch v := val.GetValue().(type) {
11331147
case *pb.JValue_Z:
11341148
var b uint8
@@ -1153,6 +1167,7 @@ func (s *Server) setFieldValue(
11531167
case *pb.JValue_L:
11541168
env.SetObjectField(obj, fid, s.getObject(v.L))
11551169
}
1170+
return nil
11561171
}
11571172

11581173
func (s *Server) getStaticFieldValue(
@@ -1195,7 +1210,10 @@ func (s *Server) setStaticFieldValue(
11951210
cls *jni.Class,
11961211
fid jni.FieldID,
11971212
val *pb.JValue,
1198-
) {
1213+
) error {
1214+
if val == nil || val.GetValue() == nil {
1215+
return fmt.Errorf("value must not be nil")
1216+
}
11991217
switch v := val.GetValue().(type) {
12001218
case *pb.JValue_Z:
12011219
var b uint8
@@ -1220,4 +1238,5 @@ func (s *Server) setStaticFieldValue(
12201238
case *pb.JValue_L:
12211239
env.SetStaticObjectField(cls, fid, s.getObject(v.L))
12221240
}
1241+
return nil
12231242
}

grpc/server/jni_raw/void_detector.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,15 @@ func (d *voidDetector) doInit(env *jni.Env) error {
5757
return nil
5858
}
5959

60+
// close releases the JNI global reference held by the voidDetector.
61+
// Must be called inside a VM.Do callback when the detector is no longer needed.
62+
func (d *voidDetector) close(env *jni.Env) {
63+
if d.voidType != nil {
64+
env.DeleteGlobalRef(d.voidType)
65+
d.voidType = nil
66+
}
67+
}
68+
6069
// isVoid returns true when method's return type is java.lang.Void.TYPE.
6170
func (d *voidDetector) isVoid(
6271
env *jni.Env,

handlestore/server.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,13 @@ type Server struct {
1717
}
1818

1919
// ReleaseHandle releases a previously-stored JNI global reference.
20+
// Release is atomic — it removes the handle from the store and deletes
21+
// the JNI global ref in a single locked operation, avoiding TOCTOU races.
2022
func (s *Server) ReleaseHandle(_ context.Context, req *pb.ReleaseHandleRequest) (*pb.ReleaseHandleResponse, error) {
2123
handle := req.GetHandle()
2224
if handle == 0 {
2325
return &pb.ReleaseHandleResponse{}, nil
2426
}
25-
if s.Handles.Get(handle) == nil {
26-
return nil, status.Errorf(codes.NotFound, "handle %d not found", handle)
27-
}
2827
if err := s.VM.Do(func(env *jni.Env) error {
2928
s.Handles.Release(env, handle)
3029
return nil

tools/pkg/grpcgen/client.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ func buildClientMethod(
185185
GoName: goName,
186186
RequestType: reqType,
187187
ResponseType: respType,
188-
HasError: m.Error,
188+
HasError: m.HasError,
189189
HasResult: m.ReturnKind != javagen.ReturnVoid,
190190
GoReturnType: m.GoReturn,
191191
}

tools/pkg/grpcgen/server.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func buildServerMethod(
219219
SpecGoName: m.GoName,
220220
RequestType: reqType,
221221
ResponseType: respType,
222-
HasError: m.Error,
222+
HasError: m.HasError,
223223
HasResult: m.ReturnKind != javagen.ReturnVoid,
224224
GoReturnType: m.GoReturn,
225225
}

tools/pkg/grpcgen/server_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ func TestBuildServerMethod_ObjectNoError(t *testing.T) {
519519
ReturnKind: javagen.ReturnObject,
520520
GoReturn: "*jni.Object",
521521
Returns: "android.some.Unknown",
522-
Error: false,
522+
HasError: false,
523523
}
524524
sm := buildServerMethod(
525525
m,

0 commit comments

Comments
 (0)