Incomplete Patch to Issue #1392: PCAP Parser Still Trusts `phdr.len`

[yzlzbql]

Summary

Open-Vehicle-Monitoring-System-3 issue #1392 tracks a buffer overflow in canformat_pcap.cpp, where the PCAP CAN parser's phdr.len field was not properly validated. The patch associated with the issue appears to have added a guard to the PCAP serialization path (canformat_pcap::get()), rejecting internal CAN frames whose DLC is greater than 8.

However, the latest validated checkout still lacks the corresponding parser-side guard in canformat_pcap::put(). A crafted PCAP CAN record with phdr.len = 9 reaches the parser and is used as the length argument to memcpy, causing AddressSanitizer to report a stack-buffer-overflow.

Details

Classic CAN payloads are stored in fixed 8-byte buffers in this code path. Both conversion directions need to enforce that invariant:

canformat_pcap::get(): CAN_log_message_t -> PCAP record
canformat_pcap::put(): PCAP record -> CAN_log_message_t

The visible patch for issue #1392 added a DLC > 8 check in canformat_pcap::get() before copying into the PCAP record's fixed 8-byte data field:

if (message->frame.FIR.B.DLC > 8)
  {
  ESP_LOGW(TAG, "DLC too long: %d", message->frame.FIR.B.DLC);
  return std::string("");
  }
m.phdr.len = message->frame.FIR.B.DLC;
memcpy(m.data, message->frame.data.u8, message->frame.FIR.B.DLC);

That protects PCAP records produced by OVMS itself. It does not protect the parser path for externally supplied or otherwise crafted PCAP data.

In the latest validated checkout, canformat_pcap::put() still assigns the PCAP record length and then copies with it without first rejecting values above the 8-byte CAN data capacity:

message->type = CAN_LogFrame_RX;
message->frame.FIR.B.RTR = (idf & CANFORMAT_PCAP_FL_RTR)?CAN_RTR:CAN_no_RTR;
message->frame.FIR.B.FF = (idf & CANFORMAT_PCAP_FL_EXT)?CAN_frame_ext:CAN_frame_std;
message->frame.MsgID = idf & CANFORMAT_PCAP_FL_MASK;
message->origin = MyCan.GetBus(0);
message->frame.FIR.B.DLC = m.record.phdr.len;
memcpy(message->frame.data.u8, m.record.data, m.record.phdr.len);

The missing parser-side check is the same length invariant in the opposite direction. A malformed PCAP record can set phdr.len larger than the fixed 8-byte payload, and put() will use that value in the copy.

A minimal fix is to reject oversized PCAP CAN record lengths before assigning the frame DLC or copying payload bytes:

if (m.record.phdr.len > sizeof(m.record.data))
  {
  // Bad frame length - discard
  return consumed;
  }