PATCH: Disallow a netmask of zero unless the IP is also all zeroes
I ran into this alarming mistake again the other day. Luckily it was on a
dev system. Someone sees an entry in a pg_hba.conf that looks like this:
host all all 0.0.0.0/0 md5
They are gobsmacked when they learn this means to let everyone in. So they
fix it by adding new entries that look like this:
host all all 10.2.55.4/0 md5
host all all 10.2.55.5/0 md5
host all all 10.2.55.6/0 md5
It should, of course, be:
host all all 10.2.55.4/32 md5
I say "of course" but few people (even tech ones) know the distinction.
(Nor should they have to! But that's for a nearby thread). This patch aims
to prevent this very bad footgun by only allowing a /0 if the IP consists
of only zeroes. It works for ipv4 and ipv6.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
Attachments:
0001-Only-allow-a-CIDR-mask-of-zero-if-the-IP-contains-only-zeroes.patchapplication/x-patch; name=0001-Only-allow-a-CIDR-mask-of-zero-if-the-IP-contains-only-zeroes.patchDownload
From 9ba3d75999da4c2dfe258516cefa6343851d8955 Mon Sep 17 00:00:00 2001
From: Greg Sabino Mullane <greg@turnstep.com>
Date: Tue, 11 Feb 2025 11:16:11 -0500
Subject: [PATCH] Only allow a CIDR mask of zero if the IP contains only
zeroes.
Prevents a common error of not realizing the security implications of 1.2.3.4/0
---
src/backend/libpq/hba.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 510c9ffc6d7..95ff890d461 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1600,6 +1600,26 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel)
token->string);
return NULL;
}
+
+ /*
+ * Throw an error if we are putting a /0 on a non-zero IP
+ * address
+ */
+ if (strspn(cidr_slash + 1, "0") == strlen(cidr_slash + 1)
+ && strcspn(token->string, "123456789abcdefABCDEF") != strlen(token->string))
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid CIDR mask in address \"%s\"",
+ token->string),
+ errhint("A mask of 0 will allow ALL IP addresses."),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, file_name)));
+ *err_msg = psprintf("invalid CIDR mask in address \"%s\"",
+ token->string);
+ return NULL;
+ }
+
parsedline->masklen = parsedline->addrlen;
pfree(str);
}
--
2.30.2
Greg Sabino Mullane <htamfids@gmail.com> writes:
I say "of course" but few people (even tech ones) know the distinction.
(Nor should they have to! But that's for a nearby thread). This patch aims
to prevent this very bad footgun by only allowing a /0 if the IP consists
of only zeroes. It works for ipv4 and ipv6.
More generally, should we reject if the netmask causes *any* nonzero
IP bits to be ignored? Our CIDR type already imposes that rule:
regression=# select '1.2.3.4/24'::cidr;
ERROR: invalid cidr value: "1.2.3.4/24"
LINE 1: select '1.2.3.4/24'::cidr;
^
DETAIL: Value has bits set to right of mask.
I'm a bit distressed to realize that hba.c isn't using cidr_in.
Maybe we should try to share code instead of duplicating yet more.
regards, tom lane
On 11 Feb 2025, at 21:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm a bit distressed to realize that hba.c isn't using cidr_in.
Maybe we should try to share code instead of duplicating yet more.
+1. I have a note along these lines on my never-shrinking TODO, I think it
would be great if we took a stab at that.
--
Daniel Gustafsson
On Tue, Feb 11, 2025 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
More generally, should we reject if the netmask causes *any* nonzero
IP bits to be ignored? Our CIDR type already imposes that rule:
Yeah, I like that idea a lot. That's a great DETAIL message.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
On 2/11/25 9:25 PM, Tom Lane wrote:
Greg Sabino Mullane <htamfids@gmail.com> writes:
I say "of course" but few people (even tech ones) know the distinction.
(Nor should they have to! But that's for a nearby thread). This patch aims
to prevent this very bad footgun by only allowing a /0 if the IP consists
of only zeroes. It works for ipv4 and ipv6.More generally, should we reject if the netmask causes *any* nonzero
IP bits to be ignored? Our CIDR type already imposes that rule:
+1 From me too. I think we should fix the general issue rather than
special casing /0.
Andreas