Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SSL 2 version constant to 0x0002 #1694

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Fix SSL 2 version constant to 0x0002 #1694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
426070b
Fix SSL 2 version constant to 0x0002
droe Jan 19, 2025
88c6ab9
Add SSLv2 ClientHello version field tests for SSLv2 and TLSv1
droe Jan 23, 2025
69a8956
Run changes in feature branch through clang-format
droe Jan 29, 2025
37014ef
Add packet captures for SSL 2, and SSL 2 compatible TLS 1
droe Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Packet++/header/SSLCommon.h
View file
Open in desktop
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you happen to have a SSLv2 pcap that we can use in tests? If so, it'd be great to add a test for it!

When I wrote this code I couldn't find a pcap file...

Copy link
Author

@droe droe Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Lekensteyn/wireshark-notes/blob/master/ssl3/sslv2.pcapng

I don't think you currently support SSL 2 record format or SSL 2 compatible SSL 3 handshakes (as per RFC 6101 Appendix E); if you or a user of PCPP were to support the latter, you will have to handle either "0x00 0x02" or "0x03 0x00" / "0x03 0x01" / ... in the handshake version field. Therefore I do believe the best and most correct and useful value for the constant in the enum would be 0x0002. (Also note that SSL 2.0 is called "SSL 0.2" in the old Netscape spec.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks! Would you consider adding a test with a single SSLv2 packet from this file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair request, I'll look into it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

SSL2RecordSSL2ClientHelloTest      : PASSED
SSL2RecordTLS1ClientHelloTest      : PASSED

Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ namespace pcpp
enum SSLVersionEnum
{
/** SSL 2.0 */
SSL2 = 0x0200,
SSL2 = 0x0002,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't change the byte order here because it'll be different from the rest of the values in this enum

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seladb Actually I think 0x0200 is wrong, and it should be 0x0020 based on other enum value byte order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@droe Please change the value to 0x0020.

Let take a look a Wireshark code:

#define SSLV2_VERSION           0x0002 /* not in record layer, SSL_CLIENT_SERVER from
                                          http://www-archive.mozilla.org/projects/security/pki/nss/ssl/draft02.html */
#define SSLV3_VERSION          0x300
#define TLSV1_VERSION          0x301

and let's look at PCPP code:

