Signed 'char' passed to an 'unsigned char' parameter in 'GroupId::expandUnique'
Summary
GroupId::expandUnique walks a caller-supplied GID prefix and passes each element to util::hexCharToUInt, which takes an unsigned char. The array elements are char, so any byte at or above 0x80 arrives as a negative value and is converted at the call boundary. The GID string comes straight from a JSON-RPC client, which is free to send anything.
Details
/* src/GroupId.cc:70-84 */
70 int GroupId::expandUnique(a2_gid_t& n, const char* hex)
71 {
...
74 for (i = 0; hex[i]; ++i) {
75 unsigned int c = util::hexCharToUInt(hex[i]);/* src/util.h:249 */
249 unsigned int hexCharToUInt(unsigned char ch);hex[i] is char. Byte 0xC3 is -61 there and becomes 195 on the way into the parameter. The converted value is then used as a lookup-table index.
The table is 256 entries wide, so the index itself stays in range and I found no out-of-bounds access. What remains is that the same byte has two different values depending on which side of the call you read it from, with no cast to say so.
RPC methods that take a GID reach this: aria2.tellStatus, aria2.getFiles, aria2.getUris, aria2.getOption and the rest of the GID-keyed API. Without --rpc-secret there is no authentication in front of them.
A fix belongs at the call site:
unsigned int c = util::hexCharToUInt(static_cast<unsigned char>(hex[i]));GroupId::toNumericId has the identical line at :111 and is reached from --gid instead. it is filed separately because the two entry points are different and fixing one leaves the other.
PoC
Requires the implicit-conversion checks, which -fsanitize=undefined does not include. Build as in the #2381 (-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/). With that ignorelist a well-formed run reports nothing, so every hit is signal.
# rpc.py : drives aria2's JSON-RPC endpoint with a GID string containing bytes >= 0x80
import json, subprocess, sys, tempfile, time, urllib.request, os
BIN, PORT = sys.argv[1], 18402
w = tempfile.mkdtemp()
log = open(os.path.join(w, "a.log"), "w+b")
p = subprocess.Popen([BIN, "--enable-rpc", "--rpc-listen-all=false",
"--rpc-listen-port=%d" % PORT, "--dir=" + w, "--no-conf=true",
"--console-log-level=warn", "--summary-interval=0"],
stdout=log, stderr=subprocess.STDOUT)
def rpc(method, params):
b = json.dumps({"jsonrpc": "2.0", "id": "1", "method": method,
"params": params}).encode()
q = urllib.request.Request("http://127.0.0.1:%d/jsonrpc" % PORT, data=b,
headers={"Content-Type": "application/json"})
try:
return urllib.request.urlopen(q, timeout=5).read()
except Exception as e:
return str(e).encode()
for _ in range(80):
if b"result" in rpc("aria2.getVersion", []):
break
time.sleep(0.25)
gid = bytes([0xff, 0xfe]).decode("latin-1") # non-ASCII, so JSON encodes it as UTF-8
for m in ("aria2.tellStatus", "aria2.getFiles", "aria2.getOption", "aria2.getUris"):
rpc(m, [gid])
time.sleep(1)
p.terminate(); p.wait(timeout=20)
log.flush(); log.seek(0)
print(log.read().decode("utf8", "replace"))UBSAN_OPTIONS=print_stacktrace=1 python3 rpc.py ./src/aria2csrc/GroupId.cc:75:42: runtime error: implicit conversion from type 'char' of value -61
(8-bit, signed) to type 'unsigned char' changed the value to 195 (8-bit, unsigned)
#0 aria2::GroupId::expandUnique(unsigned long&, char const*) src/GroupId.cc:75:42
[...]
SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change src/GroupId.cc:75:42The same run also reports JsonParser.cc:640-641, because the request body passes through the JSON parser before the GID reaches expandUnique. That is a separate issue.
Impact
Implementation-defined conversion at a call boundary, on unauthenticated remote input.
Source: aria2/aria2