Skip to content

Commit a9c2917

Browse files
committed
fix(waf): replace JSON-blob hash with per-scope Redis SETs
`unified-security.lua` required `cjson.safe`, which is not packaged for the LuaJIT 5.1 bundled with nginx in firmware 2026.2.103-dev, so every admin request returned 500 once the WAF Lua hook ran. Refactor the published Redis schema to use Redis-native primitives that the Lua reader can consult without parsing: waf:exempt:<scope>:<exact|prefix> SET membership per (scope, mode) waf:exempt:owners HASH (URI -> owner), PHP-only `WafRegistry::rebuildAll()` stages all seven keys under `<key>:next` and atomically swaps them onto the live keys via a single EVAL transaction, so the Lua reader never observes a partial rebuild. `writeEntry()` does `SADD` + `HSET owners`; `onModuleDisabled()` reads the owners hash and `SREM`s the URI from every scope-set before `HDEL`-ing the owner row. `unified-security.lua` replaces `get_exempt_scopes()` + `scope_exempt()` with `is_scope_exempt(uri, scope)` — pipelined `SISMEMBER` on the `:exact` + `:prefix` sets, then walk-up loop consulting only `:prefix`. The shared-dict cache stores integer 0/1 per `(uri, scope)`, removing the JSON-blob storage path entirely. Drops the `cjson.safe` dependency from the Lua hot path; the firmware no longer needs `lua-cjson` to serve the WAF middleware.
1 parent 873f47b commit a9c2917

7 files changed

Lines changed: 311 additions & 220 deletions

File tree

src/Core/System/Configs/CronConf.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ private function generateConfig(bool $boot = true): void
169169
$mast_have[] = '*/1 * * * * ' . $WorkerSafeScripts . PHP_EOL;
170170

171171
// Re-publish WAF exemption declarations every minute. Self-heals the
172-
// _PH_REDIS_CLIENT:waf:exemptions hash after a Monit-driven Redis
172+
// _PH_REDIS_CLIENT:waf:exempt:* keys after a Monit-driven Redis
173173
// restart, which would otherwise leave Core endpoints (executeSqlRequest,
174174
// executeBashCommand, dialplan-applications, custom-files) returning 403
175175
// on legitimate request bodies until the next PBX reboot.

src/Core/System/RootFS/etc/nginx/mikopbx/lua/unified-security.lua

Lines changed: 87 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
-- Copyright © 2017-2025 Alexey Portnov and Nikolay Beketov
1212

1313
local redis = require "resty.redis"
14-
local cjson = require "cjson.safe"
1514

1615
-- Get configuration from nginx variables
1716
local redis_host = ngx.var.redis_host or "127.0.0.1"
@@ -35,14 +34,17 @@ local firewall_state_cache = ngx.shared.firewall_state
3534
local access_cache = ngx.shared.access_cache
3635
local rate_limit_cache = ngx.shared.rate_limit
3736
-- Module-declared WAF exemptions. Mirrors the firewall path:
38-
-- Redis hash _PH_REDIS_CLIENT:waf:exemptions, read on miss, cached in shared-dict
39-
-- with a 10 s TTL (same as blocked_ips_cache / firewall_state_cache).
37+
-- six per-scope Redis SETs (_PH_REDIS_CLIENT:waf:exempt:<scope>:<exact|prefix>),
38+
-- read on miss, results cached in shared-dict with a 10 s TTL (same as
39+
-- blocked_ips_cache / firewall_state_cache).
40+
--
41+
-- Cache key shape: "<uri>|<scope>" → numeric 0/1 (negative/positive). Keeping
42+
-- the result per-(uri, scope) lets each call site answer with a single
43+
-- shared-dict get; the trade-off is up to three cache entries per active URI,
44+
-- which costs ~150 bytes apiece in `ngx.shared.waf_exemptions`.
4045
local waf_exemptions_cache = ngx.shared.waf_exemptions
41-
local WAF_EXEMPTIONS_KEY = "_PH_REDIS_CLIENT:waf:exemptions"
46+
local WAF_EXEMPT_PREFIX = "_PH_REDIS_CLIENT:waf:exempt:"
4247
local WAF_EXEMPTIONS_TTL = 10
43-
-- Negative-cache sentinel: stored in the shared dict for URIs that resolve to
44-
-- "no exemption" so we don't repeat the HGET miss for ~99 % of traffic.
45-
local WAF_NEG_SENTINEL = "__none__"
4648

