Skip to content

Commit ad5b6f2

Browse files
michaelpqmichaelpq
committed
Revert error handling improvements for cryptohashes
This reverts commits ab27df2, af8d530 and 3a0cced, that introduced pg_cryptohash_error(). In order to make the core code able to pass down the new error types that this introduced, some of the MD5-related routines had to be reworked, causing an ABI breakage, but we found that some external extensions rely on them. Maintaining compatibility outweights the error report benefits, so just revert the change in v14. Reported-by: Laurenz Albe Discussion: https://postgr.es/m/9f0c0a96d28cf14fc87296bbe67061c14eb53ae8.camel@cybertec.at
1 parent 4aee39d commit ad5b6f2

File tree

19 files changed

+88
-297
lines changed

19 files changed

+88
-297
lines changed

‎contrib/passwordcheck/passwordcheck.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ check_password(const char *username,
7474
*
7575
* We only check for username = password.
7676
*/
77-
const char *logdetail = NULL;
77+
char *logdetail;
7878

7979
if (plain_crypt_verify(username, shadow_pass, username, &logdetail) == STATUS_OK)
8080
ereport(ERROR,

‎contrib/pgcrypto/internal-sha2.c

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,7 @@ int_sha2_update(PX_MD *h, const uint8 *data, unsigned dlen)
101101
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
102102

103103
if (pg_cryptohash_update(ctx, data, dlen) < 0)
104-
elog(ERROR, "could not update %s context: %s", "SHA2",
105-
pg_cryptohash_error(ctx));
104+
elog(ERROR, "could not update %s context", "SHA2");
106105
}
107106

108107
static void
@@ -111,8 +110,7 @@ int_sha2_reset(PX_MD *h)
111110
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
112111

113112
if (pg_cryptohash_init(ctx) < 0)
114-
elog(ERROR, "could not initialize %s context: %s", "SHA2",
115-
pg_cryptohash_error(ctx));
113+
elog(ERROR, "could not initialize %s context", "SHA2");
116114
}
117115

118116
static void
@@ -121,8 +119,7 @@ int_sha2_finish(PX_MD *h, uint8 *dst)
121119
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
122120

123121
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
124-
elog(ERROR, "could not finalize %s context: %s", "SHA2",
125-
pg_cryptohash_error(ctx));
122+
elog(ERROR, "could not finalize %s context", "SHA2");
126123
}
127124

128125
static void

‎contrib/pgcrypto/internal.c

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,7 @@ int_md5_update(PX_MD *h, const uint8 *data, unsigned dlen)
8989
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
9090

9191
if (pg_cryptohash_update(ctx, data, dlen) < 0)
92-
elog(ERROR, "could not update %s context: %s", "MD5",
93-
pg_cryptohash_error(ctx));
92+
elog(ERROR, "could not update %s context", "MD5");
9493
}
9594

9695
static void
@@ -99,8 +98,7 @@ int_md5_reset(PX_MD *h)
9998
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
10099

101100
if (pg_cryptohash_init(ctx) < 0)
102-
elog(ERROR, "could not initialize %s context: %s", "MD5",
103-
pg_cryptohash_error(ctx));
101+
elog(ERROR, "could not initialize %s context", "MD5");
104102
}
105103

106104
static void
@@ -109,8 +107,7 @@ int_md5_finish(PX_MD *h, uint8 *dst)
109107
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
110108

111109
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
112-
elog(ERROR, "could not finalize %s context: %s", "MD5",
113-
pg_cryptohash_error(ctx));
110+
elog(ERROR, "could not finalize %s context", "MD5");
114111
}
115112

116113
static void
@@ -142,8 +139,7 @@ int_sha1_update(PX_MD *h, const uint8 *data, unsigned dlen)
142139
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
143140

144141
if (pg_cryptohash_update(ctx, data, dlen) < 0)
145-
elog(ERROR, "could not update %s context: %s", "SHA1",
146-
pg_cryptohash_error(ctx));
142+
elog(ERROR, "could not update %s context", "SHA1");
147143
}
148144

149145
static void
@@ -152,8 +148,7 @@ int_sha1_reset(PX_MD *h)
152148
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
153149

154150
if (pg_cryptohash_init(ctx) < 0)
155-
elog(ERROR, "could not initialize %s context: %s", "SHA1",
156-
pg_cryptohash_error(ctx));
151+
elog(ERROR, "could not initialize %s context", "SHA1");
157152
}
158153

