Security & integrity issues in the common PKO / Tales of Pirates source

[Panda]

Security & integrity issues in the common PKO / Tales of Pirate source

A heads-up for fellow server operators.

Most of us are running the same widely-distributed server source, which means we also inherit the same long-standing bugs. While going through the files I put together a list of security, duplication, and stability issues that are worth patching on any live server. None of these are hard to fix once you know where to look — sharing them so the whole community can harden up.

Locations are anchored to file/symbol names; exact line numbers vary between forks. I've kept this to "what and where" rather than step-by-step exploits, for obvious reasons.

---

» Economy / duplication

• Percentage_Random guaranteed drop — functions.lua. If a rate multiplier pushes the chance above 1.0 before the min(1, …) clamp, the roll always succeeds → guaranteed rare drops.
• Stall purchase integer overflow — CharStall.cpp. The total is summed in a 32-bit value before the gold check, so a large count wraps it → high-value items for almost nothing.
• Trade / bank / stall / vault dupe window — item moves happen in memory and only persist at the periodic save; a crash mid-transaction can dupe or delete items.
• Unguarded currency math — broad lack of overflow checks on price × count / gold / IMP / EXP arithmetic.
• EXP 32-bit overflow — high-level experience overflows its field; an x64 build does not fix it.

» Remote crash / memory safety

• HandleTable Resolve() null-deref / UAF — ToGameServer.cpp (map-switch / disconnect). A stale connection handle resolves to null and is dereferenced.
• Packet-pool exhaustion null-deref — Sender.cpp. Allocation returns null under pressure and is used unchecked.
• GroupServer KickUser null-deref — GroupServerAppServ.cpp. The "player not found" path dereferences the missing pointer.
• MoveCity birthpoint null-deref — Character.cpp. GetRandBirthPoint() can return null for an unknown destination and is dereferenced without a check.
• Uninitialized-record UB — the .bin record loaders bake uninitialized memory into records (e.g. CChaRecord scaling fields); a malformed .bin can also truncate a size_t → int size field.
  
  » Anti-cheat / client trust
  • No movement-speed or teleport validation — the server trusts client-supplied waypoints → speed-hack / position-teleport.
  • Skill range checked after the effect, not before → out-of-range skill use.
  • No rate-limiting on move / skill / trade / stall — only chat is throttled.
  • Predictable / racy RNG — rand() and Lua math.random() are shared across threads without seeding discipline.
  
  » Auth / session
  • Non-constant-time password comparison, and no per-account brute-force lockout (only a per-IP login cap).
  • Stuck login_status flag — no watchdog/timeout; a ghost session or double-login race can lock an account out until the flag is cleared.
  • Auto-ban lockout loop — a stuck kick cycle can set the ban flag on a legitimate account.
  • Debug commands leak server state — status/ping-style debug commands can return metrics and locate characters when not gated behind a verified GM check.
  
  » Data integrity
  • Kitbag checksum failure = total inventory loss — Kitbag.cpp (String2KitbagData). One corrupted inventory blob loses the whole bag and can block login.
  • Blob-stored inventory / bank — no per-item GUID or audit trail, so dupes are undetectable and overflow truncates silently.
  
  » Long-uptime correctness
  • GetTickCount() 32-bit wrap (~49.7 days) — cooldowns, buffs, and timers misbehave after the counter wraps on long-running worlds.
  
  » Codebase hygiene
  • No DB-outage handling — a synchronous DB connection with no timeout/circuit-breaker; a dropped database hangs the world.
  • No log rotation/retention in the default logging path → unbounded growth.
  • Aging bundled dependencies — EOL libraries with known CVEs on the legacy build path; unpinned versions.
  
  

  ---

  
  
  Bottom line: most of these are a few lines each — add the missing null checks, clamp the RNG and the currency math, validate movement/skill input server-side, and put bounds on the record loaders. The duplication and economy ones (the first few) are the ones I'd prioritize if you run an open economy.
  

---