[SECURITY] Unwrap Panic in Holo-BGP decode Function

[raefko]

Unwrap Panic in Holo-BGP decode Function

Author(s): Nabih Benazzouz @Fuzzinglabs

Date: 14/01/2025

Executive Summary

This report details a bug discovered in the Holo-BGP component of the holo-routing/holo repository. A panic occurs when the code calls Option::unwrap() on a None value in the function responsible for decoding BGP messages. This issue can lead to crashes under specific conditions—particularly when malformed or unexpected data is passed into the decoder.

• Impact: A potential denial of service (DoS), as an attacker (or even a misconfiguration) could send malformed BGP packets that trigger the panic and crash the application.
• Key Findings:
  • The bug occurs at holo-bgp/src/packet/message.rs#L806.
  • The nexthop.unwrap() call panics when nexthop is None.

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_1() {
    let bytes = vec![
        255, 255, 255, 255, 255, 255, 255, 255,
        255, 255, 255, 255, 255, 255, 255, 255,
        0, 28, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 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 panic stems from calling nexthop.unwrap() without confirming that nexthop is Some(...). When the BGP update message does not include a valid nexthop, the nexthop field is None, leading to a panic when unwrap() is called.

if !prefixes.is_empty() {
    reach = Some(ReachNlri {
        prefixes,
        nexthop: nexthop.unwrap(),
    });
}

Detailed Behavior

---- packet::update::test_decode_crash_1 stdout ----

thread 'packet::update::test_decode_crash_1' panicked at holo-bgp/src/packet/message.rs:811:34:
called `Option::unwrap()` on a `None` value
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
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/core/src/panicking.rs:145:5
   3: core::option::unwrap_failed
             at /rustc/a580b5c379b4fca50dfe5afc0fc0ce00921e4e00/library/core/src/option.rs:2015:5
   4: core::option::Option<T>::unwrap
             at /home/raefko/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:978:21
   5: holo_bgp::packet::message::UpdateMsg::decode
             at ./src/packet/message.rs:811:26
   6: holo_bgp::packet::message::Message::decode
             at ./src/packet/message.rs:333:27
   7: mod::packet::update::test_decode_crash_1
             at ./tests/packet/update.rs:182:13
   8: mod::packet::update::test_decode_crash_1::{{closure}}
             at ./tests/packet/update.rs:170:25
   9: 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
  10: 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:

if !prefixes.is_empty() {
    if let Some(nexthop) = nexthop {
        reach = Some(ReachNlri { prefixes, nexthop });
    } else {
        // Handle the case where nexthop is None
        // For example, you can log an error or assign a default value
    }
}