[Security] TOCTOU race condition allows duplicate victim registration in /register
Severity: HIGH
Affected file: core/user.py (/register route, lines 72-102), core/db.py (geo table schema, line 25)
Description
The /register endpoint enforces "each vId maps to exactly one victim record set" using a non-atomic check-then-act sequence:
cant = int(db.sentences_victim('count_times', vId, 3, 0)) # line 87 - CHECK
db.sentences_victim('insert_click', [vId, trape.url_to_clone, ...], 2) # line 89
db.sentences_victim('delete_networks', [vId], 2) # line 90
if cant > 0:
# UPDATE branch
...
else:
# line 96-101 - ACT: INSERT into victims, victims_data, victims_battery, geo
db.sentences_victim('insert_victim', [victimConnect, vId, time.time()], 2)
db.sentences_victim('insert_victim_data', [vId], 2)
db.sentences_victim('insert_victim_battery', [vId], 2)
db.sentences_victim('insert_victim_geo', [victimGeo, vId], 2)vId is fully attacker-controlled (taken directly from request.form['vId']). The check (count_times) and the subsequent writes are separate SQL statements with no transaction boundary and no application-level lock. There is also no UNIQUE constraint on victims, victims_data, or victims_battery — only the geo table has a PRIMARY KEY(id) (see core/db.py:25).
Because of this, two concurrent /register requests carrying the same vId can both read cant == 0 before either has committed its insert, and both take the INSERT branch.
Proof of Concept
- Send two simultaneous
POST /registerrequests with the samevId(e.g.vId=test123) and arbitrary required form fields (cpu,city,country_code2,country_name,ip,latitude,longitude,isp,country_code3,state_prov,zipcode,organization). - Both requests call
count_times('test123')before either has committedinsert_click, so both observecant == 0. - Both proceed to
INSERT INTO victims,victims_data,victims_battery, andgeofor the same id. - The first
INSERT INTO geosucceeds; the second raises an unhandledsqlite3.IntegrityError(500 error) becausegeo.idis aPRIMARY KEY. Meanwhilevictims,victims_data, andvictims_battery(which have no uniqueness constraint) now contain duplicate rows for the same vId.
Impact
- Duplicate rows in
victims/victims_data/victims_batterycorrupt downstream analytics and any logic assuming one row per vId (e.g.get_data/get_previewjoins, which useINNER JOIN geoand could behave inconsistently against duplicated victim rows). - Triggers an unhandled
IntegrityError, producing a 500 response and noisy failure for legitimate concurrent registrations (e.g., a fast page reload).
Suggested Remediation
- Wrap the check-then-act sequence in a single transaction, or use an atomic
INSERT ... ON CONFLICT(INSERT OR IGNORE/INSERT OR REPLACE) keyed onvId. - Add a
UNIQUE/PRIMARY KEYconstraint onidforvictims,victims_data, andvictims_batteryto make duplicate inserts fail safely and consistently. - Catch
sqlite3.IntegrityErroraround the insert path and fall back to the update path instead of returning an unhandled 500.
Source: jofpin/trape