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

Bug: Ipv4Packet::get_total_length() incorrect byte order #686

Closed
fujiapple852 opened this issue Sep 24, 2023 · 0 comments · Fixed by #716
Closed

Bug: Ipv4Packet::get_total_length() incorrect byte order #686

fujiapple852 opened this issue Sep 24, 2023 · 0 comments · Fixed by #716
Labels
bug Something isn't working
Milestone

Comments

@fujiapple852
Copy link
Owner

fujiapple852 commented Sep 24, 2023

The Ipv4 total_length has a platform dependant byte order (see byte_order.rs for details) and therefore this function does not always return the correct byte order:

pub fn get_total_length(&self) -> u16 {
u16::from_be_bytes(self.buf.get_bytes(TOTAL_LENGTH_OFFSET))
}

This in turn can lead to the payload length calculation being incorrect:

fn ipv4_payload_length(ipv4: &Ipv4Packet<'_>) -> usize {
(ipv4.get_total_length() as usize).saturating_sub(ipv4.get_header_length() as usize * 4)
}

This is not causing a real world issue today as, when the byte order is interpreted incorrectly it typically leads to a very large value for total_length but this is harmless as the code takes the minimum of this and the total buffer size.

To fix this we would require access to the calculated PlatformIpv4FieldByteOrder in the latter function, however this is not something we wish to expose to the raw packet modules. Instead we defer the issue to the caller by returning all available bytes after the Ipv4 header and options. This behaviour is also consistent with the behaviour of all other payload methods in the packet modules.

@fujiapple852 fujiapple852 added the bug Something isn't working label Sep 24, 2023
@fujiapple852 fujiapple852 added this to the 0.9.0 milestone Sep 24, 2023
@fujiapple852 fujiapple852 self-assigned this Sep 24, 2023
@fujiapple852 fujiapple852 changed the title Bug: IPv4::get_total_length() incorrect byte order Bug: Ipv4Packet::get_total_length() incorrect byte order Sep 24, 2023
@fujiapple852 fujiapple852 removed their assignment Nov 2, 2023
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 1, 2023
[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
Labels
bug Something isn't working
Projects
None yet
1 participant