			/** SSL 3.0 */
			SSL3 = 0x0300,
			/** TLS 1.0 */
			TLS1_0 = 0x0301,

It's obvious that SSL2 should be 0x0020 instead of 0x0002.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that does not make any sense. You seem to be confused about some key aspect of this. If you explain step by step how you determined the (wrong) value 0x0020, I might be able to tell what exactly you are confused about.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seladb could you help double check?

/** SSL 3.0 */
SSL3 = 0x0300,
/** TLS 1.0 */
Expand Down
5 changes: 1 addition & 4 deletions Packet++/src/SSLCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace pcpp

SSLVersion::SSLVersionEnum SSLVersion::asEnum(bool countTlsDraftsAs1_3)
{
if (m_SSLVersionValue >= 0x0300 && m_SSLVersionValue <= 0x0304)
if ((m_SSLVersionValue >= 0x0300 && m_SSLVersionValue <= 0x0304) || m_SSLVersionValue == 0x0002)
return static_cast<SSLVersion::SSLVersionEnum>(m_SSLVersionValue);

if ((m_SSLVersionValue >= 0x7f0e && m_SSLVersionValue <= 0x7f1c) || m_SSLVersionValue == 0xfb17 ||
Expand All @@ -23,9 +23,6 @@ namespace pcpp
return static_cast<SSLVersion::SSLVersionEnum>(m_SSLVersionValue);
}

if (m_SSLVersionValue == 0x200)
return SSLVersion::SSL2;

return SSLVersion::Unknown;
}

Expand Down
1 change: 1 addition & 0 deletions Tests/Packet++Test/PacketExamples/ssl2rec-ssl2clienthello.dat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
000000000000000000000000080045000058b183400040068b1a7f0000017f0000019136115101fe87084a5a8ace80180156fe4c00000101080a27f4368827f4368680220100020009000000100500800300800700c07a16c3dc4a8bcc5b161d1d10883e225e
Binary file added Tests/Packet++Test/PacketExamples/ssl2rec-ssl2clienthello.pcapng
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions Tests/Packet++Test/PacketExamples/ssl2rec-tls1clienthello.dat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
00005e0001670010a49159eb0800450000914a4d40008006344b0a9662910a9d040b067c01bb0440cd534be5ea345018ffff677b00008067010301004e000000100100800300800700c006004002008004008000003900003800003500003300003200000400000500002f00001600001300feff00000a00001500001200fefe0000090000640000620000030000063143c465d52771e466a42be1701787fd
Binary file added Tests/Packet++Test/PacketExamples/ssl2rec-tls1clienthello.pcap
View file
Open in desktop
Binary file not shown.
2 changes: 2 additions & 0 deletions Tests/Packet++Test/TestDefinition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ PTF_TEST_CASE(TLS1_3ParsingTest);
PTF_TEST_CASE(TLSCipherSuiteTest);
PTF_TEST_CASE(ClientHelloTLSFingerprintTest);
PTF_TEST_CASE(ServerHelloTLSFingerprintTest);
PTF_TEST_CASE(SSL2RecordSSL2ClientHelloTest);
PTF_TEST_CASE(SSL2RecordTLS1ClientHelloTest);

// Implemented in IgmpTests.cpp
PTF_TEST_CASE(IgmpParsingTest);
Expand Down
67 changes: 67 additions & 0 deletions Tests/Packet++Test/Tests/SSLTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "../Utils/TestUtils.h"
#include "EndianPortable.h"
#include "Packet.h"
#include "PayloadLayer.h"
#include "SSLLayer.h"
#include "SystemUtils.h"
#include <fstream>
Expand Down Expand Up @@ -753,3 +754,69 @@ PTF_TEST_CASE(ServerHelloTLSFingerprintTest)
PTF_ASSERT_EQUAL(tlsFingerprint.toString(), "771,49195,23-65281-11-35-16");
PTF_ASSERT_EQUAL(tlsFingerprint.toMD5(), "eca9b8f0f3eae50309eaf901cb822d9b");
} // ServerHelloTLSFingerprintTest

static uint16_t getSSL2ClientHelloVersion(uint8_t* data, size_t size)
{
if (size < 2)
{
return 0;
}

if ((data[0] & 0x80) == 0)
{
// Record has padding, three-byte record header.
// Either data record or security escape.
return 0;
}
// Record has no padding, two-byte record header.
// Neither data record nor security escape.

size_t const rec_header_length = 2;
size_t const clienthello_header_length = 9;
size_t reclen = ((data[0] & 0x7f) << 8) | data[1];
if (size != rec_header_length + reclen || reclen < clienthello_header_length)
{
return 0;
}

size_t pos = rec_header_length;
uint8_t const SSL_MT_CLIENT_HELLO = 1;
if (data[pos] != SSL_MT_CLIENT_HELLO)
{
return 0;
}
pos++;
return be16toh(*(uint16_t*)&data[pos]);
}

