Fix SSL 2 version constant to 0x0002 #1694

droe · 2025-01-19T16:19:41Z

SSL 2 uses a version field of 0x0002, not 0x0200. This is confirmed not only in the original Netscape spec [1] and RFC draft of the time [2], but also in major implementations such as OpenSSL [3] and Wireshark [4].

[1] https://www-archive.mozilla.org/projects/security/pki/nss/ssl/draft02.html
[2] https://datatracker.ietf.org/doc/html/draft-hickman-netscape-ssl-00
[3] https://github.com/openssl/openssl/blob/OpenSSL_0_9_6m/ssl/ssl2.h#L66-L71
[4] https://github.com/wireshark/wireshark/blob/release-4.4/epan/dissectors/packet-tls-utils.h#L266-L277

tigercosmos · 2025-01-19T17:20:27Z

It's just bytes order. PCPP here is using host order, and other libraries that you posted are using network order.

droe · 2025-01-19T17:24:25Z

If it was byte order, as you suggest, then the constants for SSL 3 and TLS would be wrong.

SSL 2 uses a version field of 0x0002, not 0x0200. This is confirmed not only in the original Netscape spec [1] and RFC draft of the time [2], but also in major implementations such as OpenSSL [3] and Wireshark [4]. [1] https://www-archive.mozilla.org/projects/security/pki/nss/ssl/draft02.html [2] https://datatracker.ietf.org/doc/html/draft-hickman-netscape-ssl-00 [3] https://github.com/openssl/openssl/blob/OpenSSL_0_9_6m/ssl/ssl2.h#L66-L71 [4] https://github.com/wireshark/wireshark/blob/release-4.4/epan/dissectors/packet-tls-utils.h#L266-L277

droe · 2025-01-19T17:53:09Z

I missed code in Packet++/src/SSLCommon.cpp that used 0x200 in the first attempt, second attempt also addresses that.

Dimi1010 · 2025-01-19T18:13:51Z

Closing and reopening to run CI after base branch change.

codecov · 2025-01-19T18:28:52Z

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.12%. Comparing base (bc5c08d) to head (37014ef).
Report is 9 commits behind head on dev.

Files with missing lines	Patch %	Lines
Tests/Packet++Test/Tests/SSLTests.cpp	76.47%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1694      +/-   ##
==========================================
- Coverage   83.15%   83.12%   -0.03%     
==========================================
  Files         277      279       +2     
  Lines       48185    48433     +248     
  Branches    10165    10315     +150     
==========================================
+ Hits        40067    40261     +194     
+ Misses       7237     7047     -190     
- Partials      881     1125     +244