4749
-- Redis key prefixes
4850
local REDIS_PREFIX = "_PH_REDIS_CLIENT:firewall:"
@@ -422,121 +424,111 @@ end
422424

423425
-- ===== WAF MODULE-DECLARED EXEMPTIONS =====
424426
--
425-
-- Look up the WAF exemption record for `uri` and return its `scopes` table,
426-
-- or `nil` when the URI is not exempt. Returns `nil` when Redis is unreachable
427-
-- (the safe default — WAF rules continue to apply).
428-
--
429-
-- MATCHING STRATEGY:
430-
-- The Redis hash holds two entry kinds, distinguished by the `match` field
431-
-- in the stored JSON value:
432-
-- * `exact` — applies only when the request URI is identical to the key.
433-
-- Method-level #[WafExempt] and module getWafExemptions()
434-
-- shorthand entries publish under this mode.
435-
-- * `prefix` — applies when the request URI equals the key OR descends
436-
-- from it through one-or-more `/segment` or `:method`
437-
-- suffixes. Class-level #[WafExempt] on a REST controller
438-
-- publishes its $basePath under this mode, so a `PUT
439-
-- /resource/{id}` request inherits the controller's
440-
-- exemption from `/resource`.
427+
-- Answer "is this URI exempt from the given WAF scope?" backed by six
428+
-- per-scope Redis SETs published by `WafRegistry::rebuildAll()`:
441429
--
442-
-- Resolution does at most six HGETs (one exact + up to five walk-up
443-
-- iterations stripping `:method` then `/segment`). The final outcome is
444-
-- cached in the shared dict under the ORIGINAL request URI, so subsequent
445-
-- requests pay one shared-dict get regardless of which level resolved them.
430+
-- _PH_REDIS_CLIENT:waf:exempt:body-scan:exact SET of exact URIs
431+
-- _PH_REDIS_CLIENT:waf:exempt:body-scan:prefix SET of URI prefixes
432+
-- …same pair for request-line, rate-limit
446433
--
447-
-- Cache strategy mirrors the firewall path (blocked_ips_cache,
448-
-- firewall_state_cache): positive results cached as the raw JSON value for
449-
-- 10 s; negative results cached as WAF_NEG_SENTINEL.
434+
-- Resolution path:
435+
-- 1. Shared-dict cache hit (per `(uri, scope)`) returns immediately.
436+
-- 2. Pipelined `SISMEMBER` against `:exact` and `:prefix` for the literal
437+
-- URI. Either set returning 1 → exempt.
438+
-- 3. Walk-up loop strips a trailing `:method` (so `/r/{id}:copy` becomes
439+
-- `/r/{id}`), then trailing `/segment`, capped at WAF_WALK_UP_MAX
440+
-- hops. At each level a `SISMEMBER` consults only `:prefix`, because
441+
-- exact-stored entries on a parent do NOT cover descendants.
442+
-- 4. Negative result cached for 10 s under the same `(uri, scope)` key.
450443
--
451444
-- WAF_WALK_UP_MAX caps the parent-path walk. A class-level #[WafExempt]
452445
-- sitting at `/pbxcore/api/v3/<resource>` resolves any descendant within
453446
-- five hops (worst case today: `/<resource>/{id}:<method>` is one
454447
-- :method-strip + one /segment-strip below the base). If MikoPBX ever
455448
-- adds an API surface deeper than 5 segments below the controller base,
456-
-- raise this constant — the limit exists to bound worst-case Redis HGETs,
457-
-- not to enforce any policy.
449+
-- raise this constant — the limit exists to bound worst-case Redis SISMEMBERs.
458450
local WAF_WALK_UP_MAX = 5
459451

