-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Quic ietf 4967 v3 #7095
Quic ietf 4967 v3 #7095
Conversation
Ticket: 4967 The format of initial packet for quic ietf, ie quic v1, is described in rfc 9000, section 17.2.2
so that we can use new functions in quic parser
and logs interesting extensions from crypto frame
As it can be 4, but it can also be 1, based on the first decrypted byte
The way to determine if the payload is encrypted is by storing in the state if we have seen a crypto frame in both directions...
So as to keep parse not too big
for detection
Ticket: 5143
Codecov Report
@@ Coverage Diff @@
## master #7095 +/- ##
==========================================
- Coverage 75.54% 75.39% -0.15%
==========================================
Files 628 629 +1
Lines 185325 185363 +38
==========================================
- Hits 139996 139761 -235
- Misses 45329 45602 +273
Flags with carried forward coverage won't be shown. Click here to find out more. |
WARNING:
Pipeline 6451 |
pub(crate) struct Crypto { | ||
pub ciphers: Vec<TlsCipherSuiteID>, | ||
pub extv: Vec<QuicTlsExtension>, | ||
//does not work, because of lifetime due to references to the slice used for parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this comment move to extv
just above and explain why it is like this instead of using Vec<TlsExtension>
? Comments about why code that is in use are like they are, are more useful than comments explaining commented out things, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, mostly code tidiness suggestions. Will also give this a run on my own quic traffic.
_ => {} | ||
} | ||
let mut values = Vec::new(); | ||
match e { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be broken out into a util func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonish is there a better rustier way to build a string and write integer into it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do
let mut s = String::new();
s.push_str(&my_u16.to_string());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Jason, looks better indeed
} | ||
|
||
// get interesting stuff out of parsed tls extensions | ||
fn quic_get_tls_extensions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this func be done in a more compact way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more compact with Jason advice
} | ||
extv.push(QuicTlsExtension { etype, values }) | ||
} | ||
if client { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps move this into a client specific util func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
let mut dash = false; | ||
for c in &ch.ciphers { | ||
if dash { | ||
match write!(&mut ja3, "-") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these kinds of statements are taking 3 lines each for no good reason. Can these be made more compact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let _ = write...
(noted above already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push_str
seems nice
if framebuf.len() < 4 + hkey.sample_len() { | ||
return false; | ||
} | ||
let mut h2 = Vec::with_capacity(hlen + header.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the max size we might alloc here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not more than the UDP payload size...
Should it be more obvious or debug tested ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was trying to figure out where it is set and the logic is quite hard to follow but ultimately seems to come from quic_var_uint
, so it might be higher if not properly checked?
If it shouldn't be > UDP payload size, could it be u16
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It comes indeed from quic_var_uint
But then, we have if header.length > rest.len() {
before `let (mut framebuf, next_buf) = rest.split_at(header.length);
Do you want the check to happen earlier ?
framebuf = &output; | ||
} | ||
buf = next_buf; | ||
match QuicData::from_bytes(framebuf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this func be broken up, for example handling the following in a until func? Not only is the func large, the indent levels are also getting hard to track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Should the {
"version": "1",
"sni": "mask.icloud.com",
"ja3": "771,51914-4865-4866-4867,64250-0-10-16-5-13-18-51-45-43-57-27-20-64250,6682-29-23-24-25,",
"extensions": [
{
"type": "Grease"
},
{
"type": "ServerName",
"values": [
"mask.icloud.com"
]
},
{
"type": "SupportedGroups"
},
{
"type": "ApplicationLayerProtocolNegotiation",
"values": [
"h3"
]
},
{
"type": "StatusRequest"
},
{
"type": "SignatureAlgorithms"
},
{
"type": "SignedCertificateTimestamp"
},
{
"type": "KeyShare"
},
{
"type": "PskExchangeModes"
},
{
"type": "SupportedVersions"
},
{
"type": "TlsExtensionType(57 / 0x39)"
},
{
"type": "TlsExtensionType(27 / 0x1b)"
},
{
"type": "ServerCertificateType"
},
{
"type": "Grease"
}
]
} |
@jasonish any thoughts on the logging here (see example above). I find the Additionally I wonder how to log the unknown extensions. If we log them this way and then later add support for them its in a way a breaking output change? :) |
Yes it is My understanding is that every ja3 should contain 4 commas to distinguish the 5 different fields |
match write!(ja3, "{}", u16::from(etype)) { | ||
_ => {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idiomatic way to avoid the result warning is:
let _ = write!(...
if state.parse(buf, false) { | ||
return AppLayerResult::ok(); | ||
} | ||
return AppLayerResult::err(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really tiny nit.. Idiomatic Rust would be:
if state.parse(buf, false) {
AppLayerResult::ok()
} else {
AppLayerResult::err()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I did not know Rust was in the explicit else side
I think it looks fine as I can't really see another way. Would we want to log all the extensions by default? Or should we have an extended mode that logs extensions which is off by default.
Maybe something like:
I also noticed that we're using names like |
In the RFC https://datatracker.ietf.org/doc/html/rfc3546 |
Replaced by #7101 |
I think using a name of |
Wonder if we should also make things more compact, e.g. instead of |
These wordings come from crate tls-parser... |
I'm not opposed to it. An upstream library could change its naming which would affect us resulting in a breaking change. |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4967
https://redmine.openinfosecfoundation.org/issues/5143
Describe changes:
suricata-verify-pr: 775
OISF/suricata-verify#775
Replaces #7082 with
Still to do :