PTF_TEST_CASE(SSL2RecordSSL2ClientHelloTest)
{
timeval time;
gettimeofday(&time, nullptr);

READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/ssl2rec-ssl2clienthello.dat");

pcpp::Packet clientHelloPacket(&rawPacket1);

// PCPP does not know how to parse SSLv2 yet, so we find the version field manually.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably miss something here - the change you made in SSLCommon should cover parsing of SSLv2, am I wrong?

Why do we need to parse the packet manually then? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of what is says in the comment: PCPP does not know how to parse SSL 2 record format, nor SSL 2 handshake messages. While SSL 3 developed from SSL 2 and has similar concepts and a similar structure, it is more useful to think of it as a different protocol. SSL 2 support is not just a matter of looking at the version field in SSL 3 and later. SSL 2 uses a different record format (that has no version field!), different protocol layering, and handshake messages look different. For instance, cipher suite code points are three bytes, not two bytes. Telling the difference between SSL 2 and SSL 3 without context is not straightforward and takes heuristics.

The point of this PR is only to fix the objectively wrong SSL 2 version code point from 0x0200 to the only code point you'll ever see on the wire for SSL 2: 0x0002.

To bring full SSL 2 support to PCPP, much more work would be needed.

I've included the tests that you asked for only to settle the "endianness" misconceptions by demonstrating that with SSL 2 compatible SSL 3 / TLS handshakes (as per RFC 6101 Appendix E), you will see 0x0002 for an SSL 2 ClientHello transported over an SSL 2 record, and 0x0301 for an TLS 1 ClientHello transported over an SSL 2 record.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize SSLv2 is so different from SSLv3 (aka TLS). Maybe the solution is to actually remove SSLv2 from the enum if we don't actually support it?

I assume no one complained so far because they didn't use SSLv2, so even though it's a breaking change it's not really breaking anything because things are already broken... 😕

@droe do you think it'd be worth the effort to implement a separate SSLv2 parser?

Copy link
Author

@droe droe Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur, removing SSL2 from the enum would also be a sensible fix. Happy to update the branch accordingly if you prefer to go down that route.

I don't have an opinion on whether it's worth implementing full support for SSL 2 and/or SSL 2 compatible SSL 3 / TLS 1.x in PCPP. For most use of PCPP, it's probably irrelevant. For some niche use cases, it might be useful. I'm not sure if it can be done without API-breaking changes.

I'm not going to be contributing full support for SSL 2 or SSL 2 compatible SSL 3 / TLS 1.x. My mission is just to shepherd this change in order to fix or remove the wrong code point for SSL 2.

pcpp::PayloadLayer* payloadLayer = clientHelloPacket.getLayerOfType<pcpp::PayloadLayer>();
PTF_ASSERT_NOT_NULL(payloadLayer);
uint16_t version = getSSL2ClientHelloVersion(payloadLayer->getPayload(), payloadLayer->getPayloadLen());
PTF_ASSERT_EQUAL(version, pcpp::SSLVersion::SSL2)
} // SSL2RecordSSL2ClientHelloTest

PTF_TEST_CASE(SSL2RecordTLS1ClientHelloTest)
{
timeval time;
gettimeofday(&time, nullptr);

READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/ssl2rec-tls1clienthello.dat");

pcpp::Packet clientHelloPacket(&rawPacket1);

// PCPP does not know how to parse SSLv2 yet, so we find the version field manually.
pcpp::PayloadLayer* payloadLayer = clientHelloPacket.getLayerOfType<pcpp::PayloadLayer>();
PTF_ASSERT_NOT_NULL(payloadLayer);
uint16_t version = getSSL2ClientHelloVersion(payloadLayer->getPayload(), payloadLayer->getPayloadLen());
PTF_ASSERT_EQUAL(version, pcpp::SSLVersion::TLS1_0)
} // SSL2RecordTLS1ClientHelloTest
2 changes: 2 additions & 0 deletions Tests/Packet++Test/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ int main(int argc, char* argv[])
PTF_RUN_TEST(TLSCipherSuiteTest, "ssl");
PTF_RUN_TEST(ClientHelloTLSFingerprintTest, "ssl");
PTF_RUN_TEST(ServerHelloTLSFingerprintTest, "ssl");
PTF_RUN_TEST(SSL2RecordSSL2ClientHelloTest, "ssl;ssl2");
PTF_RUN_TEST(SSL2RecordTLS1ClientHelloTest, "ssl;ssl2");
Comment on lines +213 to +214
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe make the names shorter?

SSL2ClientHelloTest
TLS1ClientHelloTest

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would remove very important information: These are a SSL 2 ClientHello in an SSL 2 record, and TLS 1 ClientHello in an SSL 2 record, respectively. The "SSL 2 record" bit is crucial because the wire format is different. TLS 1 handshake in SSL 2 record is different from a regular TLS 1 handshake in a TLS 1 record.

As of now, PCPP knows how to parse record format for SSL 3 through TLS 1.3, but not SSL 2 (neither plain SSL 2 nor SSL 2 compatible SSL 3 and later).


PTF_RUN_TEST(SllPacketParsingTest, "sll");
PTF_RUN_TEST(SllPacketCreationTest, "sll");
Expand Down
Loading