commit 096cfadd1008089cdbe54d79dc12090a87e83d0c
parent b07ddd6d1ec13de17bd4c51b2836c064b4ff920c
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 09:49:18 +0200
do not die on non-UTF-8 input
Diffstat:
3 files changed, 196 insertions(+), 5 deletions(-)
diff --git a/src/challenger/meson.build b/src/challenger/meson.build
@@ -12,6 +12,7 @@ check_SCRIPTS = [
'test-challenger-pkce',
'test-challenger-revisit',
'test-challenger-badutf8',
+ 'test-challenger-pinlimit',
]
test_helper_cat = configure_file(input: 'cat.sh', output: 'cat.sh', copy: true)
@@ -28,6 +29,12 @@ test_conf = configure_file(
copy: true,
)
+test_conf = configure_file(
+ input: 'test-challenger-pinlimit.conf',
+ output: 'test-challenger-pinlimit.conf',
+ copy: true,
+)
+
foreach s : check_SCRIPTS
tscript = '@0@.sh'.format(s)
test_exe = configure_file(input: tscript, output: tscript, copy: true)
diff --git a/src/challenger/test-challenger-badutf8.sh b/src/challenger/test-challenger-badutf8.sh
@@ -0,0 +1,159 @@
+#!/usr/bin/env bash
+# This file is in the public domain.
+#
+# Tests that a /challenge upload with a field that is not valid UTF-8
+# is rejected with 400 and does NOT take the daemon down.
+#
+# Regression test: json_string() returns NULL for invalid UTF-8, and the
+# resulting json_object_set_new() failure used to be caught by a
+# GNUNET_assert(), so a single unauthenticated POST aborted
+# challenger-httpd.
+
+set -eu
+
+# Exit, with status code "skip" (no 'real' failure)
+function exit_skip() {
+ echo " SKIP: $1"
+ exit 77
+}
+
+# Exit, with error message (hard failure)
+function exit_fail() {
+ echo " FAIL: $@"
+ exit 1
+}
+
+# Cleanup to run whenever we exit
+function cleanup()
+{
+ for n in $(jobs -p)
+ do
+ kill $n 2> /dev/null || true
+ done
+ rm -f "$LAST_RESPONSE" "$FILENAME"
+ wait
+}
+
+LAST_RESPONSE=$(mktemp responseXXXXXX.log)
+FILENAME="test-challenger-badutf8.txt"
+
+# Install cleanup handler (except for kill -9)
+trap cleanup EXIT
+
+export PATH="$PATH:."
+
+echo -n "Testing for jq"
+jq -h > /dev/null || exit_skip "jq required"
+echo " FOUND"
+echo -n "Testing for curl"
+curl -h > /dev/null || exit_skip "curl required"
+echo " FOUND"
+echo -n "Testing for wget"
+wget -h > /dev/null || exit_skip "wget required"
+echo " FOUND"
+echo -n "Testing for challenger-httpd ..."
+challenger-httpd -h > /dev/null || exit_skip "challenger-httpd required"
+echo " FOUND"
+
+CONF="test-challenger.conf"
+BURL="http://localhost:9967"
+REDIRECT_URI="http://client.example.com/"
+
+echo -n "Initialize challenger database ..."
+challenger-dbinit -r -c "${CONF}" &> dbinit.log
+echo " OK"
+
+echo -n "Add challenger client ..."
+CLIENT_SECRET="secret-token:secret"
+challenger-admin -c "${CONF}" -a "${CLIENT_SECRET}" "${REDIRECT_URI}" &> admin.log
+echo " OK"
+# We just reset the DB, thus the client ID must be 1 here:
+CLIENT_ID=1
+
+echo -n "Start challenger-httpd ..."
+challenger-httpd -L INFO -c "${CONF}" &> httpd.log &
+
+# Wait for challenger to be available
+for n in $(seq 1 50)
+do
+ echo -n "."
+ sleep 0.2
+ OK=0
+ wget --tries=1 --timeout=1 "${BURL}/config" -o /dev/null -O /dev/null >/dev/null || continue
+ OK=1
+ break
+done
+if [ 1 != $OK ]
+then
+ exit_skip "Failed to launch challenger service"
+fi
+echo " OK"
+
+echo -n "Setup new validation process..."
+STATUS=$(curl "${BURL}/setup/${CLIENT_ID}" \
+ -H "Authorization: Bearer ${CLIENT_SECRET}" \
+ -d '' \
+ -w "%{http_code}" -s -o $LAST_RESPONSE)
+
+if [ "$STATUS" != "200" ]
+then
+ exit_fail "Expected 200 OK. Got: $STATUS" $(cat $LAST_RESPONSE)
+fi
+NONCE=$(jq -r .nonce < "$LAST_RESPONSE")
+echo " OK"
+
+# Note: '--data' (not '--data-urlencode') so that the '%FF' reaches the
+# server verbatim and is URL-decoded into a lone 0xFF byte, which is not
+# valid UTF-8.
+echo -n "Submitting address with a non-UTF-8 value..."
+STATUS=$(curl "${BURL}/challenge/${NONCE}" \
+ -X POST \
+ -H "Accept: application/json" \
+ --data "filename=%FF" \
+ -w "%{http_code}" -s -o $LAST_RESPONSE || echo "000")
+
+if [ "$STATUS" != "400" ]
+then
+ exit_fail "Expected 400 Bad Request. Got: $STATUS" $(cat $LAST_RESPONSE)
+fi
+echo " OK"
+
+echo -n "Checking that challenger-httpd is still alive..."
+wget --tries=1 --timeout=5 "${BURL}/config" -o /dev/null -O /dev/null \
+ || exit_fail "challenger-httpd died on a non-UTF-8 upload"
+echo " OK"
+
+echo -n "Submitting address with a non-UTF-8 field name..."
+STATUS=$(curl "${BURL}/challenge/${NONCE}" \
+ -X POST \
+ -H "Accept: application/json" \
+ --data "file%FFname=x" \
+ -w "%{http_code}" -s -o $LAST_RESPONSE || echo "000")
+
+if [ "$STATUS" != "400" ]
+then
+ exit_fail "Expected 400 Bad Request. Got: $STATUS" $(cat $LAST_RESPONSE)
+fi
+echo " OK"
+
+echo -n "Checking that challenger-httpd is still alive..."
+wget --tries=1 --timeout=5 "${BURL}/config" -o /dev/null -O /dev/null \
+ || exit_fail "challenger-httpd died on a non-UTF-8 field name"
+echo " OK"
+
+# The rejected requests must not have consumed anything: a well-formed
+# address is still accepted afterwards.
+echo -n "Submitting a well-formed address..."
+STATUS=$(curl "${BURL}/challenge/${NONCE}" \
+ -X POST \
+ -H "Accept: application/json" \
+ --data-urlencode "filename=${FILENAME}" \
+ -w "%{http_code}" -s -o $LAST_RESPONSE)
+
+if [ "$STATUS" != "200" ]
+then
+ exit_fail "Expected 200 OK. Got: $STATUS" $(cat $LAST_RESPONSE)
+fi
+echo " OK"
+
+exit 0
diff --git a/src/challengerdb/do_challenge_address.sql b/src/challengerdb/do_challenge_address.sql
@@ -39,6 +39,7 @@ AS $$
DECLARE
my_status RECORD;
my_do_update BOOL;
+ my_address_differs BOOL;
BEGIN
my_do_update = FALSE;
@@ -87,8 +88,25 @@ THEN
END IF;
out_solved=FALSE;
+-- Two addresses are the same address if they are the same JSON *value*.
+-- Comparing the raw text instead would make a purely cosmetic difference --
+-- a different field order, redundant whitespace -- count as a different
+-- address, with two consequences. Firstly, the daemon itself re-appends
+-- 'read_only' to the address as the *last* field (see
+-- challenger-httpd_challenge.c), so an unchanged address can come back here
+-- with its fields in a different order and silently cost an honest user one
+-- of their address attempts. Secondly, the budget is deliberately
+-- 'address_attempts_left' addresses times 'pin_transmissions_left' PINs
+-- each; if cosmetic variants counted as distinct addresses, all of those
+-- messages could be aimed at a single recipient. Comparing as JSONB also
+-- makes this agree with the C layer, which compares addresses with
+-- json_equal() in addr_equal().
+my_address_differs = ( (my_status.address IS NOT NULL) AND
+ (in_address::JSONB IS DISTINCT FROM
+ my_status.address::JSONB) );
+
IF ( (0 = my_status.address_attempts_left) AND
- (in_address != my_status.address) )
+ my_address_differs )
THEN
out_address_refused=TRUE;
out_last_pin=0;
@@ -96,13 +114,20 @@ THEN
END IF;
out_address_refused=FALSE;
-IF ( (my_status.address IS NULL) OR
- (in_address != my_status.address) )
+IF ( my_address_differs OR
+ (my_status.address IS NULL) )
THEN
- -- we are changing the address, update counters
+ -- We are changing the address, update counters. Refilling
+ -- 'pin_transmissions_left' and clearing the retransmission cooldown is
+ -- intentional: the user gets a fixed number of addresses and, for each
+ -- address, a fixed number of PIN transmissions, so that a user who
+ -- mistyped their address does not have to wait out the cooldown of a
+ -- message that went to somebody else.
my_status.address_attempts_left
= GREATEST(0,my_status.address_attempts_left - 1);
- my_status.address = in_address;
+ -- Store the canonical (JSONB) rendering, so that what is on file does not
+ -- depend on the field order the client happened to use.
+ my_status.address = in_address::JSONB::TEXT;
my_status.pin_transmissions_left = 3;
my_status.last_tx_time = 0;
my_do_update=TRUE;
