Forum teuk.org

Full Audit & Hardening: Security, Bug Fixes, SQL Cleanup, and Live Test Suite at 67/67

in Mediabot · started by TeuK · 3w ago

TeuK · 3w ago

1. DBD::MariaDB Migration

The database driver was migrated from the legacy DBD::mysql to DBD::MariaDB throughout the entire codebase.

All DSN strings were updated from DBI:mysql: to DBI:MariaDB:, with host forced to 127.0.0.1 (MariaDB refuses port-based connections when localhost is used, falling back to Unix sockets). The deprecated mysql_insertid attribute was replaced with the standard $dbh->last_insert_id() in 11 locations across 6 files. All mysql_* connection attributes were removed or replaced with their MariaDB equivalents, and charset was moved into the DSN string directly.


2. Security Hardening — Five Phases

Phase 1 — last_insert_id() correctness

Beyond the driver migration, a subtle bug was fixed in userAdd() (Helpers.pm): last_insert_id() was being called twice — once for the USER_HOSTMASK insert and again for the return value. Since the second call happened after the hostmask insert, it was returning the wrong ID. The ID is now captured once, immediately after the INSERT INTO USER, and reused for both the hostmask registration and the return value.

Phase 2 — SQL injection via interpolation

Several queries were still building WHERE clauses via direct string interpolation (WHERE $where). These were replaced with explicit literal SQL and parameterized ? placeholders throughout Auth.pm and ChannelCommands.pm.

Phase 3 — Shell injection via open FH, "cmd $var |"

All remaining two-argument open calls with shell commands were converted to the three-argument list form (open $fh, "-|", @args), eliminating shell metacharacter injection in External.pm, Helpers.pm, and Radio.pm.

Phase 4 — youtube-dlyt-dlp

References to the deprecated youtube-dl binary were unified under yt-dlp, with the path made configurable via radio.YTDLP_PATH in the configuration file.

Phase 5 — Partyline hardening

The Partyline TCP admin interface received rate limiting (10 commands per 5 seconds), IRC nick validation, and a whitelist of allowed IRC verbs to prevent injection via the .raw command.


3. SQL Cleanup

Implicit JOINs eliminated

27 implicit JOINs of the form FROM TABLE_A, TABLE_B WHERE TABLE_A.id = TABLE_B.id were converted to explicit JOIN ... ON ... or LEFT JOIN ... ON ... syntax across 9 files. This affects query plan generation and readability.

SELECT * qualified

Over 20 SELECT * queries were replaced with explicit column lists. Existence checks were converted to SELECT 1. Where all columns are genuinely needed (e.g., User->create() building a full object), the queries were left as-is with a comment. One remaining SELECT * in User.pm was converted to an explicit column list in this session.

Parameterized LIKE

Radio.pm contained a LIKE '%' . $sSearch . '%' query built by string concatenation, with manual escaping of single quotes and semicolons. The manual escaping was removed and the pattern passed as a ? parameter, which is both safer and cleaner.


4. Bug Fixes

sth->finish leaks

Multiple database statement handles were not being closed after use. Fixed in Channel.pm (8+ accessors and setters), UserCommands.pm (mbModUser_ctx, userInfo_ctx), Hailo.pm, Helpers.pm, ChannelCommands.pm, and others. In get_user_level(), finish was being called before fetchrow_hashref, causing the method to always return the default value of 0.

Channel.pm setters — silent failure

All six setters (set_topic, set_tmdb_lang, set_key, set_description, set_chanmode, set_auto_join) called execute() without checking the return value, then updated the in-memory object attribute unconditionally. If the UPDATE failed (e.g., DB connection lost), the object state became inconsistent with the database. Each setter now checks execute() and only updates the in-memory attribute on success, logging the error otherwise.

decode_json without eval

Three decode_json calls in Helpers.pm and Radio.pm were not protected by eval {}. A malformed JSON response from an external API would crash the bot’s event loop. All are now wrapped in eval { decode_json ... } with appropriate error handling.

Bare filehandles converted to lexical

14 bare filehandles (LIQUIDSOAP_HARBOR, LIQUIDSOAP, LIQUIDSOAP2, LIQUIDSOAP_TELNET_SERVER, YOUTUBE_INFOS) in Radio.pm and PIDFILE in Mediabot.pm were converted to lexical my $fh_xxx variables declared before the unless (open ...) block (declaring inside the unless would limit scope to that block only, causing “requires explicit package name” errors).

Log.pm — nested sub declarations

sub info, sub debug, and sub error were declared syntactically inside the body of sub log (at {} depth 2). Perl treats named subs as global regardless of nesting, but this is a source of Variable will not stay shared warnings and significant confusion during code review. The three helpers were moved to package level.

