Integer truncation in 'Option::getAsInt'

Summary

NumberOptionHandler validates option values against int64_t bounds. Option::getAsInt, which is how most call sites read those values back, returns strtol's long truncated into an int32_t. For any option whose handler has no int32-sized maximum, the number that passed validation and the number the program uses are different. --dscp=9223372036854775807 validates as "at least 0" and comes back as -1. Settable locally and through RPC aria2.changeGlobalOption.

Details

117  int32_t Option::getAsInt(PrefPtr pref) const
118  {
119    const std::string& value = get(pref);
120    if (value.empty()) {
121      return 0;
122    }
123    else {
124      return strtol(value.c_str(), nullptr, 10);
125    }
126  }

strtol returns long, 64-bit on LP64. The return type is int32_t. The narrowing at :124 is implicit and silent.

The validating side is 64-bit:

/* src/OptionHandlerImpl.h:77-81 */
class NumberOptionHandler : public AbstractOptionHandler {
private:
  int64_t min_;
  int64_t max_;

Handlers registered without a meaningful upper bound are the ones that matter. --dscp is registered with a minimum of 0 and no maximum (src/OptionHandlerFactory.cc:666):

666      OptionHandler* op(new NumberOptionHandler(PREF_DSCP, TEXT_DSCP, "0", 0));

--bt-max-open-files behaves the same way. The truncated value is then consumed directly; for --dscp the consumer is SocketCore::setIpDscp (src/Context.cc:224), which shifts it left by two bit shift of a negative value, filed separately.

A fix belongs in getAsInt. Either widen the return type to int64_t, or range-check the strtol result and reject out-of-range values instead of narrowing them silently.

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/).

for O in --dscp=9223372036854775807 --dscp=4294967296 --bt-max-open-files=2147483648; do
  UBSAN_OPTIONS=print_stacktrace=1 ./src/aria2c "$O" --dry-run=true --max-tries=1 \
    --dir=/tmp/poc --no-conf=true http://127.0.0.1:1/x
done

=== --dscp=9223372036854775807
src/Option.cc:124:12: runtime error: implicit conversion from type 'long' of value
    9223372036854775807 (64-bit, signed) to type 'int32_t' (aka 'int')
    changed the value to -1 (32-bit, signed)
    #0 aria2::Option::getAsInt(aria2::Pref const*) const  src/Option.cc:124:12
    #1 aria2::Context::Context(...)                       src/Context.cc:224
    [...]
SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation src/Option.cc:124:12

src/SocketCore.h:147:22: runtime error: left shift of negative value -1
    #0 aria2::SocketCore::setIpDscp(int)                  src/SocketCore.h:147:22

=== --dscp=4294967296
src/Option.cc:124:12: ... changed the value to 0 (32-bit, signed)

=== --bt-max-open-files=2147483648
src/Option.cc:124:12: ... changed the value to -2147483648 (32-bit, signed)

Impact

Integer truncation between an option's validation and its use. Local configuration or an RPC client with changeGlobalOption access, not a remote network peer.