Flag	Coverage Δ
alpine320	`75.10% <73.33%> (-0.07%)`	⬇️
fedora40	`75.16% <73.33%> (-0.03%)`	⬇️
macos-13	`80.61% <78.37%> (-0.04%)`	⬇️
macos-14	`80.61% <78.37%> (-0.04%)`	⬇️
macos-15	`80.59% <78.37%> (-0.03%)`	⬇️
mingw32	`70.80% <100.00%> (-0.12%)`	⬇️
mingw64	`70.76% <100.00%> (-0.12%)`	⬇️
npcap	`85.15% <100.00%> (-0.16%)`	⬇️
rhel94	`74.99% <73.33%> (-0.06%)`	⬇️
ubuntu2004	`58.59% <66.66%> (-0.01%)`	⬇️
ubuntu2004-zstd	`58.71% <66.66%> (-0.01%)`	⬇️
ubuntu2204	`74.90% <73.33%> (-0.04%)`	⬇️
ubuntu2204-icpx	`61.28% <100.00%> (-0.11%)`	⬇️
ubuntu2404	`75.16% <73.33%> (-0.06%)`	⬇️
unittest	`83.12% <78.37%> (-0.03%)`	⬇️
windows-2019	`85.25% <100.00%> (-0.09%)`	⬇️
windows-2022	`85.28% <100.00%> (-0.09%)`	⬇️
winpcap	`85.24% <100.00%> (-0.08%)`	⬇️
xdp	`50.43% <73.33%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tigercosmos · 2025-01-20T00:09:13Z

@droe So is this still an issue?

droe · 2025-01-20T00:17:55Z

Yes, certainly. Code point 0x0200 is wrong, 0x0002 as proposed in this PR is correct.

tigercosmos · 2025-01-20T07:29:31Z

@droe In Wireshark the values are:

#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 in PCPP, the current (master) values are:

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

According to the byte order, I think SSL2 should be 0x0020, not 0x0002?

droe · 2025-01-20T07:47:15Z

It should be 0x0002 for SSL 2. This is not a byte order problem (and you're swapping nibbles, not bytes).

tigercosmos · 2025-01-20T08:03:13Z

I meant: major is 0x00, minor is 0x02. If you swap the bytes, 0x02 can be 0x2.

droe · 2025-01-20T09:12:17Z

Not sure I understand what you're saying, uint8_t 0x02 and 0x2 are the same thing. You are running the version uint16_t from the wire through be16toh() at deserialization time, so the code ends up seeing SSLVersion 0x0002 in host byte order for the SSL 2 version bytes 00 02 on the wire.

seladb · 2025-01-23T05:45:38Z

Packet++/header/SSLCommon.h

@@ -117,7 +117,7 @@ namespace pcpp
 		enum SSLVersionEnum
 		{
 			/** SSL 2.0 */
-			SSL2 = 0x0200,
+			SSL2 = 0x0002,


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

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

@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.

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.

@seladb could you help double check?

seladb · 2025-01-23T05:48:18Z

Packet++/header/SSLCommon.h

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...

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.)

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

Fair request, I'll look into it.

SSL2RecordSSL2ClientHelloTest : PASSED SSL2RecordTLS1ClientHelloTest : PASSED

seladb · 2025-02-02T00:51:24Z

Tests/Packet++Test/main.cpp

+	PTF_RUN_TEST(SSL2RecordSSL2ClientHelloTest, "ssl;ssl2");
+	PTF_RUN_TEST(SSL2RecordTLS1ClientHelloTest, "ssl;ssl2");


nit: maybe make the names shorter?

SSL2ClientHelloTest TLS1ClientHelloTest

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).

Tests/Packet++Test/PacketExamples/ssl2rec-ssl2clienthello.dat

seladb · 2025-02-02T00:58:24Z

Tests/Packet++Test/Tests/SSLTests.cpp

+
+	pcpp::Packet clientHelloPacket(&rawPacket1);
+
+	// PCPP does not know how to parse SSLv2 yet, so we find the version field manually.


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? 🤔

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.

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?

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.

tigercosmos · 2025-02-03T00:46:39Z

Packet++/header/SSLCommon.h

@@ -117,7 +117,7 @@ namespace pcpp
 		enum SSLVersionEnum
 		{
 			/** SSL 2.0 */
-			SSL2 = 0x0200,
+			SSL2 = 0x0002,


@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.

droe requested a review from seladb as a code owner January 19, 2025 16:19

droe force-pushed the droe/fix-ssl2 branch from 02624e5 to 426070b Compare January 19, 2025 17:49

Dimi1010 changed the base branch from master to dev January 19, 2025 18:12

Dimi1010 closed this Jan 19, 2025

Dimi1010 reopened this Jan 19, 2025

seladb reviewed Jan 23, 2025

View reviewed changes

droe added 2 commits January 24, 2025 00:09

Add SSLv2 ClientHello version field tests for SSLv2 and TLSv1

88c6ab9

Run changes in feature branch through clang-format

69a8956

seladb reviewed Feb 2, 2025

View reviewed changes

Add packet captures for SSL 2, and SSL 2 compatible TLS 1

37014ef

tigercosmos requested changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSL 2 version constant to 0x0002 #1694

Fix SSL 2 version constant to 0x0002 #1694

droe commented Jan 19, 2025

tigercosmos commented Jan 19, 2025

droe commented Jan 19, 2025

droe commented Jan 19, 2025

Dimi1010 commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

seladb Jan 23, 2025

tigercosmos Jan 23, 2025

tigercosmos Feb 3, 2025

droe Feb 3, 2025

tigercosmos Feb 3, 2025

seladb Jan 23, 2025

droe Jan 23, 2025 •

edited

Loading

seladb Jan 23, 2025

droe Jan 23, 2025

droe Jan 23, 2025

seladb Feb 2, 2025

droe Feb 2, 2025

seladb Feb 2, 2025

droe Feb 2, 2025

seladb Feb 3, 2025

droe Feb 3, 2025 •

edited

Loading

tigercosmos Feb 3, 2025

		PTF_RUN_TEST(SSL2RecordSSL2ClientHelloTest, "ssl;ssl2");
		PTF_RUN_TEST(SSL2RecordTLS1ClientHelloTest, "ssl;ssl2");


		pcpp::Packet clientHelloPacket(&rawPacket1);

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

Fix SSL 2 version constant to 0x0002 #1694

Are you sure you want to change the base?

Fix SSL 2 version constant to 0x0002 #1694

Conversation

droe commented Jan 19, 2025

tigercosmos commented Jan 19, 2025

droe commented Jan 19, 2025

droe commented Jan 19, 2025

Dimi1010 commented Jan 19, 2025 • edited Loading

codecov bot commented Jan 19, 2025 • edited Loading

Codecov Report

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

tigercosmos commented Jan 20, 2025

droe commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droe Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droe Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

droe Jan 23, 2025 •

edited

Loading

droe Feb 3, 2025 •

edited

Loading