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

Jump to bottom

Protocol mismatch causes a panic #745

Closed
fujiapple852 opened this issue Oct 31, 2023 · 0 comments · Fixed by #746
Closed

Protocol mismatch causes a panic #745

fujiapple852 opened this issue Oct 31, 2023 · 0 comments · Fixed by #746
Labels
bug Something isn't working
Milestone
0.9.0

Comments

@fujiapple852
Copy link
Owner

fujiapple852 commented Oct 31, 2023 •
edited
Loading

When receiving a TimeExceeded or DestinationUnreachable ICMP packet Trippy will attempt to extract and parse the original datagram which is nested in the payload.

Trippy assumes that the original datagram will be of the type that was dispatched by Trippy. So for example, in ICMP mode Trippy dispatches ICMP packet and in UDP mode Trippy dispatches UDP packets.

However, as Trippy uses a raw socket, it will receive all ICMP messages destined for the host, not just those related to packets dispatched by Trippy (For The UDP and TCP protocols, only packets destined for our src port will be delivered to us by the OS and so no other identifier is needed).

This is not normally an issue as Trippy will discard ICMP responses with an identifier which does not match the expected identifier (typically set to be the process id so unique on the host).

However if the original datagram received is of the wrong type (i.e. Trippy sent a TCP datagram and received an ICMP response with an original datagram of type UDP) then Trippy will fail to parse the original datagram and panic.

The fix is to validate that the expected and the actual protocol of the original datagram match and if they do not then discard the packet.

Issue observed in Trippy v0.8.0 on macOS for Ipv6/TCP tracing. This may be an Ipv6 only issue (for Ipv4 the OS should only delivery packets destined for our src port, but that does not appear to be the case for Ipv6) however the fix makes sense for both ipv4 and ipv6 and so is applied to both.
@fujiapple852 fujiapple852 added the bug Something isn't working label Oct 31, 2023
@fujiapple852 fujiapple852 added this to the 0.9.0 milestone Oct 31, 2023
@fujiapple852 fujiapple852 self-assigned this Oct 31, 2023
fujiapple852 added a commit that referenced this issue Oct 31, 2023
@fujiapple852
fix(net): ensure the actual and expected original datagram protocol m…
364b802 
…atch or discard the packet (#745)
@fujiapple852 fujiapple852 mentioned this issue Oct 31, 2023
Merged
fujiapple852 added a commit that referenced this issue Oct 31, 2023
@fujiapple852
fix(net): ensure the actual and expected original datagram protocol m…
5192b6a 
…atch or discard the packet (#745)
fujiapple852 added a commit that referenced this issue Nov 1, 2023
@fujiapple852
fix(net): ensure the actual and expected original datagram protocol m…
aec58f9 
…atch or discard the packet (#745)
@fujiapple852 fujiapple852 removed their assignment Nov 2, 2023
@chenrui333 chenrui333 mentioned this issue Nov 30, 2023
Merged
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 1, 2023
@0323pin
net/trippy: update to 0.9.0
71ffae6 
[0.9.0] - 2023-11-30

Added

- Added support for tracing flows
  ([#776](fujiapple852/trippy#776))
- Added support for `icmp` extensions
  ([#33](fujiapple852/trippy#33))
- Added support for `MPLS` label stack class `icmp` extension
  objects ([#753](fujiapple852/trippy#753))
- Added support for [paris]
  (https://github.com/libparistraceroute/libparistraceroute) ECMP routing
  for `IPv6/udp` ([#749](fujiapple852/trippy#749))
- Added `--unprivileged` (`-u`) flag to allow tracing without elevated
  privileges (macOS
  only) ([#101](fujiapple852/trippy#101))
- Added `--tui-privacy-max-ttl` flag to hide host and IP details for low ttl
  hops ([#766](fujiapple852/trippy#766))
- Added `toggle-privacy` (default: `p`) key binding to show or hide private
  hops ([#823](fujiapple852/trippy#823))
- Added `toggle-flows` (default: `f`) key binding to show or hide tracing
  flows ([#777](fujiapple852/trippy#777))
- Added `--dns-resolve-all` (`-y`) flag to allow tracing to all IPs resolved
  from DNS lookup
  entry ([#743](fujiapple852/trippy#743))
- Added `dot` report mode (`-m dot`) to output hop graph in Graphviz `DOT`
  format ([#582](fujiapple852/trippy#582))
- Added `flows` report mode (`-m flows`) to output a list of all unique tracing
  flows ([#770](fujiapple852/trippy#770))
- Added `--icmp-extensions` (`-e`) flag for parsing `IPv4`/`IPv6` `icmp`
  extensions ([#751](fujiapple852/trippy#751))
- Added `--tui-icmp-extension-mode` flag to control how `icmp` extensions are
  rendered ([#752](fujiapple852/trippy#752))
- Added `--print-config-template` flag to output a template config
  file ([#792](fujiapple852/trippy#792))
- Added `--icmp` flag as a shortcut for `--protocol icmp`
  ([#649](fujiapple852/trippy#649))
- Added `toggle-help-alt` (default: `?`) key binding to show or hide
  help ([#694](fujiapple852/trippy#694))
- Added panic handing to Tui
  ([#784](fujiapple852/trippy#784))
- Added official Windows `scoop` package
  ([#462](fujiapple852/trippy#462))
- Added official Windows `winget` package
  ([#460](fujiapple852/trippy#460))
- Release `musl` Debian `deb` binary asset
  ([#568](fujiapple852/trippy#568))
- Release `armv7` Linux binary assets
  ([#712](fujiapple852/trippy#712))
- Release `aarch64-apple-darwin` (aka macOS Apple Silicon) binary
  assets ([#801](fujiapple852/trippy#801))
- Added additional Rust Tier 1 and Tier 2 binary assets
  ([#811](fujiapple852/trippy#811))

Changed

- [BREAKING CHANGE] `icmp` extension object data added to `json` and `stream`
  reports ([#806](fujiapple852/trippy#806))
- [BREAKING CHANGE] IPs field added to `csv` and all tabular
  reports ([#597](fujiapple852/trippy#597))
- [BREAKING CHANGE] Command line flags `--dns-lookup-as-info` and
  `--tui-preserve-screen` no longer require a boolean
  argument ([#708](fujiapple852/trippy#708))
- [BREAKING CHANGE] Default key binding for `ToggleFreeze` changed from `f`
  to `ctrl+f` ([#785](fujiapple852/trippy#785))
- Always render AS lines in hop details mode
  ([#825](fujiapple852/trippy#825))
- Expose DNS resolver module as part of `trippy` library
  ([#754](fujiapple852/trippy#754))
- Replaced unmaintained `tui-rs` crate with `ratatui` crate
  ([#569](fujiapple852/trippy#569))

Fixed

- Reverse DNS lookup not working in reports
  ([#509](fujiapple852/trippy#509))
- Crash on NetBSD during window resizing
  ([#276](fujiapple852/trippy#276))
- Protocol mismatch causes tracer panic
  ([#745](fujiapple852/trippy#745))
- Incorrect row height in Tui hop detail navigation view for hops with no
  responses ([#765](fujiapple852/trippy#765))
- Unnecessary socket creation in certain tracing modes
  ([#647](fujiapple852/trippy#647))
- Incorrect byte order in `IPv4` packet length calculation
  ([#686](fujiapple852/trippy#686))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
0.9.0
1 participant
@fujiapple852