Detecting an SSL/TLS Client Handshake

[Elkvis]

After a bit of research, I've come up with this little function to determine if a client is attempting to negotiate an SSL/TLS connection to my server, before I actually try to hand off the socket to OpenSSL. This function returns true if SSL/TLS detected, otherwise it returns false.

bool DetectSslTlsHandshake(int socket)
{
  unsigned char buffer[6];

  // Sniff the first few bytes from the client to see if it might be SSL/TLS.
  // Use the MSG_PEEK flag so as not to remove them from the buffer.
  int bytes = recv(socket, buffer, sizeof(buffer), MSG_PEEK);

  if (bytes >= 3)
  {
    if (buffer[0] & 0x80) // SSLv2 maybe
    {
      int length = (int(buffer[0] & 0x7f) << 8) + buffer[1];
      if (length > 9) // likely SSLv2
      {
        if (buffer[2] == 0x01) // definitely SSLv2
        {
          std::cout << "SSL v2.0 Handshake detected" << std::endl;
          return true;
        }
      }
    }
    else if (bytes >= 6 && buffer[0] == 0x16) // SSLv3/TLSv1.x maybe
    {
      if (buffer[1] == 3) // very likely SSLv3/TLSv1.x handshake
      {
        if (buffer[2]) // TLS v1.x
        {
          std::cout << "TLS v1.x Handshake detected" << std::endl;
        }
        else // SSL v3.0
        {
          std::cout << "SSL v3.0 Handshake detected" << std::endl;
        }
        return true;
      }
    }
  }

  return false;
}

I'd like some input on ways to improve it, if any, particularly if there are any security problems lurking in this code. Ideally, I'd like to be able to determine which version of TLS is being negotiated, but buffer[2] is always 1, regardless of which version of TLS is incoming. If this is not something I can get from the initial packet, that's fine, but it would be a "nice to have" feature.

[Salem]

Your recv() is wrong.

The only guarantee for a stream protocol is that the bytes are received in the order that they're sent.
Any sense of "packet" is lost in transmission. So even if the sender sent 3 bytes in a single send() call, this does not imply that the corresponding recv() will always and exactly receive those 3 bytes.

unsigned char *p = buffer;
size_t remainder = sizeof(buffer);
do {
  n = recv( socket, p, remainder, MSG_PEEK);
  if ( n > 0 ) {
    // success of some sort - progress
    p += n;
    remainder -= n;
  } else if ( n == 0 ) {
    // remote disconnected
  } else {
    // error.  Typically exit unless errno is EAGAIN and the
    // socket is non-blocking
  }
} while ( remainder > 0 );

It also means you can't do tests like
if (bytes >= 3)
You have to inspect the data.

[Elkvis]

Your recv() is wrong.

The only guarantee for a stream protocol is that the bytes are received in the order that they're sent.
Any sense of "packet" is lost in transmission. So even if the sender sent 3 bytes in a single send() call, this does not imply that the corresponding recv() will always and exactly receive those 3 bytes.

unsigned char *p = buffer;
size_t remainder = sizeof(buffer);
do {
  n = recv( socket, p, remainder, MSG_PEEK);
  if ( n > 0 ) {
    // success of some sort - progress
    p += n;
    remainder -= n;
  } else if ( n == 0 ) {
    // remote disconnected
  } else {
    // error.  Typically exit unless errno is EAGAIN and the
    // socket is non-blocking
  }
} while ( remainder > 0 );

In the vast majority of cases, it will work as written, and I don't really care about the edge cases, because this function is being used purely for informational purposes, and no decisions are made based on its return value.

It also means you can't do tests like
if (bytes >= 3)
You have to inspect the data.

Does the code that follows that line somehow not qualify as "inspecting the data?" Perhaps you could explain what you mean in greater detail.

[Salem]

> Does the code that follows that line somehow not qualify as "inspecting the data?"
Well you're relying on the recv() performing as expected.
You're assuming that recv() will receive the right number of bytes for the right protocol choice.


> In the vast majority of cases, it will work as written, and I don't really care about the edge cases,
This kind of sloppy thinking is why gets() existed in the API for 30 years.

I guess your initial request was meaningless then, if you've no interest in the answer.

I'd like some input on ways to improve it, if any, particularly if there are any security problems lurking in this code.

Security (or the lack of it) is ALWAYS about edge cases.

Doing it right all the time means you're more likely to do it right when it really matters.

[Elkvis]

Security (or the lack of it) is ALWAYS about edge cases.

Doing it right all the time means you're more likely to do it right when it really matters.

Then maybe you can explain how this code is insecure.

I never go past the end of my buffer on a read or write.
I never inspect more data than I know I've received,
I disregard the data altogether if I don't receive enough to analyze.