Visitar URL original
pgconn: add ParseConfigOptions.ConnStringAllowedKeys · jackc/pgx@0639b37 · GitHub
Skip to content

Commit 0639b37

Browse files
sean-jackc
sean-
authored and
jackc
committed
pgconn: add ParseConfigOptions.ConnStringAllowedKeys
If any portion of the DSN is attacker-influenced (an app builds postgres://host/db?<user_supplied_options>, or a tenant supplies a connection URL), the attacker can set servicefile=, sslkey=, sslcert=, sslrootcert=, or passfile= to read arbitrary local files or, via servicefile, redirect the connection to a host they choose. libpq deliberately does not accept servicefile from the connection string for this reason; pgx's divergence has been documented but remains a footgun. Add ParseConfigOptions.ConnStringAllowedKeys, a caller-supplied allow-list of parameter keys permitted in connString. Any key outside the list causes ParseConfigWithOptions to fail before any filesystem or network access. Environment variables and defaults are unaffected. The default (nil) preserves existing behaviour. Fails closed: future libpq keywords that pgconn learns to parse are rejected unless the caller allow-lists them. Also consolidate the three duplicated {"dbname": "database"} alias maps in config.go into a package-level connStringKeyAliases map plus a canonicalConnStringKey() helper used by all parsers and by the new check. Reported by CrowdStrike (F08/F09).
1 parent b28e65b commit 0639b37

2 files changed

Lines changed: 131 additions & 24 deletions

File tree

‎pgconn/config.go‎

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,45 @@ type Config struct {
109109
createdByParseConfig bool // Used to enforce created by ParseConfig rule.
110110
}
111111

112+
// connStringKeyAliases maps libpq parameter keywords to the canonical key names this package
113+
// uses internally in the parsed-settings map. Most keywords are already canonical; this map
114+
// holds only those whose pgx-internal name differs from the libpq spelling.
115+
var connStringKeyAliases = map[string]string{
116+
"dbname": "database",
117+
}
118+
119+
// canonicalConnStringKey returns the canonical settings-map key for a libpq parameter keyword.
120+
func canonicalConnStringKey(k string) string {
121+
if c, ok := connStringKeyAliases[k]; ok {
122+
return c
123+
}
124+
return k
125+
}
126+
112127
// ParseConfigOptions contains options that control how a config is built such as GetSSLPassword.
113128
type ParseConfigOptions struct {
114129
// GetSSLPassword gets the password to decrypt a SSL client certificate. This is analogous to the libpq function
115130
// PQsetSSLKeyPassHook_OpenSSL.
116131
GetSSLPassword GetSSLPasswordFunc
132+
133+
// ConnStringAllowedKeys, if non-nil, restricts which parameter keys may appear in connString
134+
// itself. Any other key (whether connString is in keyword/value or URL form) causes
135+
// ParseConfigWithOptions to return an error before any filesystem access or network
136+
// resolution is attempted. Environment variables (PGHOST, PGSERVICEFILE, ...) and built-in
137+
// defaults are not checked: only keys that originate from the connString argument.
138+
//
139+
// Keys may be given in either their libpq spelling ("dbname") or pgx-internal spelling
140+
// ("database"); both are accepted.
141+
//
142+
// A nil slice (the default) applies no restriction and matches libpq behaviour. An empty
143+
// non-nil slice rejects every key, i.e. connString must be empty.
144+
//
145+
// Use this when any part of connString is built from input the application does not fully
146+
// control (tenant configuration, RPC parameters, admin UI fields). List only the keys that
147+
// input is expected to supply. This fails closed: a future libpq parameter that pgconn learns
148+
// to parse will be rejected unless the application has explicitly allowed it, rather than
149+
// silently passing through.
150+
ConnStringAllowedKeys []string
117151
}
118152

119153
// Copy returns a deep copy of the config that is safe to use and modify.
@@ -298,6 +332,18 @@ func ParseConfigWithOptions(connString string, options ParseConfigOptions) (*Con
298332
}
299333
}
300334