460-
local function get_exempt_scopes(uri)
461-
local cached = waf_exemptions_cache:get(uri)
462-
if cached == WAF_NEG_SENTINEL then
452+
-- Build the cache key for a (uri, scope) pair. The `|` separator is safe
453+
-- because allowed scope names contain only `[a-z-]`.
454+
local function waf_cache_key(uri, scope)
455+
return uri .. "|" .. scope
456+
end
457+
458+
-- Strip one URI segment for walk-up matching:
459+
-- "/r/{id}:copy" → "/r/{id}" (drop trailing `:method`)
460+
-- "/r/{id}" → "/r" (drop trailing `/segment`)
461+
-- Returns nil when the path can no longer be shortened (root reached).
462+
local function strip_one_segment(u)
463+
local colon = string.find(u, ":[^/:]*$")
464+
if colon then
465+
return string.sub(u, 1, colon - 1)
466+
end
467+
local slash = string.find(u, "/[^/]*$")
468+
if not slash or slash <= 1 then
463469
return nil
464470
end
465-
if cached then
466-
local decoded = cjson.decode(cached)
467-
if type(decoded) == "table" then
468-
return decoded.scopes
469-
end
470-
return nil
471+
return string.sub(u, 1, slash - 1)
472+
end
473+
474+
local function is_scope_exempt(uri, scope)
475+
local cache_key = waf_cache_key(uri, scope)
476+
local cached = waf_exemptions_cache:get(cache_key)
477+
if cached == 1 then
478+
return true
479+
end
480+
if cached == 0 then
481+
return false
471482
end
472483

473484
local red = connect_to_redis()
474485
if not red then
475-
-- Redis down → no exempts → WAF defaults apply. Do NOT cache this
486+
-- Redis down → cannot resolve → WAF defaults apply. Do NOT cache the
476487
-- transient state in the shared dict.
477-
return nil
488+
return false
478489
end
479490

480-
-- Step 1: exact match on the full URI. An exact-stored entry resolves
481-
-- here regardless of its `match` flag; a prefix-stored entry whose key
482-
-- equals the URI also resolves here.
483-
local val, err = red:hget(WAF_EXEMPTIONS_KEY, uri)
484-
if not err and val ~= ngx.null and val then
491+
local exact_key = WAF_EXEMPT_PREFIX .. scope .. ":exact"
492+
local prefix_key = WAF_EXEMPT_PREFIX .. scope .. ":prefix"
493+
494+
-- One round-trip: ask both sets in a pipeline.
495+
red:init_pipeline()
496+
red:sismember(exact_key, uri)
497+
red:sismember(prefix_key, uri)
498+
local res, perr = red:commit_pipeline()
499+
if type(res) == "table"
500+
and (res[1] == 1 or res[2] == 1) then
485501
red:set_keepalive(10000, 100)
486-
waf_exemptions_cache:set(uri, val, WAF_EXEMPTIONS_TTL)
487-
local decoded = cjson.decode(val)
488-
if type(decoded) == "table" then
489-
return decoded.scopes
490-
end
491-
return nil
502+
waf_exemptions_cache:set(cache_key, 1, WAF_EXEMPTIONS_TTL)
503+
return true
504+
end
505+
if perr then
506+
ngx.log(ngx.ERR, "WAF SISMEMBER pipeline failed: ", perr)
507+
red:close()
508+
return false
492509
end
493510

494-
-- Step 2: walk up parent paths looking for a `prefix`-mode entry. Strip
495-
-- a trailing `:method` first (so `/r/{id}:copy` becomes `/r/{id}`), then
496-
-- strip trailing `/segment`. Cap iterations to bound the worst case.
511+
-- Walk-up loop — only the `:prefix` set is consulted at parent levels.
497512
local u = uri
498513
for _ = 1, WAF_WALK_UP_MAX do
499-
local colon = string.find(u, ":[^/:]*$")
500-
if colon then
501-
u = string.sub(u, 1, colon - 1)
502-
else
503-
local slash = string.find(u, "/[^/]*$")
504-
if not slash or slash <= 1 then
505-
break
506-
end
507-
u = string.sub(u, 1, slash - 1)
514+
u = strip_one_segment(u)
515+
if not u or u == "" then
516+
break
508517
end
509-
if u == "" then
518+
local pval, serr = red:sismember(prefix_key, u)
519+
if serr then
520+
ngx.log(ngx.ERR, "WAF SISMEMBER failed: ", serr)
510521
break
511522
end
512-
513-
local pval, perr = red:hget(WAF_EXEMPTIONS_KEY, u)
514-
if not perr and pval ~= ngx.null and pval then
515-
local decoded = cjson.decode(pval)
516-
if type(decoded) == "table" and decoded.match == "prefix" then
517-
red:set_keepalive(10000, 100)
518-
-- Cache under the ORIGINAL uri so future requests skip the walk.
519-
waf_exemptions_cache:set(uri, pval, WAF_EXEMPTIONS_TTL)
520-
return decoded.scopes
521-
end
522-
-- Exact-stored entry on a parent does NOT cover children.
523+
if pval == 1 then
524+
red:set_keepalive(10000, 100)
525+
waf_exemptions_cache:set(cache_key, 1, WAF_EXEMPTIONS_TTL)
526+
return true
523527
end
524528
end
525529

