Skip to content

Commit e9d8e44

Browse files
committed
Checks case of inbound commitments before comparison.
Before this change, a malicious taker could send the same commitment with different case/formatting, and it would not be rejected as reuse, which is the intended behaviour. The commitment is defined by the H(P2) value, not the string. Reported by @m0wer
1 parent ce32baf commit e9d8e44

File tree

2 files changed

+67
-3
lines changed

2 files changed

+67
-3
lines changed

src/jmdaemon/daemon_protocol.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,16 +81,25 @@ def check_utxo_blacklist(commitment, persist=False):
8181
else return True.
8282
If flagged, persist the usage of this commitment to the above file.
8383
"""
84-
#TODO format error checking?
84+
# Fix DOS potential: should check what is given as a value, not as a string;
85+
# but since it's passed as a string, the format must be canonical:
86+
try:
87+
bytes.fromhex(commitment)
88+
except ValueError:
89+
return False
90+
# note: bytes.fromhex allows whitespace, so remove it
91+
commitment = commitment.strip().lower()
92+
8593
fname = blacklist_location
8694
if os.path.isfile(fname):
8795
with open(fname, "rb") as f:
88-
blacklisted_commitments = [x.decode('ascii').strip() for x in f.readlines()]
96+
blacklisted_commitments = [x.decode('ascii').strip().lower() for x in f.readlines()]
8997
else:
9098
blacklisted_commitments = []
9199
if commitment in blacklisted_commitments:
92100
return False
93101
elif persist:
102+
# persisted commitments guaranteed to be canonical format
94103
blacklisted_commitments += [commitment]
95104
with open(fname, "wb") as f:
96105
f.write('\n'.join(blacklisted_commitments).encode('ascii'))
@@ -843,8 +852,10 @@ def on_commitment_transferred(self, nick, commitment):
843852
with a commitment to announce in public (obfuscation of source).
844853
We simply post it in public (not affected by whether we ourselves
845854
are *accepting* commitment broadcasts.
855+
Note that we canonicalize the format here; should not logically
856+
be necessary because it's done in check_utxo_blacklist, but just in case.
846857
"""
847-
self.mcc.pubmsg("!hp2 " + commitment)
858+
self.mcc.pubmsg("!hp2 " + commitment.strip().lower())
848859

849860
@maker_only
850861
def on_push_tx(self, nick, tx):

test/jmdaemon/test_daemon_protocol.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,3 +326,56 @@ def test_waiter(self):
326326
def _called_by_deffered(self):
327327
global end_early
328328
end_early = False
329+
330+
def test_check_utxo_blacklist_case_and_whitespace(tmp_path):
331+
"""Regression test: a commitment that has already been used must not
332+
be accepted again, regardless of hex-digit case or surrounding
333+
whitespace. Before the fix, check_utxo_blacklist did a case-sensitive
334+
string comparison, so a taker could reuse a commitment by sending it
335+
in a different case.
336+
"""
337+
from jmdaemon.daemon_protocol import (check_utxo_blacklist,
338+
set_blacklist_location)
339+
340+
# Fresh, isolated commitmentlist file per test run, courtesy of the
341+
# pytest tmp_path fixture.
342+
commitmentlist = tmp_path / "commitmentlist"
343+
set_blacklist_location(str(commitmentlist))
344+
345+
# A plausible 32-byte (64 hex char) commitment.
346+
lc = "deadbeef" * 8
347+
uc = lc.upper()
348+
mixed = "DeAdBeEf" * 8
349+
padded = " " + lc + "\n"
350+
351+
# First use: accepted, and persisted to disk.
352+
assert check_utxo_blacklist(lc, persist=True) is True
353+
assert commitmentlist.is_file()
354+
355+
# Exact reuse: rejected.
356+
assert check_utxo_blacklist(lc) is False
357+
358+
# Case-variant reuse: must also be rejected. This is the regression
359+
# the fix targets — before the fix these returned True.
360+
assert check_utxo_blacklist(uc) is False
361+
assert check_utxo_blacklist(mixed) is False
362+
363+
# Whitespace-variant reuse: also rejected.
364+
assert check_utxo_blacklist(padded) is False
365+
366+
# A genuinely new commitment is still accepted.
367+
other = "cafebabe" * 8
368+
assert check_utxo_blacklist(other) is True
369+
370+
# Malformed input is rejected outright (defensive validation).
371+
assert check_utxo_blacklist("not-a-hex-string!") is False
372+
assert check_utxo_blacklist("abc") is False # odd length
373+
374+
# Legacy-on-disk path: simulate a pre-fix commitmentlist that was
375+
# written in uppercase (e.g. from an !hp2 broadcast that arrived in
376+
# uppercase under the old code). A canonical lowercase query must
377+
# still detect the reuse after the fix normalizes entries on read.
378+
legacy = tmp_path / "legacy_commitmentlist"
379+
legacy.write_bytes(("F00DF00D" * 8 + "\n").encode("ascii"))
380+
set_blacklist_location(str(legacy))
381+
assert check_utxo_blacklist("f00df00d" * 8) is False

0 commit comments

Comments
 (0)