159154
static void
@@ -162,8 +157,7 @@ int_sha1_finish(PX_MD *h, uint8 *dst)
162157
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
163158

164159
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
165-
elog(ERROR, "could not finalize %s context: %s", "SHA1",
166-
pg_cryptohash_error(ctx));
160+
elog(ERROR, "could not finalize %s context", "SHA1");
167161
}
168162

169163
static void

‎contrib/uuid-ossp/uuid-ossp.c

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -319,17 +319,14 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
319319
pg_cryptohash_ctx *ctx = pg_cryptohash_create(PG_MD5);
320320

321321
if (pg_cryptohash_init(ctx) < 0)
322-
elog(ERROR, "could not initialize %s context: %s", "MD5",
323-
pg_cryptohash_error(ctx));
322+
elog(ERROR, "could not initialize %s context", "MD5");
324323
if (pg_cryptohash_update(ctx, ns, sizeof(uu)) < 0 ||
325324
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
326-
elog(ERROR, "could not update %s context: %s", "MD5",
327-
pg_cryptohash_error(ctx));
325+
elog(ERROR, "could not update %s context", "MD5");
328326
/* we assume sizeof MD5 result is 16, same as UUID size */
329327
if (pg_cryptohash_final(ctx, (unsigned char *) &uu,
330328
sizeof(uu)) < 0)
331-
elog(ERROR, "could not finalize %s context: %s", "MD5",
332-
pg_cryptohash_error(ctx));
329+
elog(ERROR, "could not finalize %s context", "MD5");
333330
pg_cryptohash_free(ctx);
334331
}
335332
else
@@ -338,15 +335,12 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
338335
unsigned char sha1result[SHA1_DIGEST_LENGTH];
339336