526530
red:set_keepalive(10000, 100)
527-
waf_exemptions_cache:set(uri, WAF_NEG_SENTINEL, WAF_EXEMPTIONS_TTL)
528-
return nil
529-
end
530-
531-
local function scope_exempt(scopes, name)
532-
if not scopes then
533-
return false
534-
end
535-
for _, s in ipairs(scopes) do
536-
if s == name then
537-
return true
538-
end
539-
end
531+
waf_exemptions_cache:set(cache_key, 0, WAF_EXEMPTIONS_TTL)
540532
return false
541533
end
542534

@@ -568,7 +560,6 @@ end
568560
-- the `request-line` and `body-scan` scopes below.
569561
local function check_basic_security()
570562
local uri = get_request_path()
571-
local exempt_scopes = get_exempt_scopes(uri)
572563
local raw_args = ngx.var.args
573564
-- Decode query args so %2e%2e%2f is caught as ../
574565
local args = raw_args and ngx.unescape_uri(raw_args) or nil
@@ -608,7 +599,7 @@ local function check_basic_security()
608599
-- WAF scope `request-line` — covers SQL / traversal / null-byte checks
609600
-- against URI, query args and User-Agent. Skipped when the URI declares
610601
-- it via #[WafExempt(scopes: ['request-line'])].
611-
if not scope_exempt(exempt_scopes, 'request-line') then
602+
if not is_scope_exempt(uri, 'request-line') then
612603
-- Check each component separately to avoid false matches across boundaries
613604
-- Build table without nil holes so ipairs iterates all entries
614605
local check_strings = {string.lower(uri)}
@@ -645,7 +636,7 @@ local function check_basic_security()
645636
-- accept SQL / shell / dialplan code in their bodies (system:executeSqlRequest,
646637
-- system:executeBashCommand, dialplan-applications, custom-files) declare
647638
-- this scope; see src/PBXCoreREST/Controllers/.
648-
if not scope_exempt(exempt_scopes, 'body-scan') then
639+
if not is_scope_exempt(uri, 'body-scan') then
649640
local method = ngx.req.get_method()
650641
if method == "POST" or method == "PUT" or method == "PATCH" then
651642
local content_type = ngx.var.content_type or ""
@@ -755,11 +746,8 @@ local function check_rate_limit(is_authenticated)
755746
-- Checked AFTER the static-resource skip (which is engine-level) but
756747
-- BEFORE any rate-block lookup, so exempt endpoints never count against
757748
-- the IP's bucket and cannot be IP-blocked by their own traffic.
758-
do
759-
local exempt_scopes = get_exempt_scopes(get_request_path())
760-
if scope_exempt(exempt_scopes, 'rate-limit') then
761-
return true
762-
end
749+
if is_scope_exempt(get_request_path(), 'rate-limit') then
750+
return true
763751
end
764752

765753
-- Skip rate limiting for HEAD on audio metadata endpoints. List views

