Telegram for Android: Use-after-free in Connection::onReceivedData

Reporting this in collaboration with Mitch, who found crashes indicating this issue and brought it to my attention.

Related to https://bugs.chromium.org/p/project-zero/issues/detail?id=2505; there's another issue which I overlooked when reviewing the fix. If we look more closely at `Connection::onReceivedData`, there's another possibility that can invalidate `buffer`.

```
void Connection::onReceivedData(NativeByteBuffer *buffer) {
  AES_ctr128_encrypt(buffer->bytes(), buffer->bytes(), buffer->limit(), &decryptKey, decryptIv, decryptCount, &decryptNum);
  
  // snip...

  NativeByteBuffer *reuseLater = nullptr;
  while (buffer->hasRemaining()) {
    // snip...

    uint32_t old = buffer->limit();
    buffer->limit(buffer->position() + currentPacketLength);

    ConnectionsManager::getInstance(currentDatacenter->instanceNum).onConnectionDataReceived(this, buffer, currentPacketLength);
    buffer->position(buffer->limit());  // <-- Use here
    buffer->limit(old);

    // snip...
```

We can see the call to `ConnectionsManager::onConnectionDataReceived`, and it should be the case that buffer etc. remain valid past this call. If we look at the implementation, we can see that (at least in the case where we aren't using a proxy) we call `connection->reconnect()`

```
void ConnectionsManager::onConnectionDataReceived(Connection *connection, NativeByteBuffer *data, uint32_t length) {
  bool error = false;
  if (length <= 24 + 32) {
    int32_t code = data->readInt32(&error);
    if (code == 0) {
      // snip...
    } else {
      Datacenter *datacenter = connection->getDatacenter();
      if (LOGS_ENABLED) DEBUG_W(\"mtproto error = %d\", code);
      if (code == -444 && connection->getConnectionType() == ConnectionTypeGeneric && !proxyAddress.empty() && !proxySecret.empty()) {
        // snip...
      } else if (code == -404 && (datacenter->isCdnDatacenter || PFS_ENABLED)) {
        // snip...
      } else {
        connection->reconnect();
      }
    }
    return;
  }

  // snip...
```

`Connection::reconnect` calls down into `Connection::suspendConnection`, which releases `restOfTheData`, which may alias `buffer`.

```
void Connection::suspendConnection(bool idle) {
  reconnectTimer->stop();
  waitForReconnectTimer = false;
  if (connectionState == TcpConnectionStageIdle || connectionState == TcpConnectionStageSuspended) {
    return;
  }
  if (LOGS_ENABLED) DEBUG_D(\"connection(%p, account%u, dc%u, type %d) suspend\", this, currentDatacenter->instanceNum, currentDatacenter->getDatacenterId(), connectionType);
  connectionState = idle ? TcpConnectionStageIdle : TcpConnectionStageSuspended;
  dropConnection();
  ConnectionsManager::getInstance(currentDatacenter->instanceNum).onConnectionClosed(this, 0);
  firstPacketSent = false;
  if (restOfTheData != nullptr) {
    restOfTheData->reuse();  // <-- Free here
    restOfTheData = nullptr;
  }
  lastPacketLength = 0;
  connectionToken = 0;
  wasConnected = false;
}

This will then lead to a use-after-free on buffer after returning from `ConnectionsManager::onConnectionDataReceived`.

Crashes with a matching stack trace have been observed, so it is definitely possible for this to occur - since the simplest case requires that the client be connected directly to the datacenter instead of through a proxy, I haven't attempted to reproduce the issue yet. 

It is however possible to reach `connection->reconnect()` in multiple ways inside `ConnectionsManager::onConnectionDataReceived`, so it's very likely that this is also reachable in a similar way to issue 2505.

This bug is subject to a 90-day disclosure deadline. If a fix for this
issue is made available to users before the end of the 90-day deadline,
this bug report will become public 30 days after the fix was made
available. Otherwise, this bug report will become public at the deadline.
The scheduled deadline is 2024-07-23.





Found by: markbrand@google.com