From 2fddba63017b1b5db11c0b0aa067ba467bb3da2b Mon Sep 17 00:00:00 2001 From: emanuele-f Date: Sat, 13 Aug 2022 16:10:38 +0200 Subject: [PATCH] Fix possible JNI local reference overflow and races Dumping connections payload requires creating (local) references, which are limited to 512. This commit greatly reduces their lifetime, from several seconds to less than 1 second, reducing the likehood of an overflow. Moreover it adds missing synchronization to the connection payload. --- .../remote_capture/MitmReceiver.java | 2 +- .../model/ConnectionDescriptor.java | 28 +++++++++---------- app/src/main/jni/core/jni_impl.c | 11 +++++++- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/com/emanuelef/remote_capture/MitmReceiver.java b/app/src/main/java/com/emanuelef/remote_capture/MitmReceiver.java index b3ee0aa0..e3d24ca3 100644 --- a/app/src/main/java/com/emanuelef/remote_capture/MitmReceiver.java +++ b/app/src/main/java/com/emanuelef/remote_capture/MitmReceiver.java @@ -306,7 +306,7 @@ public class MitmReceiver implements Runnable, ConnectionsListener, MitmListener if(conn.status == ConnectionDescriptor.CONN_STATUS_CLOSED) conn.status = ConnectionDescriptor.CONN_STATUS_CLIENT_ERROR; } else - conn.addPayloadChunk(new PayloadChunk(message, getChunkType(type), isSent(type), tstamp)); + conn.addPayloadChunkMitm(new PayloadChunk(message, getChunkType(type), isSent(type), tstamp)); } private synchronized void addPendingMessage(PendingMessage pending) { diff --git a/app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java b/app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java index fe5736d3..942008eb 100644 --- a/app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java +++ b/app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java @@ -101,7 +101,7 @@ public class ConnectionDescriptor { public String info; public String url; public String l7proto; - private ArrayList payload_chunks; + private final ArrayList payload_chunks; // must be synchronized public final int uid; public final int ifidx; public final int incr_id; @@ -140,6 +140,7 @@ public class ConnectionDescriptor { l7proto = ""; country = ""; asn = new Geomodel.ASN(); + payload_chunks = new ArrayList<>(); mitm_decrypt = _mitm_decrypt; } @@ -180,8 +181,10 @@ public class ConnectionDescriptor { // Some pending updates with payload may still be received after low memory has been // triggered and payload disabled if(!CaptureService.isLowMemory()) { - payload_chunks = update.payload_chunks; - payload_truncated = update.payload_truncated; + synchronized (this) { + payload_chunks.addAll(update.payload_chunks); + payload_truncated = update.payload_truncated; + } } } } @@ -294,29 +297,24 @@ public class ConnectionDescriptor { public boolean isDecrypted() { return !isNotDecryptable() && (getNumPayloadChunks() > 0); } public boolean isCleartext() { return !encrypted_payload && !encrypted_l7; } - public int getNumPayloadChunks() { return (payload_chunks == null) ? 0 : payload_chunks.size(); } + public synchronized int getNumPayloadChunks() { return payload_chunks.size(); } - public @Nullable PayloadChunk getPayloadChunk(int idx) { + public synchronized @Nullable PayloadChunk getPayloadChunk(int idx) { if(getNumPayloadChunks() <= idx) return null; return payload_chunks.get(idx); } - public void addPayloadChunk(PayloadChunk chunk) { - if(payload_chunks == null) - payload_chunks = new ArrayList<>(); + public synchronized void addPayloadChunkMitm(PayloadChunk chunk) { payload_chunks.add(chunk); payload_length += chunk.payload.length; } - public void dropPayload() { - payload_chunks = null; + public synchronized void dropPayload() { + payload_chunks.clear(); } - private boolean hasHttp(boolean is_sent) { - if(getNumPayloadChunks() == 0) - return false; - + private synchronized boolean hasHttp(boolean is_sent) { for(PayloadChunk chunk: payload_chunks) { if(chunk.is_sent == is_sent) return (chunk.type == PayloadChunk.ChunkType.HTTP); @@ -327,7 +325,7 @@ public class ConnectionDescriptor { public boolean hasHttpRequest() { return hasHttp(true); } public boolean hasHttpResponse() { return hasHttp(false); } - private String getHttp(boolean is_sent) { + private synchronized String getHttp(boolean is_sent) { if(getNumPayloadChunks() == 0) return ""; diff --git a/app/src/main/jni/core/jni_impl.c b/app/src/main/jni/core/jni_impl.c index 42e72735..78981488 100644 --- a/app/src/main/jni/core/jni_impl.c +++ b/app/src/main/jni/core/jni_impl.c @@ -172,9 +172,12 @@ static jobject getConnUpdate(pcapdroid_t *pd, const conn_and_tuple_t *conn) { (*env)->DeleteLocalRef(env, url); (*env)->DeleteLocalRef(env, l7proto); } - if(data->update_type & CONN_UPDATE_PAYLOAD) + if(data->update_type & CONN_UPDATE_PAYLOAD) { (*env)->CallVoidMethod(env, update, mids.connUpdateSetPayload, data->payload_chunks, data->payload_truncated); + (*pd->env)->DeleteLocalRef(pd->env, data->payload_chunks); + data->payload_chunks = NULL; + } // reset the update flag data->update_type = 0; @@ -273,6 +276,8 @@ static int dumpConnectionUpdate(pcapdroid_t *pd, const conn_and_tuple_t *conn, j /* Perform a full dump of the active connections */ static void sendConnectionsDump(pcapdroid_t *pd) { JNIEnv *env = pd->env; + //jniDumpReferences(env); + jobject new_conns = (*env)->NewObjectArray(env, pd->new_conns.cur_items, cls.conn, NULL); jobject conns_updates = (*env)->NewObjectArray(env, pd->conns_updates.cur_items, cls.conn_update, NULL); @@ -311,6 +316,7 @@ static void sendConnectionsDump(pcapdroid_t *pd) { cleanup: (*env)->DeleteLocalRef(env, new_conns); (*env)->DeleteLocalRef(env, conns_updates); + //jniDumpReferences(env); } /* ******************************************************* */ @@ -405,6 +411,9 @@ static bool dumpPayloadChunk(struct pcapdroid *pd, const pkt_context_t *pctx, in bool rv = false; if(pctx->data->payload_chunks == NULL) { + // Directly allocating an ArrayList rather than creating it afterwards saves us from a data copy. + // However, this creates a local reference, which is retained until sendConnectionsDump is called. + // NOTE: Android only allows up to 512 local references. pctx->data->payload_chunks = (*env)->NewObject(env, cls.arraylist, mids.arraylistNew); if((pctx->data->payload_chunks == NULL) || jniCheckException(env)) return false;