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.

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

Don't try to print any information about gQUIC packets. #960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rpaulo
Copy link
Contributor

@rpaulo rpaulo commented Nov 17, 2021

Fixes #959.

@fxlb
Copy link
Member

fxlb commented Nov 20, 2021

Thanks for the patch.
Have you documentation about "magic" value 0x51?
Is a test is needed for the "Short Header" case?

@rpaulo
Copy link
Contributor Author

rpaulo commented Nov 20, 2021

The 0x51 is just the beginning of the Google QUIC version numbers. I don't have any documentation on it.
Short headers have no version number, so nothing is needed there.

@fxlb
Copy link
Member

fxlb commented Nov 22, 2021

Looking for information about 0x51, I looked at:
https://datatracker.ietf.org/doc/html/draft-ietf-quic-transport-32
And found in it the link:
https://github.com/quicwg/base-drafts/wiki/QUIC-Versions
0x51 is not the only non-IETF QUIC version value...

@rpaulo
Copy link
Contributor Author

rpaulo commented Nov 22, 2021

I was told that 0x51 will be the last version before the transition to IETF QUIC is finalized. Regarding the other versions, we should let the authors of them add support to tcpdump if it’s important to them.

@fxlb
Copy link
Member

fxlb commented Nov 24, 2021

To detect only IETF QUIC, the quic_detect function could be enhanced to check, in the long header case, than the version is 0, 1 or 0x?a?a?a?a.

@rpaulo
Copy link
Contributor Author

rpaulo commented Dec 12, 2021

It could but it would make that function unnecessarily complicated. The versions you pointed out were used before RFC 9000 was released.

@fxlb
Copy link
Member

fxlb commented Dec 22, 2021

Yes, but tcpdump needs to behave correctly with any input (gquic, pre-rfc9000, experiments, etc.). And with a fuzzed/corrupted capture file the version may be any random value...
Why not a quic_detect() update like (or better idea?):

diff --git a/print-quic.c b/print-quic.c
index be1a5450..14069638 100644
--- a/print-quic.c
+++ b/print-quic.c
@@ -114,9 +114,23 @@ quic_detect(netdissect_options *ndo, const u_char *p, const u_int len)
 		return 0;
 	first_octet = GET_U_1(p);
 	/* All QUIC packets must have the Fixed Bit set to 1. */
-	if ((first_octet & 0x40) == 0x40)
-		return 1;
-	else
+	if ((first_octet & 0x40) == 0x40) {
+		if (first_octet & 0x80) {
+			/* Long Header (Header Form set to 1) */
+			uint32_t version;
+			if (len < 6)
+				return 0;
+			p++;
+			version = GET_BE_U_4(p);
+			if (version == 0 || version == 1 ||
+			    (version & 0x0f0f0f0f) == 0x0a0a0a0a)
+				return 1;
+			else
+				return 0;
+		} else
+			/* Short Header */
+			return 1;
+	} else
 		return 0;
 }
 

@rpaulo
Copy link
Contributor Author

rpaulo commented Dec 22, 2021

I think that you can do that. However, by time tcpdump 5 ships, I suspect gQUIC will not be enabled anymore, but who knows.
I do think that quic_detect() should be as simple as possible. If we were to add full support for proprietary protocols like gQUIC then it would have to be different, but right now, I don't see much benefit in spending time on anything else but IETF QUIC.

@fxlb
Copy link
Member

fxlb commented Jan 12, 2022

I do think that quic_detect() should be as simple as possible.

Why not. But in this case it seems better to remove the 0x51 version test (particular case) and test version like:
version == 0 || version == 1 || (version & 0x0f0f0f0f) == 0x0a0a0a0a)
All other versions should give : "not RFC9000 packet"

@rpaulo
Copy link
Contributor Author

rpaulo commented Jan 12, 2022

Not all QUIC packets carry a version number so that would only work for LH packets.

@fxlb
Copy link
Member

fxlb commented Jan 13, 2022 via email

@rpaulo
Copy link
Contributor Author

rpaulo commented Jan 13, 2022

Okay, your patch works for me. Feel free to commit that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Question about quic printer
2 participants