`switch_stun_packet_attribute_get_xor_mapped_address()` read and
XOR-rewrote a 16-byte IPv6 address whenever the `family` byte was 2,
without checking the attribute value was that long. Reject `family == 2`
values shorter than `sizeof(switch_stun_ipv6_t)` and clear the output
address and port. Adds a regression test.
Bounds and termination fixes across the STUN attribute receive path
in `handle_ice` and `switch_stun_lookup`:
- `switch_stun_packet_next_attribute` and its `_hbo` variant now
confirm the 4-byte attribute header is fully within `end` before
dereferencing `type`/`length`, and include the header when checking
that the value fits, so a truncated or overrunning attribute is not
read past the buffer.
- Compute `end_buf` as the 20-byte STUN header plus the attribute
section (`SWITCH_STUN_PACKET_MIN_LEN + header.length`) so the walk
covers every attribute, including trailing ones.
- Make `switch_stun_packet_next_attribute` the sole loop terminator
and drop the redundant `xlen` guard; its seed differed between the
two functions and could skip a trailing attribute in
`switch_stun_lookup`.
- `switch_stun_packet_attribute_get_username` reserves a byte for the
terminator and always NUL-terminates, since callers use the result
as a C string.
switch_b64_encode wrote each base64 character before checking the
output bound, and computed that bound as `bytes >= (int)olen - 1`.
Both are unsafe:
- Writing before the check meant a buffer with no room for a data
byte still received one: `olen == 1` wrote a character at index 0
and the NUL at index 1, and `olen == 0` (where `(int)olen - 1` is
-1) wrote two bytes, both past the end.
- Casting the unsigned `olen` to `int` truncated it above INT_MAX,
producing a wrong bound for very large buffers.
Reject `olen == 0`, make the output index `bytes` a `size_t`, and test
`bytes + 1 >= olen` before each write, so the bound never casts or
underflows and always leaves the last byte for the terminator. Apply
the same `bytes + 1 < olen` guard to the trailing partial-group
character and the `=` padding, so no write can pass the end.
Remove the dead line-wrap counter `y`: it only gated a commented-out
newline emit, so it counted and reset with no effect on the output.
Add unit tests covering the output-bound edges for every write site.
switch_b64_decode bounded its writes with `ol >= olen - 1`, where
`olen` is unsigned. An `olen` of 0 made `olen - 1` wrap to `SIZE_MAX`,
so the bound never fired and the loop wrote the entire decoded input
plus a trailing NUL past the destination. Reject `olen == 0` and test
`ol + 1 >= olen` before each write, so the comparison never subtracts
from an unsigned and always leaves room for the terminator.
The alphabet lookup table `l64` was a `char` indexed by a `char`,
which is unsafe whichever way `char` is signed:
- Where `char` is signed, an input byte >= 0x80 became a negative
index and read before the table.
- Where `char` is unsigned, the `-1` "not in alphabet" sentinel was
stored as 255, so the skip test never matched and non-alphabet
bytes were folded in as data.
Make `l64` a `signed char` and index it with `(unsigned char)`, so the
sentinel survives and the index stays in range on every platform.
Change the bit accumulator `b` from `int` to `unsigned int` to avoid
signed-overflow undefined behavior on long input; decoded output is
unchanged.
Add unit tests: an encode/decode round-trip across the padding cases,
the output-bound edges (`olen` of 0, 1, and a truncating buffer), and
a non-alphabet byte (including one >= 0x80) that must be skipped.
* Merge commit from fork
* [core] Fix XML escape encoder overrun and unsigned-char UTF-8 gate
`switch_xml_ampencode()` had two independent defects in its UTF-8
numeric-escape path.
Buffer overrun: the encoder grows its destination once per source byte,
but the realloc margin reserved only the 10 data characters of the
widest escape `"&#x%X;"` (a 21-bit code point rendered as 6 hex digits),
not the terminating NUL that `sprintf` also writes. At the margin
boundary that NUL landed one byte past the allocation. Reserve 11 bytes
in the guard (10 data chars plus the NUL) and emit the escape with
`snprintf` bounded to the remaining space, so the write stays in bounds
even if the margin is ever miscounted. The other escape sinks are all
within the widened margin and are unchanged.
Char signedness: the lead-byte test `(*s >> 8) & 0x01` reads bit 8 of a
plain `char`, which exists only after sign extension. Where `char` is
signed the high bit sign-extends and the test passes; where `char` is
unsigned it is always zero, so the numeric-escape path never ran and
multi-byte UTF-8 was emitted raw, making serialized XML differ by
architecture. Test bit 7 directly with `(*s & 0x80)`, correct regardless
of `char` signedness. This also makes the overrun fix effective on
unsigned-`char` builds, where the escape path now runs.
Add unit test `test_utf_8_wide_codepoint`, which serializes U+10FFFF and
long runs of it across buffer reallocations, sweeping an ASCII prefix so
an escape is emitted at the minimum-headroom offset, and asserts the
full serialized length.
`switch_core_media_add_crypto()` parses the SDP `a=crypto` keysalt and
decodes it into a fixed-size key buffer. Enforce the buffer contract at
the call site:
- Reject a zero-length, negative, or over-long keysalt (the length
check now has both a lower bound and an upper bound against the copy
buffer).
- Copy the keysalt token into a NUL-terminated buffer and decode from
that, so `switch_b64_decode` stops at the token instead of reading on
into the following key material (it consumes input to the NUL and
skips non-base64 bytes).
- Pass the destination buffer size as the decode bound rather than the
parsed token length.
- Require the decoded length to cover the crypto suite's key+salt so the
subsequent copy cannot read past the decoded bytes.
Add `test_add_crypto_keysalt_bounds`, a table-driven test covering the
accepted and rejected keysalt shapes across suites (AES-128/192/256),
including RFC 4568 lifetime/MKI and multi-key lines.
The RTCP NACK feedback handler used the packet's header length field as the
loop bound instead of the byte count the caller already validated. An
oversized length ran the loop past the fixed-size RTCP buffer, an
out-of-bounds access.
Clamp the entry count to the feedback words that fit in the received packet.
Add an end-to-end test feeding an oversized-length NACK through the RTCP
reader.
The positive-offset branch of `${variable:offset}` expansion advanced
`sub_val` past the cloned heap buffer without bounds-checking `offset`
against `strlen(sub_val)`. Subsequent `strlen()` and `strcat()` then
read adjacent heap memory into the output buffer.
* [Core, mod_commands] Interface allowlist (#3086)
* [core] Add interface allowlist to gate module app/api registration
Adds an optional, presence-activated allowlist in switch.conf.xml that
controls which modules may register application / api / json_api /
chat-application interfaces. With no <interface-allowlist> configured
nothing is enforced; when at least one <allow> entry is present, only
listed interfaces register at load time and all others are refused (the
module still loads and switch_loadable_module_process still returns
SUCCESS -- the blocked interface is simply never exposed).
Entries match at three levels of precision:
mod_commands - whole module
mod_commands.system - any interface named "system"
mod_commands.system.api - a specific type (app|api|json_api|chat_app)
Enforcement lives in switch_loadable_module_process() so every module, at
boot and at runtime `load`, is subject to the same policy. This gives
operators a way to disable the "system"/"spawn" shell-exec API commands
(and equivalents) system-wide.
Also adds the `interface_allowlist_dump [modules] [plain]` API, which
walks the loaded modules and prints their interfaces in the allowlist key
format so the current state can be captured and pruned offline into config.
* [mod_commands] Add tests for the interface allowlist
New test_interface_allowlist boots the core with an active
<interface-allowlist> (conf_interface_allowlist/) that permits only a
couple of mod_commands interfaces, then loads mod_commands and verifies:
- listed commands register and run (status, version) while unlisted and
shell-exec commands are refused (system, spawn, uptime) -- refusal
surfaces as switch_api_execute returning FALSE / command-not-found,
with the command function never invoked;
- a "module.name.type" entry gates by type: the API "status" loads while
the JSON API of the same name stays blocked;
- interface_allowlist_dump prints the config format in its xml, modules
and plain variants, and reflects module capabilities (system appears in
the dump even though it was blocked from registering).
* [config] Fix interior -- in interface-allowlist comment breaking XML parse
The explanatory comment used -- as em-dash pairs. The XML parser treats
-- inside a comment as the comment close, causing an "unclosed <!--"
error that prevents the whole freeswitch.xml from parsing (boot and
reloadxml both fail). Replace the -- pairs with ordinary punctuation.
* update .gitignore
* [core] Warn when interface-allowlist section is present but parses no entries
Co-authored-by: Chris Rienzo <chris@signalwire.com>
* Merge commit from fork
Add `switch_stun_packet_verify_integrity()`, an HMAC-SHA1
MESSAGE-INTEGRITY verifier that is const and non-mutating: it runs
over a private copy of the pristine network-order packet, so the
caller's buffer and byte order stay untouched, and walks attributes
with its own unsigned bounded helper `stun_wire_attr_bounds()`
instead of the host-order iterator macros. A trailing
MESSAGE-INTEGRITY-SHA256 or FINGERPRINT after MESSAGE-INTEGRITY is
tolerated; any other trailing attribute is rejected.
Gate it in `handle_ice()` behind `ice->verify_integrity`: verify
before any ICE state is touched, keyed by message type (local
`ice->pass` for a request, remote `ice->rpass` for a response or
error response), and drop on failure. Keepalive indications carry no
MESSAGE-INTEGRITY and are ignored.
`ice->verify_integrity` is read from the `ice_verify_message_integrity`
channel variable in `switch_rtp_activate_ice()` and defaults off, so
receive-path behavior is unchanged unless it is enabled. Adds unit
tests in `tests/unit/switch_stun.c`.
* Merge commit from fork
* [core] Verify DTLS client cert against SDP fingerprint (server role)
Add opt-in verification of the client certificate when FreeSWITCH is
the DTLS server, mirroring the binding the client role already performs
on the server certificate: match the peer certificate against the SDP
`a=fingerprint` in `dtls_state_setup()`.
Selected per call by the `rtp_dtls_client_cert_verify_mode` channel
variable (`dtls_client_cert_verify_t`). An unset variable keeps the
default `DTLS_CLIENT_CERT_VERIFY_NONE`, so existing behavior is
unchanged:
- `none`: the server does not request a client certificate.
- `fingerprint`: request it (`SSL_VERIFY_PEER` + `dtls_accept_any_cert`)
and require its fingerprint to match the SDP value; a self-signed
certificate is accepted at the TLS layer and the match provides
authenticity.
- `full`: additionally let OpenSSL enforce the certificate chain.
An unrecognized mode string falls back to `fingerprint` (fail closed)
with a warning.
The mode is set per `SSL` object in `switch_rtp_add_dtls()`. Verification
fails (`DS_FAIL`, no SRTP keys derived) when the client presents no
certificate, its fingerprint does not match, or the peer advertised no
usable `a=fingerprint`: `get_evp_by_name()` returns `NULL` for a missing
or empty hash type and `switch_core_cert_extract_fingerprint()` rejects a
`NULL` algorithm rather than passing it to `X509_digest()`.
`conf/vanilla/vars.xml` documents the knob as a disabled example. Tests
in `tests/unit/switch_rtp.c` cover matching, mismatched, absent-cert,
absent-fingerprint, `none`, and unrecognized-mode cases.
---------
Co-authored-by: Andrey Volk <andywolk@gmail.com>
Co-authored-by: Chris Rienzo <chris@signalwire.com>
`read_packet()` passed a peer-supplied `Content-Length` straight to
`switch_zmalloc(body, clen + 1)`. Huge values drove `calloc` failure
and `switch_zmalloc` `abort()`-ed the daemon.
- Cap `Content-Length` at 16 MiB; reject negatives.
- Destroy the partially-built `*event` at the new rejection site and
at the existing body-recv failure path so callers don't leak it.
- Add `test_mod_event_socket` covering `INT_MAX`, above-cap,
negative, `atoi`-overflow, zero, and valid-non-zero-body cases.
Add `switch_stun_packet_verify_integrity()`, an HMAC-SHA1
MESSAGE-INTEGRITY verifier that is const and non-mutating: it runs
over a private copy of the pristine network-order packet, so the
caller's buffer and byte order stay untouched, and walks attributes
with its own unsigned bounded helper `stun_wire_attr_bounds()`
instead of the host-order iterator macros. A trailing
MESSAGE-INTEGRITY-SHA256 or FINGERPRINT after MESSAGE-INTEGRITY is
tolerated; any other trailing attribute is rejected.
Gate it in `handle_ice()` behind `ice->verify_integrity`: verify
before any ICE state is touched, keyed by message type (local
`ice->pass` for a request, remote `ice->rpass` for a response or
error response), and drop on failure. Keepalive indications carry no
MESSAGE-INTEGRITY and are ignored.
`ice->verify_integrity` is read from the `ice_verify_message_integrity`
channel variable in `switch_rtp_activate_ice()` and defaults off, so
receive-path behavior is unchanged unless it is enabled. Adds unit
tests in `tests/unit/switch_stun.c`.
Cap `Content-Length` at `HTTP_POST_MAX_BODY` (10 MiB) and size the
allocation to the actual body length (`content_length + 1` for
the trailing NUL).
Also fix `WS_BLOCK` units — `kws_raw_read` takes ms, set to 10000.
Route IPv6 writes in `switch_stun_packet_attribute_add_binded_address`
and `switch_stun_packet_attribute_add_xor_binded_address` through
`switch_stun_ipv6_t` (16-byte `address[]`) instead of `switch_stun_ip_t`
(4-byte `uint32_t address`).
Add IPv4/IPv6 unit tests for both encoders.
Co-authored-by: Andrey Volk <andywolk@gmail.com>
* [Unit-tests] Add a test provoking switch_event_add_header to memory leak.
* [Core] Fix switch_event_base_add_header leaking memory when headers are with indexes.
Recent changes made it impossible to compile freeswitch without libyuv
support.
src/switch_core_video.c: In function 'switch_img_read_from_file':
src/switch_core_video.c:3139:4: error: implicit declaration of function 'RAWToI420' [-Werror=implicit-function-declaration]
RAWToI420(data, width * 3,
^
src/switch_core_video.c:3148:4: error: implicit declaration of function 'ABGRToARGB' [-Werror=implicit-function-declaration]
ABGRToARGB(data, width * 4, img->planes[SWITCH_PLANE_PACKED], img->stride[SWITCH_PLANE_PACKED], width, height);
^
Fix this my adding/moving the checks for "SWITCH_HAVE_YUV".
Signed-off-by: Sebastian Kemper <sebastian_ml@gmx.net>
Dmitry Verenitsin