Remove non-functional SSL 2 version code point #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.

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

All modified and coverable lines are covered by tests ✅

Project coverage is 83.11%. Comparing base (3292f14) to head (0c48205).

Additional details and impacted files

@@           Coverage Diff            @@
##              dev    #1694    +/-   ##
========================================
  Coverage   83.11%   83.11%            
========================================
  Files         279      279            
  Lines       48399    48396     -3     
  Branches    10237    10500   +263     
========================================
- Hits        40227    40225     -2     
- Misses       7035     7048    +13     
+ Partials     1137     1123    -14

Flag	Coverage Δ
alpine320	`75.09% <ø> (+<0.01%)`	⬆️
fedora40	`75.13% <ø> (+0.03%)`	⬆️
macos-13	`80.60% <ø> (+<0.01%)`	⬆️
macos-14	`80.60% <ø> (+<0.01%)`	⬆️
macos-15	`80.57% <ø> (+<0.01%)`	⬆️
mingw32	`70.81% <ø> (-0.03%)`	⬇️
mingw64	`70.80% <ø> (-0.01%)`	⬇️
npcap	`85.22% <ø> (-0.01%)`	⬇️
rhel94	`74.96% <ø> (+<0.01%)`	⬆️
ubuntu2004	`58.59% <ø> (+<0.01%)`	⬆️
ubuntu2004-zstd	`58.71% <ø> (+<0.01%)`	⬆️
ubuntu2204	`74.87% <ø> (+<0.01%)`	⬆️
ubuntu2204-icpx	`61.28% <ø> (+<0.01%)`	⬆️
ubuntu2404	`75.12% <ø> (+<0.01%)`	⬆️
unittest	`83.11% <ø> (+<0.01%)`	⬆️
windows-2019	`85.25% <ø> (-0.01%)`	⬇️
windows-2022	`85.28% <ø> (-0.01%)`	⬇️
winpcap	`85.24% <ø> (+<0.01%)`	⬆️
xdp	`50.40% <ø> (+<0.01%)`	⬆️

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.

Packet++/header/SSLCommon.h

Tests/Packet++Test/main.cpp

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.

@droe I think we can remove SSL2 from the enum for now, and later consider whether to implement a full parser or not

Packet++/header/SSLCommon.h

As per discussion in seladb#1694, remove SSL 2 for now. SSL 2 is not actually implemented, and the version code point is wrong anyway. 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]. More importantly, SSL 2 has a different record format, without version field, that is used for both SSL 2 proper, and SSL 2 compatible SSL 3 / TLS. For Packet++ to see a SSL 2 version field on the wire, it would first have to support the SSL 2 record format, and at least one of SSL 2 handshake messages, or SSL 2 compatible SSL 3 or later handshakes. [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 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

Packet++/header/SSLCommon.h Outdated Show resolved Hide resolved

Packet++/header/SSLCommon.h Show resolved Hide resolved

seladb reviewed Feb 2, 2025

View reviewed changes

tigercosmos requested changes Feb 3, 2025

View reviewed changes

Packet++/header/SSLCommon.h Outdated Show resolved Hide resolved

droe force-pushed the droe/fix-ssl2 branch from 37014ef to 638a5a5 Compare February 9, 2025 12:55

droe force-pushed the droe/fix-ssl2 branch from 638a5a5 to 48c82e1 Compare February 9, 2025 13:25

droe changed the title ~~Fix SSL 2 version constant to 0x0002~~ Remove non-functional SSL 2 version code point Feb 9, 2025

droe force-pushed the droe/fix-ssl2 branch from 48c82e1 to 0c48205 Compare February 9, 2025 13:30

droe requested review from tigercosmos and seladb February 9, 2025 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non-functional SSL 2 version code point #1694

Remove non-functional SSL 2 version code point #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 Feb 2, 2025

droe Feb 2, 2025

seladb Feb 3, 2025

droe Feb 3, 2025 •

edited

Loading

seladb Feb 8, 2025


		pcpp::Packet clientHelloPacket(&rawPacket1);

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

Remove non-functional SSL 2 version code point #1694

Are you sure you want to change the base?

Remove non-functional SSL 2 version code point #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

seladb Feb 2, 2025

Choose a reason for hiding this comment

droe Feb 2, 2025

Choose a reason for hiding this comment

seladb Feb 3, 2025

Choose a reason for hiding this comment

droe Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

seladb Feb 8, 2025

Choose a reason for hiding this comment

Dimi1010 commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

droe Feb 3, 2025 •

edited

Loading