340337
if (pg_cryptohash_init(ctx) < 0)
341-
elog(ERROR, "could not initialize %s context: %s", "SHA1",
342-
pg_cryptohash_error(ctx));
338+
elog(ERROR, "could not initialize %s context", "SHA1");
343339
if (pg_cryptohash_update(ctx, ns, sizeof(uu)) < 0 ||
344340
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
345-
elog(ERROR, "could not update %s context: %s", "SHA1",
346-
pg_cryptohash_error(ctx));
341+
elog(ERROR, "could not update %s context", "SHA1");
347342
if (pg_cryptohash_final(ctx, sha1result, sizeof(sha1result)) < 0)
348-
elog(ERROR, "could not finalize %s context: %s", "SHA1",
349-
pg_cryptohash_error(ctx));
343+
elog(ERROR, "could not finalize %s context", "SHA1");
350344
pg_cryptohash_free(ctx);
351345

352346
memcpy(&uu, sha1result, sizeof(uu));

‎src/backend/commands/user.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
393393
if (password)
394394
{
395395
char *shadow_pass;
396-
const char *logdetail = NULL;
396+
char *logdetail;
397397

398398
/*
399399
* Don't allow an empty password. Libpq treats an empty password the
@@ -835,7 +835,7 @@ AlterRole(AlterRoleStmt *stmt)
835835
if (password)
836836
{
837837
char *shadow_pass;
838-
const char *logdetail = NULL;
838+
char *logdetail;
839839

840840
/* Like in CREATE USER, don't allow an empty password. */
841841
if (password[0] == '\0' ||

‎src/backend/libpq/auth-scram.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ pg_be_scram_init(Port *port,
327327
*/
328328
int
329329
pg_be_scram_exchange(void *opaq, const char *input, int inputlen,
330-
char **output, int *outputlen, const char **logdetail)
330+
char **output, int *outputlen, char **logdetail)
331331
{
332332
scram_state *state = (scram_state *) opaq;
333333
int result;

‎src/backend/libpq/auth.c

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
*/
4848
static void sendAuthRequest(Port *port, AuthRequest areq, const char *extradata,
4949
int extralen);
50-
static void auth_failed(Port *port, int status, const char *logdetail);
50+
static void auth_failed(Port *port, int status, char *logdetail);
5151
static char *recv_password_packet(Port *port);
5252
static void set_authn_id(Port *port, const char *id);
5353

@@ -56,11 +56,11 @@ static void set_authn_id(Port *port, const char *id);
5656
* Password-based authentication methods (password, md5, and scram-sha-256)
5757
*----------------------------------------------------------------
5858
*/
59-
static int CheckPasswordAuth(Port *port, const char **logdetail);
60-
static int CheckPWChallengeAuth(Port *port, const char **logdetail);
59+
static int CheckPasswordAuth(Port *port, char **logdetail);
60+
static int CheckPWChallengeAuth(Port *port, char **logdetail);
6161

62-
static int CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail);
63-
static int CheckSCRAMAuth(Port *port, char *shadow_pass, const char **logdetail);
62+
static int CheckMD5Auth(Port *port, char *shadow_pass, char **logdetail);
63+
static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail);
6464

6565

6666
/*----------------------------------------------------------------
@@ -258,7 +258,7 @@ ClientAuthentication_hook_type ClientAuthentication_hook = NULL;
258258
* particular, if logdetail isn't NULL, we send that string to the log.
259259
*/
260260
static void
261-
auth_failed(Port *port, int status, const char *logdetail)
261+
auth_failed(Port *port, int status, char *logdetail)
262262
{
263263
const char *errstr;
264264
char *cdetail;
@@ -394,7 +394,7 @@ void
394394
ClientAuthentication(Port *port)
395395
{
396396
int status = STATUS_ERROR;
397-
const char *logdetail = NULL;
397+
char *logdetail = NULL;
398398

399399
/*
400400
* Get the authentication method to use for this frontend/database
@@ -780,7 +780,7 @@ recv_password_packet(Port *port)
780780
* Plaintext password authentication.
781781
*/
782782
static int
783-
CheckPasswordAuth(Port *port, const char **logdetail)
783+
CheckPasswordAuth(Port *port, char **logdetail)
784784
{
785785
char *passwd;
786786
int result;
@@ -815,7 +815,7 @@ CheckPasswordAuth(Port *port, const char **logdetail)
815815
* MD5 and SCRAM authentication.
816816
*/
817817
static int
818-
CheckPWChallengeAuth(Port *port, const char **logdetail)
818+
CheckPWChallengeAuth(Port *port, char **logdetail)
819819
{
820820
int auth_result;
821821
char *shadow_pass;
@@ -875,7 +875,7 @@ CheckPWChallengeAuth(Port *port, const char **logdetail)
875875
}
876876

877877
static int
878-
CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail)
878+
CheckMD5Auth(Port *port, char *shadow_pass, char **logdetail)
879879
{
880880
char md5Salt[4]; /* Password salt */
881881
char *passwd;
@@ -912,7 +912,7 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail)
912912
}
913913

914914
static int
915-
CheckSCRAMAuth(Port *port, char *shadow_pass, const char **logdetail)
915+
CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
916916
{
917917
StringInfoData sasl_mechs;
918918
int mtype;
@@ -3240,8 +3240,6 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
32403240
md5trailer = packet->vector;
32413241
for (i = 0; i < encryptedpasswordlen; i += RADIUS_VECTOR_LENGTH)
32423242
{
3243-
const char *errstr = NULL;
3244-
32453243
memcpy(cryptvector + strlen(secret), md5trailer, RADIUS_VECTOR_LENGTH);
32463244

32473245
/*
@@ -3250,12 +3248,10 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
32503248
*/
32513249
md5trailer = encryptedpassword + i;
32523250

3253-
if (!pg_md5_binary(cryptvector, strlen(secret) + RADIUS_VECTOR_LENGTH,
3254-
encryptedpassword + i, &errstr))
3251+
if (!pg_md5_binary(cryptvector, strlen(secret) + RADIUS_VECTOR_LENGTH, encryptedpassword + i))
32553252
{
32563253
ereport(LOG,
3257-
(errmsg("could not perform MD5 encryption of password: %s",
3258-
errstr)));
3254+
(errmsg("could not perform MD5 encryption of password")));
32593255
pfree(cryptvector);
32603256
pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
32613257
return STATUS_ERROR;
@@ -3340,7 +3336,6 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
33403336
struct timeval timeout;
33413337
struct timeval now;
33423338
int64 timeoutval;
3343-
const char *errstr = NULL;
33443339

33453340
gettimeofday(&now, NULL);
33463341
timeoutval = (endtime.tv_sec * 1000000 + endtime.tv_usec) - (now.tv_sec * 1000000 + now.tv_usec);
@@ -3459,11 +3454,10 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
34593454

34603455
if (!pg_md5_binary(cryptvector,
34613456
packetlength + strlen(secret),
3462-
encryptedpassword, &errstr))
3457+
encryptedpassword))
34633458
{
34643459
ereport(LOG,
3465-
(errmsg("could not perform MD5 encryption of received packet: %s",
3466-
errstr)));
3460+
(errmsg("could not perform MD5 encryption of received packet")));
34673461
pfree(cryptvector);
34683462
continue;
34693463
}

‎src/backend/libpq/crypt.c

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
* sent to the client, to avoid giving away user information!
3535
*/
3636
char *
37-
get_role_password(const char *role, const char **logdetail)
37+
get_role_password(const char *role, char **logdetail)
3838
{
3939
TimestampTz vuntil = 0;
4040
HeapTuple roleTup;
@@ -116,7 +116,6 @@ encrypt_password(PasswordType target_type, const char *role,
116116
{
117117
PasswordType guessed_type = get_password_type(password);
118118
char *encrypted_password;
119-
const char *errstr = NULL;
120119

121120
if (guessed_type != PASSWORD_TYPE_PLAINTEXT)
122121
{
@@ -133,8 +132,8 @@ encrypt_password(PasswordType target_type, const char *role,
133132
encrypted_password = palloc(MD5_PASSWD_LEN + 1);
134133

135134
if (!pg_md5_encrypt(password, role, strlen(role),
136-
encrypted_password, &errstr))
137-
elog(ERROR, "password encryption failed: %s", errstr);
135+
encrypted_password))
136+
elog(ERROR, "password encryption failed");
138137
return encrypted_password;
139138

140139
case PASSWORD_TYPE_SCRAM_SHA_256:
@@ -160,18 +159,17 @@ encrypt_password(PasswordType target_type, const char *role,
160159
* 'client_pass' is the response given by the remote user to the MD5 challenge.
161160
* 'md5_salt' is the salt used in the MD5 authentication challenge.
162161
*
163-
* In the error case, save a string at *logdetail that will be sent to the
164-
* postmaster log (but not the client).
162+
* In the error case, optionally store a palloc'd string at *logdetail
163+
* that will be sent to the postmaster log (but not the client).
165164
*/
166165
int
167166
md5_crypt_verify(const char *role, const char *shadow_pass,
168167
const char *client_pass,
169168
const char *md5_salt, int md5_salt_len,
170-
const char **logdetail)
169+
char **logdetail)
171170
{
172171
int retval;
173172
char crypt_pwd[MD5_PASSWD_LEN + 1];
174-
const char *errstr = NULL;
175173

176174
Assert(md5_salt_len > 0);
177175

@@ -185,13 +183,16 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
185183

186184
/*
187185
* Compute the correct answer for the MD5 challenge.
186+
*
187+
* We do not bother setting logdetail for any pg_md5_encrypt failure
188+
* below: the only possible error is out-of-memory, which is unlikely, and
189+
* if it did happen adding a psprintf call would only make things worse.
188190
*/
189191
/* stored password already encrypted, only do salt */
190192
if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
191193
md5_salt, md5_salt_len,
192-
crypt_pwd, &errstr))
194+
crypt_pwd))
193195
{
194-
*logdetail = errstr;
195196
return STATUS_ERROR;
196197
}
197198

@@ -214,16 +215,15 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
214215
* pg_authid.rolpassword.
215216
* 'client_pass' is the password given by the remote user.
216217
*
217-
* In the error case, store a string at *logdetail that will be sent to the
218-
* postmaster log (but not the client).
218+
* In the error case, optionally store a palloc'd string at *logdetail
219+
* that will be sent to the postmaster log (but not the client).
219220
*/
220221
int
221222
plain_crypt_verify(const char *role, const char *shadow_pass,
222223
const char *client_pass,
223-
const char **logdetail)
224+
char **logdetail)
224225
{
225226
char crypt_client_pass[MD5_PASSWD_LEN + 1];
226-
const char *errstr = NULL;
227227

228228
/*
229229
* Client sent password in plaintext. If we have an MD5 hash stored, hash
@@ -251,10 +251,14 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
251251
if (!pg_md5_encrypt(client_pass,
252252
role,
253253
strlen(role),
254-
crypt_client_pass,
255-
&errstr))
254+
crypt_client_pass))
256255
{
257-
*logdetail = errstr;
256+
/*
257+
* We do not bother setting logdetail for pg_md5_encrypt
258+
* failure: the only possible error is out-of-memory, which is
259+
* unlikely, and if it did happen adding a psprintf call would
260+
* only make things worse.
261+
*/
258262
return STATUS_ERROR;
259263
}
260264
if (strcmp(crypt_client_pass, shadow_pass) == 0)

0 commit comments

Comments
 (0)