[PATCH BlueZ v2 00/10] Replace the name2utf8 copies with str2utf8

From: Luiz Augusto von Dentz <[email protected]>

There are five near copies of the same helper turning a remote name into
a UTF-8 string, in monitor/att.c, profiles/audio/mcp.c,
profiles/gap/gas.c, src/eir.c and src/shared/ad.c, and none of them
agree with each other.

The copy in src/eir.c does not clamp the length before copying into a
fixed size stack buffer, and eir_parse() can hand it 253 bytes for a 250
byte buffer, so a remote device can overflow it with a long name in an
advertising report. That is fixed first and on its own so it can be
backported.

The rest replaces the copies with a single str2utf8() in src/shared/util
and drops around 120 lines.

Three things change behaviour and are worth a look:

- Most of the copies truncated the name at the first ill-formed
  sequence, throwing the rest away, and the monitor replaced every
  non-ASCII byte with a space, mangling the valid part of the name.
  str2utf8() replaces only the ill-formed sequences with U+FFFD, so a
  name is no longer cut short by one bad byte in the middle. The
  unit/test-eir expectations are updated to match.

- strnlenutf8(), and with it strisutf8() and strtoutf8(), only checked
  the shape of the bytes, so it accepted overlong encodings, UTF-16
  surrogates and code points past U+10FFFF. Those reach D-Bus, which
  does validate UTF-8 and rejects them. It now validates as per table
  3-7 of the Unicode Standard.

- btd_device_set_name() cut the name at 248 bytes with strncpy(),
  without regard for where the characters start and end, so a longer
  name could be left with a partial sequence and be rejected by D-Bus.
  It now truncates on a character boundary. It also loses the doubled
  "device" in its name.

str2utf8() and strtoutf8() were checked against Python's UTF-8 decoder
over every one and two byte sequence, a sample of the three byte ones
and 200000 random inputs, with no mismatch.

Changes in v2:

- Add unit/test-eir tests for the longest local names eir_parse() can be
  handed. Nothing covered a name anywhere near the size of the buffer it
  is copied into, which is why the missing clamp went unnoticed. Run
  against the code before the first patch, the 253 byte one dies with
  "*** buffer overflow detected ***".

- Add a fix for ad_replace_name() reading past the name it copied, which
  those new tests turned up. It clamps the copy to HCI_MAX_NAME_LENGTH
  but then hands the unclamped length to strisutf8() and strtoutf8(), so
  a 253 byte name has both reading 3 bytes past the same buffer.

- Clamp the name in eir_parse() to HCI_MAX_NAME_LENGTH rather than to
  the size of the buffer, so that it agrees with ad_replace_name() and
  the new tests can expect the same name out of both.

- Add the Fixes tag to the first patch.

Luiz Augusto von Dentz (10):
  eir: Fix stack buffer overflow when parsing the remote name
  shared/ad: Fix reading past the name that was copied
  unit/test-eir: Add tests for the longest local names
  shared/util: Make strnlenutf8 reject ill-formed sequences
  shared/util: Add str2utf8
  unit/test-util: Add str2utf8 tests
  Replace the name2utf8 copies with str2utf8
  device: Fix the name truncation splitting UTF-8 sequences
  device: Rename btd_device_device_set_name to btd_device_set_name
  unit/test-util: Cover strtoutf8 with the str2utf8 tests

 monitor/att.c        |  66 +++++++--------------
 plugins/neard.c      |   2 +-
 plugins/sixaxis.c    |   2 +-
 profiles/audio/mcp.c |  24 +-------
 profiles/gap/gas.c   |  22 +------
 src/adapter.c        |   4 +-
 src/device.c         |  14 ++++-
 src/device.h         |   2 +-
 src/eir.c            |  20 +------
 src/shared/ad.c      |  20 +++----
 src/shared/util.c    | 136 ++++++++++++++++++++++++++++++++++---------
 src/shared/util.h    |   7 +++
 unit/test-eir.c      |  69 +++++++++++++++++++++-
 unit/test-util.c     | 121 ++++++++++++++++++++++++++++++++++++++
 14 files changed, 356 insertions(+), 153 deletions(-)

-- 
2.54.0