Fix panic when TURN server returns non-UTF-8 bytes in STUN error reason phrase

[danielbotros]

Hi! We recently ran into a panic with the webrtc-rs crate. I wanted to share some details about it and propose a simple fix. Thanks!

Background

STUN error responses carry a human-readable reason phrase (e.g. "Unauthorized", "Stale Nonce"). In the rtc-stun crate, ErrorCodeAttribute stores this phrase as raw bytes (Vec<u8>) parsed directly off the wire.

We upgraded our CoTURN server to latest, which has a bug where non UTF-8 reason phrases can end up being sent (when include_reason_string is false, the STUN message can contain uninitialized stack bytes). This caused our SDKs, which rely on webrtc-rs, to crash due to a panic in the rtc-turn crate.

The Bug

The Display implementation for ErrorCodeAttribute attempts to decode those bytes as UTF-8, and returns Err(fmt::Error {}) if decoding failed.

let reason = match String::from_utf8(self.reason.clone()) {
            Ok(reason) => reason,
            Err(_) => return Err(fmt::Error {}),
        };

Upstream in the rtc-turn crate we try to format! the error returned by the ErrorCodeAttribute Display impl which causes a panic. get_from simply parses the raw STUN message bytes into the struct fields, so does not error on invalid UTF-8 bytes.

ex.: rtc/rtc-turn/src/client/relay.rs

pub(super) fn handle_create_permission_response(
        &mut self,
        res: Message,
        peer_addr_opt: Option<SocketAddr>,
    ) -> Result<()> {
        if let Some(relay) = self.client.relays.get_mut(&self.relayed_addr) {
            if res.typ.class == CLASS_ERROR_RESPONSE {
                let mut code = ErrorCodeAttribute::default();
                let result = code.get_from(&res);
                let err = if result.is_err() {
                    Error::Other(format!("{}", res.typ))
                } else if code.code == CODE_STALE_NONCE {
                    relay.set_nonce_from_msg(&res);
                    Error::ErrTryAgain
                } else {
                    Error::Other(format!("{} (error {})", res.typ, code))
                };
                if let Some(peer_addr) = peer_addr_opt {
                    self.client
                        .events
                        .push_back(Event::CreatePermissionError(res.transaction_id, err));
                    relay.perm_map.remove(&peer_addr);
                }
            } else if let Some(peer_addr) = peer_addr_opt
                && let Some(perm) = relay.perm_map.get_mut(&peer_addr)
            {
                perm.set_state(PermState::Permitted);
                self.client
                    .events
                    .push_back(Event::CreatePermissionResponse(
                        res.transaction_id,
                        peer_addr,
                    ));
            }

            Ok(())
        } else {
            Err(Error::ErrConnClosed)
        }
    }

Proposed Fix

In the Display impl for ErrorCodeAttribute, we use String::from_utf8_lossy instead of String::from_utf8, which replaces the invalid bytes rather than erroring.

The reason phrase is purely informational and has no effect on protocol logic, so this seemed like the least invasive fix. We expect the reason phrase to be valid UTF-8, so the loss of invalid bytes in exchange of preventing a panic seems like a good trade off.

We also considered modifying get_from to validate the UTF-8 bytes, but this is a much more disruptive change and a malformed reason phrase is not worth mishandling a potentially valid error response over.

Using Debug formatting could have also fixed this panic, but it renders reason phrases as raw byte arrays defeating the purpose of human-readable error messages for the common case where bytes are valid UTF-8.

Testing

There was no existing test file. I created one and added two new test cases to catch any regressions for this.

• test_display_valid_utf8 --> asserts valid UTF-8 reason phrase displays as expected
• test_display_invalid_utf8_does_not_panic --> asserts invalid UTF-8 reason phrase does not panic and invalid bytes are replaced