From ae4d1894fd0c2b42f5fc30fcc7beacc01499db27 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Tue, 31 Mar 2026 09:12:49 -0700 Subject: [PATCH] fix(mavlink): align signing with MAVLink spec and fix performance regression Remove the non-standard MAV_SIGN_CFG parameter and align the signing implementation with the MAVLink specification. Key changes: - Remove MAV_SIGN_CFG parameter that conflicted with GCS implementations - Only enable signing when a valid key is present on the SD card - Accept SETUP_SIGNING on any link, not just USB - Reject SETUP_SIGNING while the vehicle is armed - Allow disabling signing via signed all-zero key SETUP_SIGNING message - Propagate key changes to all mavlink instances - Zero CPU/bandwidth overhead when signing is not active Fixes #26893 Signed-off-by: Ramon Roche --- docs/en/mavlink/message_signing.md | 116 +++++++++----- docs/en/mavlink/security_hardening.md | 30 ++-- src/modules/mavlink/mavlink_main.cpp | 72 ++++++--- src/modules/mavlink/mavlink_main.h | 8 +- src/modules/mavlink/mavlink_params.yaml | 9 -- src/modules/mavlink/mavlink_receiver.cpp | 3 + src/modules/mavlink/mavlink_sign_control.cpp | 150 ++++++++++++------- src/modules/mavlink/mavlink_sign_control.h | 42 ++++-- 8 files changed, 283 insertions(+), 147 deletions(-) diff --git a/docs/en/mavlink/message_signing.md b/docs/en/mavlink/message_signing.md index 62e4b17975..1357d8eddb 100644 --- a/docs/en/mavlink/message_signing.md +++ b/docs/en/mavlink/message_signing.md @@ -12,26 +12,27 @@ When signing is enabled, PX4 appends a 13-byte [signature](https://mavlink.io/en Incoming messages are checked against the shared secret key, and unsigned or incorrectly signed messages are rejected (with [exceptions for safety-critical messages](#unsigned-message-allowlist)). -The signing implementation is built into the MAVLink module and is always available — no special build flags are required. -It is enabled and disabled at runtime through the [MAV_SIGN_CFG](../advanced_config/parameter_reference.md#MAV_SIGN_CFG) parameter. +The signing implementation is built into the MAVLink module and is always available, with no special build flags required. ## Enable/Disable Signing -The [MAV_SIGN_CFG](../advanced_config/parameter_reference.md#MAV_SIGN_CFG) parameter controls whether signing is active: +Signing is controlled entirely through the standard MAVLink [SETUP_SIGNING](https://mavlink.io/en/messages/common.html#SETUP_SIGNING) message, following the [MAVLink signing specification](https://mavlink.io/en/guide/message_signing.html). -| Value | Mode | Description | -| ----- | ------------------ | ------------------------------------------------------------------------------------------------------ | -| 0 | Disabled (default) | No signing. All messages are accepted regardless of signature. | -| 1 | Non-USB | Signing is enabled on all links **except** USB serial connections. USB links accept unsigned messages. | -| 2 | Always | Signing is enforced on all links, including USB. | +- **No key on SD card**: Signing is disabled. All messages are sent unsigned and all incoming messages are accepted. +- **Valid key on SD card**: Signing is active on **all links including USB**. Outgoing messages are signed and incoming messages must be signed (with [exceptions](#unsigned-message-allowlist)). + +To **enable** signing, send a `SETUP_SIGNING` message with a valid key on any link when no key is currently provisioned (see [Key Provisioning](#key-provisioning)). + +To **disable** signing via MAVLink, send a `SETUP_SIGNING` message with an all-zero key and timestamp. This message **must be signed with the current active key**. An unsigned blank-key message is rejected. + +To **disable** signing via physical access, remove the key file from the SD card (`/mavlink/mavlink-signing-key.bin`) and reboot, or remove the SD card entirely. + +To **change** the signing key, send a `SETUP_SIGNING` message with the new key on any link. When signing is already active, the message must be signed with the current key. ::: warning -Setting `MAV_SIGN_CFG` alone does not enable signing — a secret key must also be present (see [Key Provisioning](#key-provisioning) below). -If no key has been set (or the key is all zeros with a zero timestamp), all messages are accepted regardless of this parameter. +Signing key changes (enable, disable, or rotate) are **rejected while the vehicle is armed**. The vehicle must be disarmed before signing configuration can be changed. ::: -To **disable** signing, set `MAV_SIGN_CFG` to zero. - ## Key Provisioning The signing key is set by sending the MAVLink [SETUP_SIGNING](https://mavlink.io/en/messages/common.html#SETUP_SIGNING) message (ID 256) to PX4. @@ -40,10 +41,14 @@ This message contains: - A 32-byte secret key - A 64-bit initial timestamp +PX4 accepts `SETUP_SIGNING` on **any link** (USB, telemetry radio, network, and so on). + +When signing is **not active** (no key provisioned), the first `SETUP_SIGNING` with a valid key enables signing. +When signing is **already active**, key changes (including disabling) require that the `SETUP_SIGNING` message is signed with the current key. + ::: warning -For security, PX4 only accepts `SETUP_SIGNING` messages received on a **USB** connection. -The message is silently ignored on all other link types (telemetry radios, network, and so on). -This ensures that an attacker cannot remotely change the signing key. +`SETUP_SIGNING` is rejected while the vehicle is armed. Disarm before provisioning or changing keys. +`SETUP_SIGNING` is never forwarded to other links (per the MAVLink specification). ::: ## Key Storage @@ -64,7 +69,7 @@ The file is a 40-byte binary file: The file is created with mode `0600` (owner read/write only), and the containing `/mavlink/` directory is created with mode `0700` (owner only). On startup, PX4 reads the key from this file. -If the file exists and contains a non-zero key or timestamp, signing is initialized automatically. +If the file exists and contains a non-zero key or timestamp, signing is activated automatically. ::: info The timestamp in the file is set when `SETUP_SIGNING` is received. @@ -82,45 +87,82 @@ Note that this requires physical access to the vehicle, and therefore provides t 1. The MAVLink module calls `MavlinkSignControl::start()` during startup. 2. The `/mavlink/` directory is created if it doesn't exist. -3. The `mavlink-signing-key.bin` file is opened (or created empty). -4. If a valid key is found (non-zero key or timestamp), signing is marked as initialized. -5. The `accept_unsigned` callback is registered with the MAVLink library. +3. The `mavlink-signing-key.bin` file is opened if it exists. +4. If a valid key is found (non-zero key or timestamp), signing is activated: the signing struct is wired into the MAVLink library and outgoing messages are signed. +5. If no valid key is found, the signing struct is left disconnected, and the MAVLink library operates with zero signing overhead. ### Outgoing Messages -When signing is initialized, the `MAVLINK_SIGNING_FLAG_SIGN_OUTGOING` flag is set, which causes the MAVLink library to automatically append a [SHA-256 based signature](https://mavlink.io/en/guide/message_signing.html#signature) to every outgoing MAVLink 2 message. +When signing is active (valid key present), the `MAVLINK_SIGNING_FLAG_SIGN_OUTGOING` flag is set, which causes the MAVLink library to automatically append a [SHA-256 based signature](https://mavlink.io/en/guide/message_signing.html#signature) to every outgoing MAVLink 2 message. + +When no key is present, signing is completely bypassed with no CPU or bandwidth overhead. ### Incoming Messages For each incoming message, the MAVLink library checks whether a valid signature is present. If the message is unsigned or has an invalid signature, the library calls the `accept_unsigned` callback, which decides whether to accept or reject the message based on: -1. **Signing not initialized** — If no key has been loaded, all messages are accepted. -2. **Allowlisted message** — Certain [safety-critical messages](#unsigned-message-allowlist) are always accepted. -3. **Sign mode** — The `MAV_SIGN_CFG` parameter determines behavior: - - Mode 0 (disabled): All unsigned messages are accepted. - - Mode 1 (non-USB): Unsigned messages are accepted only on USB links. - - Mode 2 (always): Unsigned messages are rejected on all links. +1. **Signing not active**: If no key has been loaded, all messages are accepted. +2. **Allowlisted message**: Certain [safety-critical messages](#unsigned-message-allowlist) are always accepted. ## Unsigned Message Allowlist -The following messages are **always** accepted unsigned, regardless of the signing mode. +The following messages are **always** accepted unsigned, regardless of the signing state. These are safety-critical messages that may originate from systems that don't support signing: -| Message | ID | Reason | -| ----------------------------------------------------------------------- | --- | -------------------------------------------------------- | -| [RADIO_STATUS](https://mavlink.io/en/messages/common.html#RADIO_STATUS) | 109 | Radio link status from SiK radios and other radio modems | -| [ADSB_VEHICLE](https://mavlink.io/en/messages/common.html#ADSB_VEHICLE) | 246 | ADS-B traffic information for collision avoidance | -| [COLLISION](https://mavlink.io/en/messages/common.html#COLLISION) | 247 | Collision threat warnings | +| Message | ID | Reason | +| ------------------------------------------------------------------------- | --- | -------------------------------------------------------- | +| [HEARTBEAT](https://mavlink.io/en/messages/common.html#HEARTBEAT) | 0 | System discovery and liveness detection | +| [RADIO_STATUS](https://mavlink.io/en/messages/common.html#RADIO_STATUS) | 109 | Radio link status from SiK radios and other radio modems | +| [ADSB_VEHICLE](https://mavlink.io/en/messages/common.html#ADSB_VEHICLE) | 246 | ADS-B traffic information for collision avoidance | +| [COLLISION](https://mavlink.io/en/messages/common.html#COLLISION) | 247 | Collision threat warnings | ## Security Considerations -- **Physical access required for key setup**: The `SETUP_SIGNING` message is only accepted over USB, so an attacker must have physical access to the vehicle to provision or change the key. +### Signing is enforced on all links + +When signing is active, **all links require signed messages**, including USB. This means: + +- An attacker cannot send unsigned commands on any link +- Changing or disabling the key requires sending a `SETUP_SIGNING` message **signed with the current key** +- Signing can be disabled via MAVLink by sending a signed `SETUP_SIGNING` with an all-zero key + +### Armed guard + +`SETUP_SIGNING` is rejected while the vehicle is armed. This prevents signing configuration from being changed during flight, whether by accident or by an attacker who has obtained the key. + +### Lost key recovery + +If the signing key is lost on the GCS side and no device has the current key: + +- **Remove the SD card** and delete `/mavlink/mavlink-signing-key.bin`, then reboot +- **Reflash via SWD/JTAG** if the SD card is not accessible + +::: warning +There is no software-only recovery path for a lost key. This is intentional: any MAVLink-based recovery mechanism would also be available to an attacker. +Physical access to the SD card or debug port is required. +::: + +### Other considerations + +- **Initial key provisioning**: When no key is provisioned, `SETUP_SIGNING` is accepted unsigned on any link. Once a key is active, subsequent changes require a signed message. Provision the initial key over a trusted connection. - **Key not exposed via parameters**: The secret key is stored in a separate file on the SD card, not as a MAVLink parameter, so it cannot be read back through the parameter protocol. -- **SD card access**: Anyone with physical access to the SD card can read or modify the `mavlink-signing-key.bin` file, or just remove the card. +- **SD card access**: Anyone with physical access to the SD card can read or modify the `mavlink-signing-key.bin` file, or remove the card entirely to disable signing. Ensure physical security of the vehicle if signing is used as a security control. - **Replay protection**: The MAVLink signing protocol includes a timestamp that prevents replay attacks. The on-disk timestamp is updated when a new key is provisioned via `SETUP_SIGNING`. - A graceful shutdown also persists the current timestamp, but since most vehicles are powered off by pulling the battery, the timestamp will typically reset to the value from the last key provisioning on reboot. -- **No encryption**: Message signing provides authentication and integrity, but messages are still sent in plaintext. - An eavesdropper can read message contents but cannot forge or modify them without the key. + A graceful shutdown also persists the current timestamp, but since most vehicles are powered off by pulling the battery, the on-disk timestamp will typically remain at the value from the last key provisioning on reboot. +- **No encryption**: Message signing provides **authentication and integrity only**. Messages are still sent in plaintext. + An eavesdropper can read all message contents (telemetry, commands, parameters, missions) but cannot forge or modify them without the key. +- **Allowlisted messages bypass signing**: A small set of [safety-critical messages](#unsigned-message-allowlist) are always accepted unsigned. An attacker can spoof these specific messages (e.g. fake `ADSB_VEHICLE` traffic) even when signing is active. + +### What signing does NOT protect against + +| Attack | Why | +|--------|-----| +| Eavesdropping | Messages are not encrypted | +| SD card extraction | Key file is readable by anyone with physical access | +| Spoofed HEARTBEAT/RADIO_STATUS/ADSB/COLLISION | These are allowlisted unsigned | +| Lost key without SD card access | Requires SWD reflash | +| Key rotation | No automatic mechanism; manual reprovisioning required | +| In-flight key changes | `SETUP_SIGNING` rejected while armed | diff --git a/docs/en/mavlink/security_hardening.md b/docs/en/mavlink/security_hardening.md index e18dcd0f67..aec2d2f883 100644 --- a/docs/en/mavlink/security_hardening.md +++ b/docs/en/mavlink/security_hardening.md @@ -37,22 +37,27 @@ See [Message Signing](message_signing.md) for full details. Steps: -1. Connect the vehicle via **USB** (key provisioning only works over USB). -2. Provision a 32-byte secret key using the [SETUP_SIGNING](https://mavlink.io/en/messages/common.html#SETUP_SIGNING) message. -3. Set [MAV_SIGN_CFG](../advanced_config/parameter_reference.md#MAV_SIGN_CFG) to **1** (signing enabled on all links except USB) or **2** (signing on all links including USB). -4. Provision the same key on all ground control stations and companion computers that need to communicate with the vehicle. -5. Verify that unsigned messages from unknown sources are rejected. +1. Connect to the vehicle over a **trusted link** (USB or other secure connection). +2. Provision a 32-byte secret key using the [SETUP_SIGNING](https://mavlink.io/en/messages/common.html#SETUP_SIGNING) message. This works on any link, but use a trusted one for initial provisioning. +3. Provision the same key on all ground control stations and companion computers that need to communicate with the vehicle. +4. Verify that unsigned messages from unknown sources are rejected. ::: info -`MAV_SIGN_CFG=1` is recommended for most deployments. -This enforces signing on telemetry radios and network links while allowing unsigned access over USB for maintenance. -USB connections require physical access to the vehicle, which provides equivalent security to physical key access. +Once a key is provisioned, signing is enforced automatically on **all links including USB**. +Changing or disabling the key requires a signed `SETUP_SIGNING` message. Signing changes are rejected while the vehicle is armed. +Signing can also be disabled by physically removing the key file from the SD card. ::: ### 2. Secure Physical Access -- Protect access to the SD card. The signing key is stored at `/mavlink/mavlink-signing-key.bin` and can be read or removed by anyone with physical access. -- USB connections bypass signing when `MAV_SIGN_CFG=1`. Ensure USB ports are not exposed in deployed configurations. +- **SD card**: The signing key is stored at `/mavlink/mavlink-signing-key.bin`. Anyone with physical access to the SD card can read, modify, or remove the key file. +- **USB ports**: USB follows the same signing rules as all other links. When signing is active, USB requires signed messages. +- **Debug ports (SWD/JTAG)**: If exposed, these allow full firmware reflash and bypass all software security. Not all vehicles expose debug connectors. + +::: warning +Signing protects all MAVLink links. The primary physical attack surface is the SD card (key file extraction or deletion). +If your threat model includes physical access, secure the SD card slot and debug ports. +::: ### 3. Secure Network Links @@ -64,8 +69,9 @@ USB connections require physical access to the vehicle, which provides equivalen ### 4. Understand the Limitations - **No encryption**: Message signing provides authentication and integrity, but messages are sent in plaintext. An eavesdropper can read telemetry and commands but cannot forge them. -- **Allowlisted messages**: A small set of [safety-critical messages](message_signing.md#unsigned-message-allowlist) (RADIO_STATUS, ADSB_VEHICLE, COLLISION) are always accepted unsigned. -- **Key management**: There is no automatic key rotation. Keys must be reprovisioned manually via USB if compromised. +- **Allowlisted messages**: A small set of [safety-critical messages](message_signing.md#unsigned-message-allowlist) (HEARTBEAT, RADIO_STATUS, ADSB_VEHICLE, COLLISION) are always accepted unsigned on all links. An attacker could spoof these specific messages. +- **Key management**: There is no automatic key rotation. Keys must be reprovisioned manually via a signed `SETUP_SIGNING` message. +- **Lost key recovery**: If the signing key is lost on all GCS devices, the only recovery is physical: remove the SD card and delete the key file, or reflash via SWD/JTAG. There is no software-only recovery path. See [Message Signing: Lost Key Recovery](message_signing.md#lost-key-recovery) for details. ## Integrator Responsibility diff --git a/src/modules/mavlink/mavlink_main.cpp b/src/modules/mavlink/mavlink_main.cpp index 802bc40858..8c26ee6a5a 100644 --- a/src/modules/mavlink/mavlink_main.cpp +++ b/src/modules/mavlink/mavlink_main.cpp @@ -97,10 +97,14 @@ mavlink_message_t *mavlink_get_channel_buffer(uint8_t channel) { return mavlink_ static bool accept_unsigned_callback(const mavlink_status_t *status, uint32_t message_id) { - Mavlink *m = Mavlink::get_instance_for_status(status); + // Use link_id to index directly: the callback fires on the instance's own + // receiver thread, so no lock needed (instance can't be destroyed while running). + if (status->signing) { + Mavlink *inst = mavlink_module_instances[status->signing->link_id]; - if (m != nullptr) { - return m -> accept_unsigned(m->sign_mode(), m -> is_usb_uart(), message_id); + if (inst != nullptr) { + return inst->accept_unsigned(message_id); + } } return false; @@ -325,20 +329,6 @@ Mavlink::get_instance_for_device(const char *device_name) return nullptr; } -Mavlink * -Mavlink::get_instance_for_status(const mavlink_status_t *status) -{ - LockGuard lg{mavlink_module_mutex}; - - for (Mavlink *inst : mavlink_module_instances) { - if (status == mavlink_get_channel_status(inst->get_instance_id())) { - return inst; - } - } - - return nullptr; -} - #ifdef MAVLINK_UDP Mavlink * Mavlink::get_instance_for_network_port(unsigned long port) @@ -1054,10 +1044,50 @@ Mavlink::handle_message(const mavlink_message_t *msg) * NOTE: this is called from the receiver thread */ - if (is_usb_uart()) { - if (_sign_control.check_for_signing(msg)) { + // SETUP_SIGNING must never be forwarded to other links (MAVLink spec requirement). + if (msg->msgid == MAVLINK_MSG_ID_SETUP_SIGNING) { + // Reject signing changes while armed + vehicle_status_s vehicle_status{}; + + if (_vehicle_status_sub.copy(&vehicle_status) + && vehicle_status.arming_state == vehicle_status_s::ARMING_STATE_ARMED) { + send_statustext_critical("MAVLink signing: rejected while armed"); return; } + + MavlinkSignControl::SetupSigningResult result = _sign_control.check_for_signing(msg); + + switch (result) { + case MavlinkSignControl::KEY_ACCEPTED: + send_statustext_info("MAVLink signing key accepted"); + break; + + case MavlinkSignControl::SIGNING_DISABLED: + send_statustext_info("MAVLink signing disabled"); + break; + + case MavlinkSignControl::BLANK_KEY_REJECTED: + send_statustext_critical("MAVLink signing: blank key rejected"); + break; + + default: + break; + } + + if (result == MavlinkSignControl::KEY_ACCEPTED || result == MavlinkSignControl::SIGNING_DISABLED) { + // Signal all other instances to reload key from file on their own thread + LockGuard lg{mavlink_module_mutex}; + + for (int i = 0; i < MAVLINK_COMM_NUM_BUFFERS; i++) { + Mavlink *inst = mavlink_module_instances[i]; + + if (inst != nullptr && inst != this) { + inst->set_signing_key_dirty(); + } + } + } + + return; } if (get_forwarding_on()) { @@ -1976,8 +2006,6 @@ Mavlink::task_main(int argc, char *argv[]) } } - _sign_control.start(_instance_id, get_status(), &accept_unsigned_callback); - int ch; _baudrate = 57600; _datarate = 0; @@ -2317,6 +2345,8 @@ Mavlink::task_main(int argc, char *argv[]) return PX4_ERROR; } + _sign_control.start(get_instance_id(), get_status(), &accept_unsigned_callback); + pthread_mutex_init(&_mavlink_shell_mutex, nullptr); pthread_mutex_init(&_message_buffer_mutex, nullptr); pthread_mutex_init(&_send_mutex, nullptr); diff --git a/src/modules/mavlink/mavlink_main.h b/src/modules/mavlink/mavlink_main.h index 3e13715d64..fde7524ff3 100644 --- a/src/modules/mavlink/mavlink_main.h +++ b/src/modules/mavlink/mavlink_main.h @@ -136,7 +136,6 @@ public: mavlink_message_t *get_buffer() { return &_mavlink_buffer; } mavlink_status_t *get_status() { return &_mavlink_status; } - static Mavlink *get_instance_for_status(const mavlink_status_t *status); void setProtocolVersion(uint8_t version); uint8_t getProtocolVersion() const { return _protocol_version; }; @@ -470,7 +469,6 @@ public: bool ftp_enabled() const { return _ftp_on; } bool hash_check_enabled() const { return _param_mav_hash_chk_en.get(); } - int32_t sign_mode() const { return _param_mav_sign_cfg.get(); } bool forward_heartbeats_enabled() const { return _param_mav_hb_forw_en.get(); } bool failure_injection_enabled() const { return _param_sys_failure_injection_enabled.get(); } @@ -494,7 +492,9 @@ public: bool radio_status_critical() const { return _radio_status_critical; } - bool accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id) { return _sign_control.accept_unsigned(sign_mode, is_usb_uart, message_id); } + bool accept_unsigned(uint32_t message_id) { return _sign_control.accept_unsigned(message_id); } + void set_signing_key_dirty() { _signing_key_dirty.store(true); } + void check_signing_key_dirty() { if (_signing_key_dirty.load()) { _signing_key_dirty.store(false); _sign_control.reload_key(); } } private: @@ -507,6 +507,7 @@ private: px4::atomic_bool _task_should_exit{false}; px4::atomic_bool _task_running{false}; + px4::atomic_bool _signing_key_dirty{false}; bool _transmitting_enabled{true}; bool _transmitting_enabled_commanded{false}; @@ -641,7 +642,6 @@ private: (ParamBool) _param_mav_usehilgps, (ParamBool) _param_mav_fwdextsp, (ParamBool) _param_mav_hash_chk_en, - (ParamInt) _param_mav_sign_cfg, (ParamBool) _param_mav_hb_forw_en, (ParamInt) _param_mav_radio_timeout, (ParamInt) _param_sys_hitl, diff --git a/src/modules/mavlink/mavlink_params.yaml b/src/modules/mavlink/mavlink_params.yaml index 10039647cd..be5aeb1c24 100644 --- a/src/modules/mavlink/mavlink_params.yaml +++ b/src/modules/mavlink/mavlink_params.yaml @@ -18,15 +18,6 @@ parameters: min: 1 max: 250 reboot_required: true - MAV_SIGN_CFG: - description: - short: MAVLink protocol signing - type: enum - values: - 0: Message signing disabled - 1: Signing enabled except on USB - 2: Signing always enabled - default: 0 MAV_PROTO_VER: description: short: MAVLink protocol version diff --git a/src/modules/mavlink/mavlink_receiver.cpp b/src/modules/mavlink/mavlink_receiver.cpp index 151a091ef2..247d3d171d 100644 --- a/src/modules/mavlink/mavlink_receiver.cpp +++ b/src/modules/mavlink/mavlink_receiver.cpp @@ -3272,6 +3272,9 @@ MavlinkReceiver::run() updateParams(); } + // Reload signing key if another instance updated it + _mavlink.check_signing_key_dirty(); + int ret = poll(&fds[0], 1, timeout); if (ret > 0) { diff --git a/src/modules/mavlink/mavlink_sign_control.cpp b/src/modules/mavlink/mavlink_sign_control.cpp index c4ace8a7ac..16f71ab340 100644 --- a/src/modules/mavlink/mavlink_sign_control.cpp +++ b/src/modules/mavlink/mavlink_sign_control.cpp @@ -43,7 +43,10 @@ static mavlink_signing_streams_t global_mavlink_signing_streams = {}; +// Messages accepted without signing per MAVLink spec recommendation. +// HEARTBEAT is required for link discovery/interop but allows spoofed phantom vehicles on GCS. static const uint32_t unsigned_messages[] = { + MAVLINK_MSG_ID_HEARTBEAT, MAVLINK_MSG_ID_RADIO_STATUS, MAVLINK_MSG_ID_ADSB_VEHICLE, MAVLINK_MSG_ID_COLLISION @@ -57,11 +60,11 @@ MavlinkSignControl::~MavlinkSignControl() { } -void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_status, +void MavlinkSignControl::start(int instance_id, mavlink_status_t *mavlink_status, mavlink_accept_unsigned_t accept_unsigned_callback) { - _mavlink_signing.link_id = _instance_id; - _mavlink_signing.flags = MAVLINK_SIGNING_FLAG_SIGN_OUTGOING; + _mavlink_status = mavlink_status; + _mavlink_signing.link_id = instance_id; _mavlink_signing.accept_unsigned_callback = accept_unsigned_callback; _is_signing_initialized = false; @@ -71,19 +74,18 @@ void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_stat PX4_ERR("failed creating module storage dir: %s (%i)", MAVLINK_FOLDER_PATH, errno); } else { - int _fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_RDONLY, PX4_O_MODE_600); + int fd = ::open(MAVLINK_SECRET_FILE, O_RDONLY); - if (_fd == -1) { + if (fd == -1) { if (errno != ENOENT) { - PX4_ERR("failed creating mavlink secret key file: %s (%i)", MAVLINK_SECRET_FILE, errno); + PX4_ERR("failed opening mavlink secret key file: %s (%i)", MAVLINK_SECRET_FILE, errno); } } else { - //if we dont have enough bytes we simply ignore it , because it may be not set yet - ssize_t bytes_read = ::read(_fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); + ssize_t bytes_read = ::read(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); if (bytes_read == MAVLINK_SECRET_KEY_LENGTH) { - bytes_read = ::read(_fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH); + bytes_read = ::read(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH); if (bytes_read == MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH) { if (_mavlink_signing.timestamp != 0 || !is_array_all_zeros(_mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) { @@ -92,99 +94,145 @@ void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_stat } } - close(_fd); + close(fd); } } - //lets reset it to nulls if it was not read properly if (!_is_signing_initialized) { - for (size_t i = 0; i < MAVLINK_SECRET_KEY_LENGTH; ++i) { - _mavlink_signing.secret_key[i] = 0; - } - + memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH); _mavlink_signing.timestamp = 0; } - // copy pointer of the signing to status struct - _mavlink_status -> signing = &_mavlink_signing; - _mavlink_status -> signing_streams = &global_mavlink_signing_streams; + _update_signing_state(); } -bool MavlinkSignControl::check_for_signing(const mavlink_message_t *msg) +MavlinkSignControl::SetupSigningResult MavlinkSignControl::check_for_signing(const mavlink_message_t *msg) { if (msg->msgid != MAVLINK_MSG_ID_SETUP_SIGNING) { - return false; + return NOT_SETUP_SIGNING; } mavlink_setup_signing_t setup_signing; mavlink_msg_setup_signing_decode(msg, &setup_signing); - //setup signing provides new key , lets update it - //we update it only in case everything was stored properly - memcpy(_mavlink_signing.secret_key, setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); - _mavlink_signing.timestamp = setup_signing.initial_timestamp; + bool new_key_blank = (setup_signing.initial_timestamp == 0 + && is_array_all_zeros(setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)); - if (setup_signing.initial_timestamp != 0 || !is_array_all_zeros(setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) { - _is_signing_initialized = true; + if (new_key_blank) { + // Disable signing: only allowed if signing is active and the message is signed + if (!_is_signing_initialized) { + // Already disabled, nothing to do + return SIGNING_DISABLED; + } - } else { + bool msg_is_signed = (msg->incompat_flags & MAVLINK_IFLAG_SIGNED); + + if (!msg_is_signed) { + PX4_WARN("SETUP_SIGNING blank key rejected: message must be signed"); + return BLANK_KEY_REJECTED; + } + + memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH); + _mavlink_signing.timestamp = 0; _is_signing_initialized = false; + + _update_signing_state(); + write_key_and_timestamp(); + + return SIGNING_DISABLED; } + memcpy(_mavlink_signing.secret_key, setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); + _mavlink_signing.timestamp = setup_signing.initial_timestamp; + _is_signing_initialized = true; + + _update_signing_state(); write_key_and_timestamp(); - return true; + return KEY_ACCEPTED; +} + +void MavlinkSignControl::reload_key() +{ + if (_mavlink_status == nullptr) { + return; + } + + _is_signing_initialized = false; + + int fd = ::open(MAVLINK_SECRET_FILE, O_RDONLY); + + if (fd != -1) { + ssize_t bytes_read = ::read(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); + + if (bytes_read == MAVLINK_SECRET_KEY_LENGTH) { + bytes_read = ::read(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH); + + if (bytes_read == MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH) { + if (_mavlink_signing.timestamp != 0 || !is_array_all_zeros(_mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) { + _is_signing_initialized = true; + } + } + } + + close(fd); + } + + if (!_is_signing_initialized) { + memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH); + _mavlink_signing.timestamp = 0; + } + + _update_signing_state(); +} + +void MavlinkSignControl::_update_signing_state() +{ + if (_is_signing_initialized) { + _mavlink_signing.flags = MAVLINK_SIGNING_FLAG_SIGN_OUTGOING; + _mavlink_status->signing = &_mavlink_signing; + _mavlink_status->signing_streams = &global_mavlink_signing_streams; + + } else { + _mavlink_signing.flags = 0; + _mavlink_status->signing = nullptr; + _mavlink_status->signing_streams = nullptr; + } } void MavlinkSignControl::write_key_and_timestamp() { - int _fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_WRONLY | O_TRUNC, PX4_O_MODE_600); + int fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_WRONLY | O_TRUNC, PX4_O_MODE_600); - if (_fd == -1) { + if (fd == -1) { if (errno != ENOENT) { PX4_ERR("failed opening mavlink secret key file for writing: %s (%i)", MAVLINK_SECRET_FILE, errno); } } else { - ssize_t bytes_write = ::write(_fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); + ssize_t bytes_write = ::write(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH); if (bytes_write == MAVLINK_SECRET_KEY_LENGTH) { - bytes_write = ::write(_fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH); + bytes_write = ::write(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH); } - close(_fd); + close(fd); } } -bool MavlinkSignControl::accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id) +bool MavlinkSignControl::accept_unsigned(uint32_t message_id) { - // if signing is not initilized properly or has all zeroes we will allow any message if (!_is_signing_initialized) { return true; } - // Always accept a few select messages even if unsigned for (unsigned i = 0; i < sizeof(unsigned_messages) / sizeof(unsigned_messages[0]); i++) { if (unsigned_messages[i] == message_id) { return true; } } - switch (sign_mode) { - // If signing is not required always return true - case MavlinkSignControl::PROTO_SIGN_OPTIONAL: - return true; - - // Accept USB links if enabled - case MavlinkSignControl::PROTO_SIGN_NON_USB: - return is_usb_uart; - - case MavlinkSignControl::PROTO_SIGN_ALWAYS: - - // fallthrough - default: - return false; - } + return false; } bool MavlinkSignControl::is_array_all_zeros(uint8_t arr[], size_t size) diff --git a/src/modules/mavlink/mavlink_sign_control.h b/src/modules/mavlink/mavlink_sign_control.h index 9c4c631797..91c68aeff5 100644 --- a/src/modules/mavlink/mavlink_sign_control.h +++ b/src/modules/mavlink/mavlink_sign_control.h @@ -59,40 +59,56 @@ public: MavlinkSignControl(); ~MavlinkSignControl(); - enum PROTO_SIGN { - PROTO_SIGN_OPTIONAL = 0, - PROTO_SIGN_NON_USB, - PROTO_SIGN_ALWAYS + /** + * Initialize signing state and read key from file. + * Only enables signing if a valid key exists on the SD card. + */ + void start(int instance_id, mavlink_status_t *mavlink_status, + mavlink_accept_unsigned_t accept_unsigned_callback); + + enum SetupSigningResult { + NOT_SETUP_SIGNING = 0, ///< Message was not SETUP_SIGNING + KEY_ACCEPTED, ///< New key provisioned successfully + SIGNING_DISABLED, ///< Signing disabled via signed blank key + BLANK_KEY_REJECTED ///< Blank key rejected (unsigned or signing not active) }; /** - * Initialize signing and read configuration from file + * Checks whether the message is SETUP_SIGNING, and if yes, updates local key. + * Enables or disables signing based on whether the new key is valid. */ - void start(int _instance_id, mavlink_status_t *_mavlink_status, mavlink_accept_unsigned_t accept_unsigned_callback); + SetupSigningResult check_for_signing(const mavlink_message_t *msg); /** - * Checks whether the message is SETUP_SIGNING, and if yes , updates local key + * Reload key from SD card file. Called on the instance's own receiver thread + * when the signing key dirty flag is set by another instance. */ - bool check_for_signing(const mavlink_message_t *msg); + void reload_key(); /** - * stores the key and timestamp from memory to file + * Stores the key and timestamp from memory to file */ void write_key_and_timestamp(); /** - * Checks whether should accept unsigned message for specific sign mode + * Checks whether an unsigned message should be accepted */ - bool accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id); + bool accept_unsigned(uint32_t message_id); + + bool is_signing_active() const { return _is_signing_initialized; } static bool is_array_all_zeros(uint8_t arr[], size_t size); + private: mavlink_signing_t _mavlink_signing {}; + mavlink_status_t *_mavlink_status{nullptr}; + + bool _is_signing_initialized{false}; /** - * Checks whether the key has been initialized + * Wire or unwire the signing struct into the mavlink status based on key state. */ - bool _is_signing_initialized; + void _update_signing_state(); };