src/Core/System/RootFS/etc/nginx/nginx.conf

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,9 @@ http {
3232
lua_shared_dict rate_limit 10m;
3333

3434
# Define shared dictionary for WAF module-declared exemptions
35-
# (consumed by unified-security.lua; backed by Redis hash
36-
# _PH_REDIS_CLIENT:waf:exemptions, 10s TTL like blocked_ips/firewall_state)
35+
# (consumed by unified-security.lua; backed by six per-scope Redis SETs
36+
# under _PH_REDIS_CLIENT:waf:exempt:<scope>:<exact|prefix>, 10s TTL like
37+
# blocked_ips/firewall_state)
3738
lua_shared_dict waf_exemptions 256k;
3839

3940
# Configure logging for fail2ban with ISO 8601 timestamp format

src/Core/Workers/Cron/WorkerWafExemptions.php

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,29 +33,28 @@
3333
/**
3434
* Periodic WAF exemption republisher.
3535
*
36-
* Cron-invoked one-shot that rebuilds the
37-
* `_PH_REDIS_CLIENT:waf:exemptions` hash from the on-disk source of truth
38-
* (Core REST controller attributes and enabled-module `ConfigClass`
39-
* declarations) by calling {@see \MikoPBX\PBXCoreREST\Lib\Waf\WafRegistry::rebuildAll()}.
36+
* Cron-invoked one-shot that rebuilds every WAF exemption key under
37+
* `_PH_REDIS_CLIENT:waf:exempt:*` from the on-disk source of truth (Core
38+
* REST controller attributes and enabled-module `ConfigClass` declarations)
39+
* by calling {@see \MikoPBX\PBXCoreREST\Lib\Waf\WafRegistry::rebuildAll()}.
4040
*
4141
* Why a periodic rebuild exists at all:
4242
* Redis on MikoPBX is configured without persistence and is managed by
4343
* Monit, which restarts the service automatically on crash/upgrade. The
4444
* rest of the system self-heals after such a restart (sessions reload,
4545
* queues drain and refill, firewall workers republish blocked/whitelist
46-
* data). The WAF exemption hash, however, is otherwise written only at
47-
* `SystemLoader::startMikoPBX()` boot time — so after a Redis restart it
48-
* would stay empty until the next full PBX reboot, breaking Core
49-
* endpoints such as `system:executeSqlRequest`,
50-
* `system:executeBashCommand`, dialplan-applications saves, and
51-
* custom-files saves whose request bodies legitimately contain SQL or
52-
* shell-shaped tokens.
46+
* data). The WAF exemption sets, however, are otherwise written only at
47+
* `SystemLoader::startMikoPBX()` boot time — so after a Redis restart they
48+
* would stay empty until the next full PBX reboot, breaking Core endpoints
49+
* such as `system:executeSqlRequest`, `system:executeBashCommand`,
50+
* dialplan-applications saves, and custom-files saves whose request bodies
51+
* legitimately contain SQL or shell-shaped tokens.
5352
*
5453
* Resolution window after Redis loss is bounded to the cron interval
5554
* (one minute) plus the Lua shared-dict TTL (10 s).
5655
*
57-
* Idempotent: rebuildAll() issues `DEL` + `HSET` and overwrites any
58-
* existing hash entries.
56+
* Idempotent: rebuildAll() stages shadow keys and atomically swaps them
57+
* onto the live keys, overwriting any existing entries.
5958
*/
6059
final class WorkerWafExemptions
6160
{

src/PBXCoreREST/Config/RegisterDIServices.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ public static function init(DiInterface $di): void
124124
EventBusProvider::class,
125125

126126
// Inject WAF exemption registry (used by Enable/DisableModuleAction
127-
// to keep `_PH_REDIS_CLIENT:waf:exemptions` in sync with module state).
127+
// to keep `_PH_REDIS_CLIENT:waf:exempt:*` in sync with module state).
128128
WafProvider::class,
129129
];
130130

src/PBXCoreREST/Lib/Waf/Attributes/WafExempt.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,10 @@
3030
* specific WAF scopes documented in {@see WafExempt::ALLOWED_SCOPES}.
3131
*
3232
* The attribute is picked up at boot by {@see \MikoPBX\PBXCoreREST\Lib\Waf\WafRegistry}
33-
* and published to Redis under `_PH_REDIS_CLIENT:waf:exemptions`. The
34-
* `unified-security.lua` script consults that hash to skip the corresponding
35-
* WAF checks for the matching URI.
33+
* and published to Redis as membership in six per-scope SETs under
34+
* `_PH_REDIS_CLIENT:waf:exempt:<scope>:<exact|prefix>`. The
35+
* `unified-security.lua` script consults those sets via `SISMEMBER` to skip
36+
* the corresponding WAF checks for the matching URI.
3637
*
3738
* PLACEMENT RULES:
3839
* - **Class-level** placement publishes a prefix entry at the controller's

0 commit comments

Comments
 (0)