userInfo_ctx — wrong signature

userInfo_ctx was defined as my ($self, $ctx) = @_ but dispatched as userInfo_ctx($ctx) (one argument). This meant $self = $ctx and $ctx = undef, causing an immediate return unless $ctx with no response sent to the user. Corrected to the standard ($ctx) signature with my $self = $ctx->bot.

Login hostmask not registered

userLogin_ctx performed UPDATE USER SET auth=1 but never inserted the caller’s hostmask into USER_HOSTMASK. After login, get_user_from_message() could not find the user by hostmask, resulting in “User not found with this hostmask” on every subsequent command. The hostmask is now registered on login (with a duplicate check).

userLogout_ctx — missing @EXPORT

The userLogout_ctx sub was defined in LoginCommands.pm but absent from @EXPORT. When Mediabot.pm called it via use Mediabot::LoginCommands, the symbol was not imported, causing a fatal “Undefined subroutine” error that crashed the Net::Async::IRC event loop with “EOF from client”.


5. Code Quality Improvements

get_user_from_message() — hostmask cache

This function performs a GROUP_CONCAT across the entire USER + USER_HOSTMASK tables on every incoming PRIVMSG. On servers with many registered users this becomes a noticeable bottleneck. A TTL-based cache (5 seconds, keyed by full IRC hostmask) was added. The cache is explicitly invalidated on login and logout via clear_user_cache(), ensuring authentication state changes are reflected immediately.

Prototypes (@) removed

95 occurrences of sub foo(@) across 12 files were removed. In modern Perl with use strict, these prototypes serve no purpose: they do not enforce types, do not affect argument passing, and can generate spurious warnings. Their presence was misleading for anyone reading the code.

Conf.pm get() — silent key miss

Conf->get() returned undef silently for any unknown configuration key, making it hard to diagnose typos or missing entries. A warn is now emitted (activatable via MEDIABOT_DEBUG_CONF=1) when a requested key does not exist in the loaded configuration.

Log level reclassification

Over 30 log messages were reclassified from level 0 (always visible) to level 1 (debug) or level 2 (verbose debug) across Helpers.pm, DBCommands.pm, ChannelCommands.pm, Mediabot.pm, UserCommands.pm, and mediabot.pl. This reduces noise in production logs while keeping the information available at higher debug levels. Messages retained at level 0 are those that should always be visible: live IRC traffic traces, fatal errors, and startup/shutdown events.


6. Live Test Suite — 67/67

A comprehensive automated live test suite was built and brought to a full pass. The suite spins up a fresh MariaDB test database, generates a configuration file, connects a spy IRC client, launches the bot as a subprocess, and runs 12 test files covering authentication, dispatch routing, channel commands, user commands, quotes, external commands, ignore/responder management, and dynamic DB commands.

Key issues resolved along the way:

  • Excess Flood disconnect: _printQuoteSyntax sends 7 NOTICE lines in rapid succession. The spy client was reading only the first, then immediately sending the next command. The remaining 6 built up in the TCP buffer and triggered the server’s flood protection, disconnecting the bot mid-suite. Fixed by draining the buffer after commands that generate multi-line responses.
  • Auth state leaking between test files: 03_auth.t performs a login but has no logout. When 05_dispatch_private.t runs its “without auth” checks, _ensure_logged_in_state reads auth=1 from the database and grants access. Fixed by sending logout at the start of 05_dispatch_private.t.
  • !q del response not matched: The deletion response "(user) done. (id: N)" did not match the test pattern expecting del|delet|remov. Changed to "(user) deleted. (id: N)".
  • userinfo mbtest causing Excess Flood: userInfo_ctx was sending 11 NOTICE lines. Compacted to 2 lines.
  • Quote ID assumption: The !q view 1 test assumed the first quote in the database always has id=1. After other test runs, IDs increment. The test now captures the actual ID from the !q add response.

7. .gitignore Updated

The .gitignore was extended to cover bot.log, bot.log.* (logrotate rotations), t/live/test.conf (auto-generated per run), and .DS_Store.


Summary

Area Changes
Driver migration DBD::mysql → DBD::MariaDB, 11× last_insert_id fix
Security 5 phases: SQL injection, shell injection, rate limiting
SQL hygiene 27 implicit JOINs, 20+ SELECT *, parameterized LIKE
Bug fixes ~25 distinct bugs across 10 modules
Code quality Cache, log levels, prototypes, Conf warn
Test suite 67/67 passing, ~220s runtime

You must be logged in to reply.