335+
if options.ConnStringAllowedKeys != nil {
336+
allowed := make(map[string]struct{}, len(options.ConnStringAllowedKeys))
337+
for _, k := range options.ConnStringAllowedKeys {
338+
allowed[canonicalConnStringKey(k)] = struct{}{}
339+
}
340+
for k := range connStringSettings {
341+
if _, ok := allowed[k]; !ok {
342+
return nil, &ParseConfigError{ConnString: connString, msg: fmt.Sprintf("connection string key %q is not in ConnStringAllowedKeys", k)}
343+
}
344+
}
345+
}
346+
301347
settings := mergeSettings(defaultSettings, envSettings, connStringSettings)
302348
if service, present := settings["service"]; present {
303349
serviceSettings, err := parseServiceSettings(settings["servicefile"], service)
@@ -615,16 +661,8 @@ func parseURLSettings(connString string) (map[string]string, error) {
615661
settings["database"] = database
616662
}
617663

618-
nameMap := map[string]string{
619-
"dbname": "database",
620-
}
621-
622664
for k, v := range parsedURL.Query() {
623-
if k2, present := nameMap[k]; present {
624-
k = k2
625-
}
626-
627-
settings[k] = v[0]
665+
settings[canonicalConnStringKey(k)] = v[0]
628666
}
629667

630668
return settings, nil
@@ -639,10 +677,6 @@ var asciiSpace = [256]uint8{'\t': 1, '\n': 1, '\v': 1, '\f': 1, '\r': 1, ' ': 1}
639677
func parseKeywordValueSettings(s string) (map[string]string, error) {
640678
settings := make(map[string]string)
641679

642-
nameMap := map[string]string{
643-
"dbname": "database",
644-
}
645-
646680
// Trim any leading whitespace so that the loop exits cleanly when only
647681
// spaces remain (e.g. trailing spaces after the last value).
648682
s = strings.TrimLeft(s, " \t\n\r\v\f")
@@ -693,9 +727,7 @@ func parseKeywordValueSettings(s string) (map[string]string, error) {
693727
s = strings.TrimLeft(s[end+1:], " \t\n\r\v\f")
694728
}
695729

696-
if k, ok := nameMap[key]; ok {
697-
key = k
698-
}
730+
key = canonicalConnStringKey(key)
699731

700732
if key == "" {
701733
return nil, errors.New("invalid keyword/value")
@@ -721,16 +753,9 @@ func parseServiceSettings(servicefilePath, serviceName string) (map[string]strin
721753
return nil, fmt.Errorf("unable to find service: %v", serviceName)
722754
}
723755

724-
nameMap := map[string]string{
725-
"dbname": "database",
726-
}
727-
728756
settings := make(map[string]string, len(service.Settings))
729757
for k, v := range service.Settings {
730-
if k2, present := nameMap[k]; present {
731-
k = k2
732-
}
733-
settings[k] = v
758+
settings[canonicalConnStringKey(k)] = v
734759
}
735760

736761
return settings, nil

‎pgconn/config_allowedkeys_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package pgconn_test
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"github.com/jackc/pgx/v5/pgconn"
8+
)
9+
10+
func TestParseConfigConnStringAllowedKeys(t *testing.T) {
11+
t.Parallel()
12+
13+
for _, tt := range []struct {
14+
name string
15+
connString string
16+
allowed []string
17+
wantErrKey string // empty means parse must succeed
18+
}{
19+
{
20+
name: "nil slice applies no restriction",
21+
connString: "host=h user=u dbname=d servicefile=/etc/passwd",
22+
allowed: nil,
23+
},
24+
{
25+
name: "allowed keys pass with dbname alias",
26+
connString: "user=u dbname=d",
27+
allowed: []string{"user", "dbname"},
28+
},
29+
{
30+
name: "allowed keys pass with canonical database",
31+
connString: "user=u dbname=d",
32+
allowed: []string{"user", "database"},
33+
},
34+
{
35+
name: "servicefile rejected",
36+
connString: "user=u dbname=d servicefile=/etc/passwd",
37+
allowed: []string{"user", "dbname"},
38+
wantErrKey: "servicefile",
39+
},
40+
{
41+
name: "host override rejected",
42+
connString: "dbname=d host=attacker.example",
43+
allowed: []string{"dbname"},
44+
wantErrKey: "host",
45+
},
46+
{
47+
name: "sslmode downgrade rejected",
48+
connString: "dbname=d sslmode=disable",
49+
allowed: []string{"dbname"},
50+
wantErrKey: "sslmode",
51+
},
52+
{
53+
name: "url form host rejected",
54+
connString: "postgres://u@attacker.example/d",
55+
allowed: []string{"user", "dbname"},
56+
wantErrKey: "host",
57+
},
58+
{
59+
name: "empty non-nil slice rejects everything",
60+
connString: "dbname=d",
61+
allowed: []string{},
62+
wantErrKey: "database",
63+
},
64+
} {
65+
t.Run(tt.name, func(t *testing.T) {
66+
opts := pgconn.ParseConfigOptions{ConnStringAllowedKeys: tt.allowed}
67+
_, err := pgconn.ParseConfigWithOptions(tt.connString, opts)
68+
if tt.wantErrKey == "" {
69+
if err != nil {
70+
t.Fatalf("unexpected error: %v", err)
71+
}
72+
return
73+
}
74+
if err == nil {
75+
t.Fatalf("expected error mentioning %q, got nil", tt.wantErrKey)
76+
}
77+
if !strings.Contains(err.Error(), tt.wantErrKey) {
78+
t.Fatalf("error %q does not mention key %q", err, tt.wantErrKey)
79+
}
80+
})
81+
}
82+
}

0 commit comments

Comments
 (0)