Signed 'char' truncation in 'util::fromHex'

Summary

util::fromHex assembles each decoded byte as an int and appends it to a std::string through operator+=(char). On platforms where char is signed, every decoded byte of 0x80 or above changes value — 255 becomes -1. This is not an edge case. roughly half the bytes of any real hash are above 0x7F, so an ordinary sha-1 or sha-256 hits it. fromHex is fed from metalink <hash> and <pieces> elements, from magnet xt=urn:btih:, from a Local Peer Discovery packet, and from the checksum option set over RPC.

Details

/* src/util.h:255-273 */
262    for (; first != last; first += 2) {
263      unsigned char high = hexCharToUInt(*first);
264      unsigned char low = hexCharToUInt(*(first + 1));
265      if (high == 255 || low == 255) {
266        dest.clear();
267        return dest;
268      }
269      dest += (high * 16 + low);
270    }

high and low are unsigned char, but integral promotion makes high * 16 + low an int in the range 0 to 255. dest is a std::string, so :269 selects operator+=(char). Values from 128 to 255 are not representable in a signed char and the conversion changes them.

The stored byte pattern is the same either way, so nothing downstream reads a wrong hash. What is wrong is the type: a byte value is being carried through a type that cannot hold it, and the conversion is implementation-defined rather than value-preserving.

Callers that take remote input:

A fix belongs at :269. Making the round trip explicit keeps the same bytes and states the intent:

      dest += static_cast<char>(static_cast<unsigned char>(high * 16 + low));

PoC

Requires the implicit-conversion checks, which -fsanitize=undefined does not include. Build as in the bittorrent::processRootDictionarycase (-fsanitize=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change plus the ignorelist that silences bitfield.h, base64.h, the util.h byte helpers and deps/).

<!-- h.meta4 -->
<?xml version="1.0" encoding="UTF-8"?>
<metalink xmlns="urn:ietf:params:xml:ns:metalink">
 <file name="h.bin"><size>4</size>
  <hash type="sha-1">ffffffffffffffffffffffffffffffffffffffff</hash>
  <url priority="1">http://127.0.0.1:1/h.bin</url>
 </file>
</metalink>

UBSAN_OPTIONS=print_stacktrace=1 ./src/aria2c -S h.meta4

UBSAN_OPTIONS=print_stacktrace=1 ./src/aria2c --dry-run=true --max-tries=1 \
  --dir=/tmp/poc --no-conf=true \
  'magnet:?xt=urn:btih:ffffffffffffffffffffffffffffffffffffffff'

UBSAN_OPTIONS=print_stacktrace=1 ./src/aria2c \
  --checksum=sha-1=ffffffffffffffffffffffffffffffffffffffff \
  --dry-run=true --max-tries=1 --dir=/tmp/poc --no-conf=true http://127.0.0.1:1/x

All three report the same line. The metalink trace:

src/util.h:269:13: runtime error: implicit conversion from type 'int' of value 255
    (32-bit, signed) to type 'char' changed the value to -1 (8-bit, signed)
    #0 std::string aria2::util::fromHex<...>(...)          src/util.h:269:13
    #1 aria2::MetalinkParserController::setHashOfChecksum(...)
                                        src/MetalinkParserController.cc:296
    #2 aria2::MetalinkParserStateMachine::setHashOfChecksum(...)
    #3 aria2::HashMetalinkParserStateV4::endElement(...)
    [...]
SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation src/util.h:269:13

Impact

Implementation defined integer conversion on remote input. no correctness issue, no memory corruption. Just noise by sanitizer. However, it seems advisable to fix this to improve code quality and ensure more reliable testing.