aboutsummaryrefslogtreecommitdiff
path: root/src/backend/libpq
Commit message (Collapse)AuthorAge
* Disconnect if socket cannot be put into non-blocking modeHeikki Linnakangas2024-03-12
| | | | | | | | | | | | | | | | | | | | Commit 387da18874 moved the code to put socket into non-blocking mode from socket_set_nonblocking() into the one-time initialization function, pq_init(). In socket_set_nonblocking(), there indeed was a risk of recursion on failure like the comment said, but in pq_init(), ERROR or FATAL is fine. There's even another elog(FATAL) just after this, if setting FD_CLOEXEC fails. Note that COMMERROR merely logged the error, it did not close the connection, so if putting the socket to non-blocking mode failed we would use the connection anyway. You might not immediately notice, because most socket operations in a regular backend wait for the socket to become readable/writable anyway. But e.g. replication will be quite broken. Backpatch to all supported versions. Discussion: https://www.postgresql.org/message-id/d40a5cd0-2722-40c5-8755-12e9e811fa3c@iki.fi
* Cope with a deficiency in OpenSSL 3.x's error reporting.Tom Lane2024-03-07
| | | | | | | | | | | | | | | | In OpenSSL 3.0.0 and later, ERR_reason_error_string randomly refuses to provide a string for error codes representing system errno values (e.g., "No such file or directory"). There is a poorly-documented way to extract the errno from the SSL error code in this case, so do that and apply strerror, rather than falling back to reporting the error code's numeric value as we were previously doing. Problem reported by David Zhang, although this is not his proposed patch; it's instead based on a suggestion from Heikki Linnakangas. Back-patch to all supported branches, since any of them are likely to be used with recent OpenSSL. Discussion: https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca
* Avoid masking EOF (no-password-supplied) conditions in auth.c.Tom Lane2024-01-03
| | | | | | | | | | | | | | | CheckPWChallengeAuth() would return STATUS_ERROR if the user does not exist or has no password assigned, even if the client disconnected without responding to the password challenge (as libpq often will, for example). We should return STATUS_EOF in that case, and the lower-level functions do, but this code level got it wrong since the refactoring done in 7ac955b34. This breaks the intent of not logging anything for EOF cases (cf. comments in auth_failed()) and might also confuse users of ClientAuthentication_hook. Per report from Liu Lang. Back-patch to all supported versions. Discussion: https://postgr.es/m/b725238c-539d-cb09-2bff-b5e6cb2c069c@esgyn.cn
* Be more wary about OpenSSL not setting errno on error.Tom Lane2023-12-11
| | | | | | | | | | | | | | | | | | | | | | OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set errno; this is apparently a reflection of recv(2)'s habit of not setting errno when reporting EOF. Ensure that we treat such cases the same as read EOF. Previously, we'd frequently report them like "could not accept SSL connection: Success" which is confusing, or worse report them with an unrelated errno left over from some previous syscall. To fix, ensure that errno is zeroed immediately before the call, and report its value only when it's not zero afterwards; otherwise report EOF. For consistency, I've applied the same coding pattern in libpq's pqsecure_raw_read(). Bare recv(2) shouldn't really return -1 without setting errno, but in case it does we might as well cope. Per report from Andres Freund. Back-patch to all supported versions. Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de
* Use BIO_{get,set}_app_data instead of BIO_{get,set}_data.Tom Lane2023-11-28
| | | | | | | | | | | | | | | | | | | | | | We should have done it this way all along, but we accidentally got away with using the wrong BIO field up until OpenSSL 3.2. There, the library's BIO routines that we rely on use the "data" field for their own purposes, and our conflicting use causes assorted weird behaviors up to and including core dumps when SSL connections are attempted. Switch to using the approved field for the purpose, i.e. app_data. While at it, remove our configure probes for BIO_get_data as well as the fallback implementation. BIO_{get,set}_app_data have been there since long before any OpenSSL version that we still support, even in the back branches. Also, update src/test/ssl/t/001_ssltests.pl to allow for a minor change in an error message spelling that evidently came in with 3.2. Tristan Partin and Bo Andreson. Back-patch to all supported branches. Discussion: https://postgr.es/m/CAN55FZ1eDDYsYaL7mv+oSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ@mail.gmail.com
* Fix timing-dependent failure in GSSAPI data transmission.Tom Lane2023-11-23
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When using GSSAPI encryption in non-blocking mode, libpq sometimes failed with "GSSAPI caller failed to retransmit all data needing to be retried". The cause is that pqPutMsgEnd rounds its transmit request down to an even multiple of 8K, and sometimes that can lead to not requesting a write of data that was requested to be written (but reported as not written) earlier. That can upset pg_GSS_write's logic for dealing with not-yet-written data, since it's possible the data in question had already been incorporated into an encrypted packet that we weren't able to send during the previous call. We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's round-down behavior, but that seems like making the caller work around a behavior that pg_GSS_write shouldn't expose in this way. Instead, adjust pg_GSS_write to never report a partial write: it either reports a complete write, or reflects the failure of the lower-level pqsecure_raw_write call. The requirement still exists for the caller to present at least as much data as on the previous call, but with the caller-visible write start point not moving there is no temptation for it to present less. We lose some ability to reclaim buffer space early, but I doubt that that will make much difference in practice. This also gets rid of a rather dubious assumption that "any interesting failure condition (from pqsecure_raw_write) will recur on the next try". We've not seen failure reports traceable to that, but I've never trusted it particularly and am glad to remove it. Make the same adjustments to the equivalent backend routine be_gssapi_write(). It is probable that there's no bug on the backend side, since we don't have a notion of nonblock mode there; but we should keep the logic the same to ease future maintenance. Per bug #18210 from Lars Kanis. Back-patch to all supported branches. Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
* Raise fixed token-length limit in hba.c.Tom Lane2023-07-27
| | | | | | | | | | | | | | Historically, hba.c limited tokens in the authentication configuration files (pg_hba.conf and pg_ident.conf) to less than 256 bytes. We have seen a few reports of this limit causing problems; notably, for moderately-complex LDAP configurations. Increase the limit to 10240 bytes as a low-risk stop-gap solution. In v13 and earlier, this also requires raising MAX_LINE, the limit on overall line length. I'm hesitant to make this code consume too much stack space, so I only raised that to 20480 bytes. Discussion: https://postgr.es/m/1588937.1690221208@sss.pgh.pa.us
* Fix handling of SCRAM-SHA-256's channel binding with RSA-PSS certificatesMichael Paquier2023-02-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | OpenSSL 1.1.1 and newer versions have added support for RSA-PSS certificates, which requires the use of a specific routine in OpenSSL to determine which hash function to use when compiling it when using channel binding in SCRAM-SHA-256. X509_get_signature_nid(), that is the original routine the channel binding code has relied on, is not able to determine which hash algorithm to use for such certificates. However, X509_get_signature_info(), new to OpenSSL 1.1.1, is able to do it. This commit switches the channel binding logic to rely on X509_get_signature_info() over X509_get_signature_nid(), which would be the choice when building with 1.1.1 or newer. The error could have been triggered on the client or the server, hence libpq and the backend need to have their related code paths patched. Note that attempting to load an RSA-PSS certificate with OpenSSL 1.1.0 or older leads to a failure due to an unsupported algorithm. The discovery of relying on X509_get_signature_info() comes from Jacob, the tests have been written by Heikki (with few tweaks from me), while I have bundled the whole together while adding the bits needed for MSVC and meson. This issue exists since channel binding exists, so backpatch all the way down. Some tests are added in 15~, triggered if compiling with OpenSSL 1.1.1 or newer, where the certificate and key files can easily be generated for RSA-PSS. Reported-by: Gunnar "Nick" Bluth Author: Jacob Champion, Heikki Linnakangas Discussion: https://postgr.es/m/17760-b6c61e752ec07060@postgresql.org Backpatch-through: 11
* Remove misguided SSL key file ownership check in libpq.Tom Lane2022-05-26
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commits a59c79564 et al. tried to sync libpq's SSL key file permissions checks with what we've used for years in the backend. We did not intend to create any new failure cases, but it turns out we did: restricting the key file's ownership breaks cases where the client is allowed to read a key file despite not having the identical UID. In particular a client running as root used to be able to read someone else's key file; and having seen that I suspect that there are other, less-dubious use cases that this restriction breaks on some platforms. We don't really need an ownership check, since if we can read the key file despite its having restricted permissions, it must have the right ownership --- under normal conditions anyway, and the point of this patch is that any additional corner cases where that works should be deemed allowable, as they have been historically. Hence, just drop the ownership check, and rearrange the permissions check to get rid of its faulty assumption that geteuid() can't be zero. (Note that the comparable backend-side code doesn't have to cater for geteuid() == 0, since the server rejects that very early on.) This does have the end result that the permissions safety check used for a root user's private key file is weaker than that used for anyone else's. While odd, root really ought to know what she's doing with file permissions, so I think this is acceptable. Per report from Yogendra Suralkar. Like the previous patch, back-patch to all supported branches. Discussion: https://postgr.es/m/MW3PR15MB3931DF96896DC36D21AFD47CA3D39@MW3PR15MB3931.namprd15.prod.outlook.com
* Allow root-owned SSL private keys in libpq, not only the backend.Tom Lane2022-03-02
| | | | | | | | | | | | | | | | | | This change makes libpq apply the same private-key-file ownership and permissions checks that we have used in the backend since commit 9a83564c5. Namely, that the private key can be owned by either the current user or root (with different file permissions allowed in the two cases). This allows system-wide management of key files, which is just as sensible on the client side as the server, particularly when the client is itself some application daemon. Sync the comments about this between libpq and the backend, too. Back-patch of a59c79564 and 50f03473e into all supported branches. David Steele Discussion: https://postgr.es/m/f4b7bc55-97ac-9e69-7398-335e212f7743@pgmasters.net
* Fix pg_hba_file_rules for authentication method certMagnus Hagander2022-01-26
| | | | | | | | | | | For authentication method cert, clientcert=verify-full is implied. But the pg_hba_file_rules entry would incorrectly show clientcert=verify-ca. Per bug #17354 Reported-By: Feike Steenbergen Reviewed-By: Jonathan Katz Backpatch-through: 12
* Revert "graceful shutdown" changes for Windows, in back branches only.Tom Lane2022-01-25
| | | | | | | | | | | | This reverts commits 6051857fc and ed52c3707, but only in the back branches. Further testing has shown that while those changes do fix some things, they also break others; in particular, it looks like walreceivers fail to detect walsender-initiated connection close reliably if the walsender shuts down this way. We'll keep trying to improve matters in HEAD, but it now seems unwise to push these changes into stable releases. Discussion: https://postgr.es/m/CA+hUKG+OeoETZQ=Qw5Ub5h3tmwQhBmDA=nuNO3KG=zWfUypFAw@mail.gmail.com
* On Windows, also call shutdown() while closing the client socket.Tom Lane2021-12-07
| | | | | | | | | | | Further experimentation shows that commit 6051857fc is not sufficient when using (some versions of?) OpenSSL. The reason is obscure, but calling shutdown(socket, SD_SEND) improves matters. Per testing by Andrew Dunstan and Alexander Lakhin. Back-patch as before. Discussion: https://postgr.es/m/af5e0bf3-6a61-bb97-6cba-061ddf22ff6b@dunslane.net
* On Windows, close the client socket explicitly during backend shutdown.Tom Lane2021-12-02
| | | | | | | | | | | | | | It turns out that this is necessary to keep Winsock from dropping any not-yet-sent data, such as an error message explaining the reason for process termination. It's pretty weird that the implicit close done by the kernel acts differently from an explicit close, but it's hard to argue with experimental results. Independently submitted by Alexander Lakhin and Lars Kanis (comments by me, though). Back-patch to all supported branches. Discussion: https://postgr.es/m/90b34057-4176-7bb0-0dbb-9822a5f6425b@greiz-reinsdorf.de Discussion: https://postgr.es/m/16678-253e48d34dc0c376@postgresql.org
* Reject extraneous data after SSL or GSS encryption handshake.Tom Lane2021-11-08
| | | | | | | | | | | | | | | | | | | | | | | The server collects up to a bufferload of data whenever it reads data from the client socket. When SSL or GSS encryption is requested during startup, any additional data received with the initial request message remained in the buffer, and would be treated as already-decrypted data once the encryption handshake completed. Thus, a man-in-the-middle with the ability to inject data into the TCP connection could stuff some cleartext data into the start of a supposedly encryption-protected database session. This could be abused to send faked SQL commands to the server, although that would only work if the server did not demand any authentication data. (However, a server relying on SSL certificate authentication might well not do so.) To fix, throw a protocol-violation error if the internal buffer is not empty after the encryption handshake. Our thanks to Jacob Champion for reporting this problem. Security: CVE-2021-23214
* Fix snapshot reference leak if lo_export fails.Heikki Linnakangas2021-11-03
| | | | | | | | | | | | | | | | | | | If lo_export() fails to open the target file or to write to it, it leaks the created LargeObjectDesc and its snapshot in the top-transaction context and resource owner. That's pretty harmless, it's a small leak after all, but it gives the user a "Snapshot reference leak" warning. Fix by using a short-lived memory context and no resource owner for transient LargeObjectDescs that are opened and closed within one function call. The leak is easiest to reproduce with lo_export() on a directory that doesn't exist, but in principle the other lo_* functions could also fail. Backpatch to all supported versions. Reported-by: Andrew B Reviewed-by: Alvaro Herrera Discussion: https://www.postgresql.org/message-id/32bf767a-2d65-71c4-f170-122f416bab7e@iki.fi
* Set type identifier on BIODaniel Gustafsson2021-08-17
| | | | | | | | | | | | | | | | | | | | | | | | | In OpenSSL there are two types of BIO's (I/O abstractions): source/sink and filters. A source/sink BIO is a source and/or sink of data, ie one acting on a socket or a file. A filter BIO takes a stream of input from another BIO and transforms it. In order for BIO_find_type() to be able to traverse the chain of BIO's and correctly find all BIO's of a certain type they shall have the type bit set accordingly, source/sink BIO's (what PostgreSQL implements) use BIO_TYPE_SOURCE_SINK and filter BIO's use BIO_TYPE_FILTER. In addition to these, file descriptor based BIO's should have the descriptor bit set, BIO_TYPE_DESCRIPTOR. The PostgreSQL implementation didn't set the type bits, which went unnoticed for a long time as it's only really relevant for code auditing the OpenSSL installation, or doing similar tasks. It is required by the API though, so this fixes it. Backpatch through 9.6 as this has been wrong for a long time. Author: Itamar Gafni Discussion: https://postgr.es/m/SN6PR06MB39665EC10C34BB20956AE4578AF39@SN6PR06MB3966.namprd06.prod.outlook.com Backpatch-through: 9.6
* Don't assume GSSAPI result strings are null-terminated.Tom Lane2021-06-23
| | | | | | | | | | | | | | | | | | | | | | | | | Our uses of gss_display_status() and gss_display_name() assumed that the gss_buffer_desc strings returned by those functions are null-terminated. It appears that they generally are, given the lack of field complaints up to now. However, the available documentation does not promise this, and some man pages for gss_display_status() show examples that rely on the gss_buffer_desc.length field instead of expecting null termination. Also, we now have a report that on some implementations, clang's address sanitizer is of the opinion that the byte after the specified length is undefined. Hence, change the code to rely on the length field instead. This might well be cosmetic rather than fixing any real bug, but it's hard to be sure, so back-patch to all supported branches. While here, also back-patch the v12 changes that made pg_GSS_error deal honestly with multiple messages available from gss_display_status. Per report from Sudheer H R. Discussion: https://postgr.es/m/5372B6D4-8276-42C0-B8FB-BD0918826FC3@tekenlight.com
* Disallow SSL renegotiationMichael Paquier2021-05-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | SSL renegotiation is already disabled as of 48d23c72, however this does not prevent the server to comply with a client willing to use renegotiation. In the last couple of years, renegotiation had its set of security issues and flaws (like the recent CVE-2021-3449), and it could be possible to crash the backend with a client attempting renegotiation. This commit takes one extra step by disabling renegotiation in the backend in the same way as SSL compression (f9264d15) or tickets (97d3a0b0). OpenSSL 1.1.0h has added an option named SSL_OP_NO_RENEGOTIATION able to achieve that. In older versions there is an option called SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS that was undocumented, and could be set within the SSL object created when the TLS connection opens, but I have decided not to use it, as it feels trickier to rely on, and it is not official. Note that this option is not usable in OpenSSL < 1.1.0h as the internal contents of the *SSL object are hidden to applications. SSL renegotiation concerns protocols up to TLSv1.2. Per original report from Robert Haas, with a patch based on a suggestion by Andres Freund. Author: Michael Paquier Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/YKZBXx7RhU74FlTE@paquier.xyz Backpatch-through: 9.6
* Fix memory leak when rejecting bogus DH parameters.Tom Lane2021-03-20
| | | | | | | | | | | | | While back-patching e0e569e1d, I noted that there were some other places where we ought to be applying DH_free(); namely, where we load some DH parameters from a file and then reject them as not being sufficiently secure. While it seems really unlikely that anybody would hit these code paths in production, let alone do so repeatedly, let's fix it for consistency. Back-patch to v10 where this code was introduced. Discussion: https://postgr.es/m/16160-18367e56e9a28264@postgresql.org
* Fix memory leak when initializing DH parameters in backendTom Lane2021-03-20
| | | | | | | | | | | | | | | | | | | When loading DH parameters used for the generation of ephemeral DH keys in the backend, the code has never bothered releasing the memory used for the DH information loaded from a file or from libpq's default. This commit makes sure that the information is properly free()'d. Back-patch of e0e569e1d. We originally thought the leak was minor and not worth back-patching, but Jelte Fennema pointed out that repeated SIGHUP's can result in very serious bloat of the postmaster, which is then multiplied by being duplicated into eadh forked child. Back-patch to v10; the code looked different before c0a15e07c, and didn't have a leak in the actually-live code paths. Michael Paquier Discussion: https://postgr.es/m/16160-18367e56e9a28264@postgresql.org
* Avoid corner-case memory leak in SSL parameter processing.Tom Lane2021-03-16
| | | | | | | | | | | | | | | | | | | | | | | After reading the root cert list from the ssl_ca_file, immediately install it as client CA list of the new SSL context. That gives the SSL context ownership of the list, so that SSL_CTX_free will free it. This avoids a permanent memory leak if we fail further down in be_tls_init(), which could happen if bogus CRL data is offered. The leak could only amount to something if the CRL parameters get broken after server start (else we'd just quit) and then the server is SIGHUP'd many times without fixing the CRL data. That's rather unlikely perhaps, but it seems worth fixing, if only because the code is clearer this way. While we're here, add some comments about the memory management aspects of this logic. Noted by Jelte Fennema and independently by Andres Freund. Back-patch to v10; before commit de41869b6 it doesn't matter, since we'd not re-execute this code during SIGHUP. Discussion: https://postgr.es/m/16160-18367e56e9a28264@postgresql.org
* Fix up usage of krb_server_keyfile GUC parameter.Tom Lane2020-12-30
| | | | | | | | | | | | | | | | | | | | | | | | | | | secure_open_gssapi() installed the krb_server_keyfile setting as KRB5_KTNAME unconditionally, so long as it's not empty. However, pg_GSS_recvauth() only installed it if KRB5_KTNAME wasn't set already, leading to a troubling inconsistency: in theory, clients could see different sets of server principal names depending on whether they use GSSAPI encryption. Always using krb_server_keyfile seems like the right thing, so make both places do that. Also fix up secure_open_gssapi()'s lack of a check for setenv() failure --- it's unlikely, surely, but security-critical actions are no place to be sloppy. Also improve the associated documentation. This patch does nothing about secure_open_gssapi()'s use of setenv(), and indeed causes pg_GSS_recvauth() to use it too. That's nominally against project portability rules, but since this code is only built with --with-gssapi, I do not feel a need to do something about this in the back branches. A fix will be forthcoming for HEAD though. Back-patch to v12 where GSSAPI encryption was introduced. The dubious behavior in pg_GSS_recvauth() goes back further, but it didn't have anything to be inconsistent with, so let it be. Discussion: https://postgr.es/m/2187460.1609263156@sss.pgh.pa.us
* Improve log messages related to pg_hba.conf not matching a connection.Tom Lane2020-12-28
| | | | | | | | | | Include details on whether GSS encryption has been activated; since we added "hostgssenc" type HBA entries, that's relevant info. Kyotaro Horiguchi and Tom Lane. Back-patch to v12 where GSS encryption was introduced. Discussion: https://postgr.es/m/e5b0b6ed05764324a2f3fe7acfc766d5@smhi.se
* Fix assorted issues in backend's GSSAPI encryption support.Tom Lane2020-12-28
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Unrecoverable errors detected by GSSAPI encryption can't just be reported with elog(ERROR) or elog(FATAL), because attempting to send the error report to the client is likely to lead to infinite recursion or loss of protocol sync. Instead make this code do what the SSL encryption code has long done, which is to just report any such failure to the server log (with elevel COMMERROR), then pretend we've lost the connection by returning errno = ECONNRESET. Along the way, fix confusion about whether message translation is done by pg_GSS_error() or its callers (the latter should do it), and make the backend version of that function work more like the frontend version. Avoid allocating the port->gss struct until it's needed; we surely don't need to allocate it in the postmaster. Improve logging of "connection authorized" messages with GSS enabled. (As part of this, I back-patched the code changes from dc11f31a1.) Make BackendStatusShmemSize() account for the GSS-related space that will be allocated by CreateSharedBackendStatus(). This omission could possibly cause out-of-shared-memory problems with very high max_connections settings. Remove arbitrary, pointless restriction that only GSS authentication can be used on a GSS-encrypted connection. Improve documentation; notably, document the fact that libpq now prefers GSS encryption over SSL encryption if both are possible. Per report from Mikael Gustavsson. Back-patch to v12 where this code was introduced. Discussion: https://postgr.es/m/e5b0b6ed05764324a2f3fe7acfc766d5@smhi.se
* Fix unportable use of getnameinfo() in pg_hba_file_rules view.Tom Lane2020-11-02
| | | | | | | | | | | | | | | | | | | | | | | fill_hba_line() thought it could get away with passing sizeof(struct sockaddr_storage) rather than the actual addrlen previously returned by getaddrinfo(). While that appears to work on many platforms, it does not work on FreeBSD 11: you get back a failure, which leads to the view showing NULL for the address and netmask columns in all rows. The POSIX spec for getnameinfo() is pretty clearly on FreeBSD's side here: you should pass the actual address length. So it seems plausible that there are other platforms where this coding also fails, and we just hadn't noticed. Also, IMO the fact that getnameinfo() failure leads to a NULL output is pretty bogus in itself. Our pg_getnameinfo_all() wrapper is careful to emit "???" on failure, and we should use that in such cases. NULL should only be emitted in rows that don't have IP addresses. Per bug #16695 from Peter Vandivier. Back-patch to v10 where this code was added. Discussion: https://postgr.es/m/16695-a665558e2f630be7@postgresql.org
* In the postmaster, rely on the signal infrastructure to block signals.Tom Lane2020-10-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | POSIX sigaction(2) can be told to block a set of signals while a signal handler executes. Make use of that instead of manually blocking and unblocking signals in the postmaster's signal handlers. This should save a few cycles, but more importantly it prevents recursive invocation of signal handlers when many signals arrive in close succession. (Assuming that the platform's signal infrastructure is designed to avoid consuming stack space in that case, but this is demonstrably true at least on Linux.) The existing code has been seen to recurse to the point of stack overflow, either in the postmaster or in a forked-off child. Back-patch of commit 9abb2bfc0. At the time, we'd only seen excess postmaster stack consumption in the buildfarm; but we now have a user report of it, and that commit has aged enough to have a fair amount of confidence that it doesn't break anything. This still doesn't change anything about the way that it works on Windows. Perhaps someone else would like to fix that? Per bug #16673 from David Geier. Back-patch to 9.6. Although the problem exists in principle before that, we've only seen it actually materialize in connection with heavy use of parallel workers, so it doesn't seem necessary to do anything in 9.5; and the relevant code is different there, too. Discussion: https://postgr.es/m/16673-d278c604f8e34ec0@postgresql.org Discussion: https://postgr.es/m/14878.1570820201@sss.pgh.pa.us
* gss: add missing references to hostgssenc and hostnogssencBruce Momjian2020-05-25
| | | | | | | | | | | | | These were missed when these were added to pg_hba.conf in PG 12; updates docs and pg_hba.conf.sample. Reported-by: Arthur Nascimento Bug: 16380 Discussion: https://postgr.es/m/20200421182736.GG19613@momjian.us Backpatch-through: 12
* Fix severe memory leaks in GSSAPI encryption support.Tom Lane2020-05-05
| | | | | | | | | | | | | | | | | Both the backend and libpq leaked buffers containing encrypted data to be transmitted, so that the process size would grow roughly as the total amount of data sent. There were also far-less-critical leaks of the same sort in GSSAPI session establishment. Oversight in commit b0b39f72b, which I failed to notice while reviewing the code in 2c0cdc818. Per complaint from pmc@citylink. Back-patch to v12 where this code was introduced. Discussion: https://postgr.es/m/20200504115649.GA77072@gate.oper.dinoex.org
* Extensive code review for GSSAPI encryption mechanism.Tom Lane2020-01-11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix assorted bugs in handling of non-blocking I/O when using GSSAPI encryption. The encryption layer could return the wrong status information to its caller, resulting in effectively dropping some data (or possibly in aborting a not-broken connection), or in a "livelock" situation where data remains to be sent but the upper layers think transmission is done and just go to sleep. There were multiple small thinkos contributing to that, as well as one big one (failure to think through what to do when a send fails after having already transmitted data). Note that these errors could cause failures whether the client application asked for non-blocking I/O or not, since both libpq and the backend always run things in non-block mode at this level. Also get rid of use of static variables for GSSAPI inside libpq; that's entirely not okay given that multiple connections could be open at once inside a single client process. Also adjust a bunch of random small discrepancies between the frontend and backend versions of the send/receive functions -- except for error handling, they should be identical, and now they are. Also extend the Kerberos TAP tests to exercise cases where nontrivial amounts of data need to be pushed through encryption. Before, those tests didn't provide any useful coverage at all for the cases of interest here. (They still might not, depending on timing, but at least there's a chance.) Per complaint from pmc@citylink and subsequent investigation. Back-patch to v12 where this code was introduced. Discussion: https://postgr.es/m/20200109181822.GA74698@gate.oper.dinoex.org
* Avoid downcasing/truncation of RADIUS authentication parameters.Tom Lane2019-11-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 6b76f1bb5 changed all the RADIUS auth parameters to be lists rather than single values. But its use of SplitIdentifierString to parse the list format was not very carefully thought through, because that function thinks it's parsing SQL identifiers, which means it will (a) downcase the strings and (b) truncate them to be shorter than NAMEDATALEN. While downcasing should be harmless for the server names and ports, it's just wrong for the shared secrets, and probably for the NAS Identifier strings as well. The truncation aspect is at least potentially a problem too, though typical values for these parameters would fit in 63 bytes. Fortunately, we now have a function SplitGUCList that is exactly the same except for not doing the two unwanted things, so fixing this is a trivial matter of calling that function instead. While here, improve the documentation to show how to double-quote the parameter values. I failed to resist the temptation to do some copy-editing as well. Report and patch from Marcos David (bug #16106); doc changes by me. Back-patch to v10 where the aforesaid commit came in, since this is arguably a regression from our previous behavior with RADIUS auth. Discussion: https://postgr.es/m/16106-7d319e4295d08e70@postgresql.org
* Avoid logging complaints about abandoned connections when using PAM.Tom Lane2019-11-05
| | | | | | | | | | | | | | | | | | | For a long time (since commit aed378e8d) we have had a policy to log nothing about a connection if the client disconnects when challenged for a password. This is because libpq-using clients will typically do that, and then come back for a new connection attempt once they've collected a password from their user, so that logging the abandoned connection attempt will just result in log spam. However, this did not work well for PAM authentication: the bottom-level function pam_passwd_conv_proc() was on board with it, but we logged messages at higher levels anyway, for lack of any reporting mechanism. Add a flag and tweak the logic so that the case is silent, as it is for other password-using auth mechanisms. Per complaint from Yoann La Cancellera. It's been like this for awhile, so back-patch to all supported branches. Discussion: https://postgr.es/m/CACP=ajbrFFYUrLyJBLV8=q+eNCapa1xDEyvXhMoYrNphs-xqPw@mail.gmail.com
* Fix compilation with older OpenSSL versionsPeter Eisentraut2019-09-28
| | | | | | | | | | | Some older OpenSSL versions (0.9.8 branch) define TLS*_VERSION macros but not the corresponding SSL_OP_NO_* macro, which causes the code for handling ssl_min_protocol_version/ssl_max_protocol_version to fail to compile. To fix, add more #ifdefs and error handling. Reported-by: Victor Wagner <vitus@wagner.pp.ru> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://www.postgresql.org/message-id/flat/20190924101859.09383b4f%40fafnir.local.vm
* Message style fixesPeter Eisentraut2019-09-23
|
* GSSAPI error message improvementsPeter Eisentraut2019-09-19
| | | | | | | | | | Make the error messages around GSSAPI encryption a bit clearer. Tweak some messages to avoid plural problems. Also make a code change for clarity. Using "conf" for "confidential" is quite confusing. Using "conf_state" is perhaps not much better but that's what the GSSAPI documentation uses, so there is at least some hope of understanding it.
* Remove superfluous newlines in function prototypes.Andres Freund2019-07-31
| | | | | | | | | | | | | | | These were introduced by pgindent due to fixe to broken indentation (c.f. 8255c7a5eeba8). Previously the mis-indentation of function prototypes was creatively used to reduce indentation in a few places. As that formatting only exists in master and REL_12_STABLE, it seems better to fix it in both, rather than having some odd indentation in v12 that somebody might copy for future patches or such. Author: Andres Freund Discussion: https://postgr.es/m/20190728013754.jwcbe5nfyt3533vx@alap3.anarazel.de Backpatch: 12-
* Fix failures to ignore \r when reading Windows-style newlines.Tom Lane2019-07-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | libpq failed to ignore Windows-style newlines in connection service files. This normally wasn't a problem on Windows itself, because fgets() would convert \r\n to just \n. But if libpq were running inside a program that changes the default fopen mode to binary, it would see the \r's and think they were data. In any case, it's project policy to ignore \r in text files unconditionally, because people sometimes try to use files with DOS-style newlines on Unix machines, where the C library won't hide that from us. Hence, adjust parseServiceFile() to ignore \r as well as \n at the end of the line. In HEAD, go a little further and make it ignore all trailing whitespace, to match what it's always done with leading whitespace. In HEAD, also run around and fix up everyplace where we have newline-chomping code to make all those places look consistent and uniformly drop \r. It is not clear whether any of those changes are fixing live bugs. Most of the non-cosmetic changes are in places that are reading popen output, and the jury is still out as to whether popen on Windows can return \r\n. (The Windows-specific code in pipe_read_line seems to think so, but our lack of support for this elsewhere suggests maybe it's not a problem in practice.) Hence, I desisted from applying those changes to back branches, except in run_ssl_passphrase_command() which is new enough and little-tested enough that we'd probably not have heard about any problems there. Tom Lane and Michael Paquier, per bug #15827 from Jorge Gustavo Rocha. Back-patch the parseServiceFile() change to all supported branches, and the run_ssl_passphrase_command() change to v11 where that was added. Discussion: https://postgr.es/m/15827-e6ba53a3a7ed543c@postgresql.org
* Fix many typos and inconsistenciesMichael Paquier2019-07-01
| | | | | Author: Alexander Lakhin Discussion: https://postgr.es/m/af27d1b3-a128-9d62-46e0-88f424397f44@gmail.com
* Remove unnecessary header from be-secure-gssapi.cMichael Paquier2019-06-29
| | | | | | | | | libpq/libpq-be.h is included by libpq/libpq.h so there is no need to explicitly include it separately. Author: Daniel Gustafsson Reviewed-by: Julien Rouhaud Discussion: https://postgr.es/m/A4852E46-9ED1-4861-A23B-22A83E34A084@yesql.se
* Fix buffer overflow when parsing SCRAM verifiers in backendMichael Paquier2019-06-17
| | | | | | | | | | | | | | | | | | | Any authenticated user can overflow a stack-based buffer by changing the user's own password to a purpose-crafted value. This often suffices to execute arbitrary code as the PostgreSQL operating system account. This fix is contributed by multiple folks, based on an initial analysis from Tom Lane. This issue has been introduced by 68e61ee, so it was possible to make use of it at authentication time. It became more easily to trigger after ccae190 which has made the SCRAM parsing more strict when changing a password, in the case where the client passes down a verifier already hashed using SCRAM. Back-patch to v10 where SCRAM has been introduced. Reported-by: Alexander Lakhin Author: Jonathan Katz, Heikki Linnakangas, Michael Paquier Security: CVE-2019-10164 Backpatch-through: 10
* Fix more typos and inconsistencies in the treeMichael Paquier2019-06-17
| | | | | Author: Alexander Lakhin Discussion: https://postgr.es/m/0a5419ea-1452-a4e6-72ff-545b1a5a8076@gmail.com
* Fix typos and inconsistencies in code commentsMichael Paquier2019-06-14
| | | | | Author: Alexander Lakhin Discussion: https://postgr.es/m/dec6aae8-2d63-639f-4d50-20e229fb83e3@gmail.com
* Move be-gssapi-common.h into src/include/libpq/Michael Paquier2019-06-08
| | | | | | | | | | | The file has been introduced in src/backend/libpq/ as of b0b39f72, but all backend-side headers of libpq are located in src/include/libpq/. Note that the identification path on top of the file referred to src/include/libpq/ from the start. Author: Michael Paquier Reviewed-by: Stephen Frost Discussion: https://postgr.es/m/20190607043415.GE1736@paquier.xyz
* Fix typos.Amit Kapila2019-05-26
| | | | | | | Reported-by: Alexander Lakhin Author: Alexander Lakhin Reviewed-by: Amit Kapila and Tom Lane Discussion: https://postgr.es/m/7208de98-add8-8537-91c0-f8b089e2928c@gmail.com
* Update copyright year.Thomas Munro2019-05-24
| | | | | Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/CA%2BhUKGJFWXmtYo6Frd77RR8YXCHz7hJ2mRy5aHV%3D7fJOqDnBHA%40mail.gmail.com
* Phase 2 pgindent run for v12.Tom Lane2019-05-22
| | | | | | | | | Switch to 2.1 version of pg_bsd_indent. This formats multiline function declarations "correctly", that is with additional lines of parameter declarations indented to match where the first line's left parenthesis is. Discussion: https://postgr.es/m/CAEepm=0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug@mail.gmail.com
* Initial pgindent run for v12.Tom Lane2019-05-22
| | | | | | | | This is still using the 2.0 version of pg_bsd_indent. I thought it would be good to commit this separately, so as to document the differences between 2.0 and 2.1 behavior. Discussion: https://postgr.es/m/16296.1558103386@sss.pgh.pa.us
* Clean up minor warnings from buildfarm.Tom Lane2019-04-28
| | | | | | | | | | | | | | | | | Be more consistent about use of XXXGetDatum macros in new jsonpath code. This is mostly to avoid having code that looks randomly different from everyplace else that's doing the exact same thing. In pg_regress.c, avoid an unreferenced-function warning from compilers that don't understand pg_attribute_unused(). Putting the function inside the same #ifdef as its only caller is more straightforward coding anyway. In be-secure-openssl.c, avoid use of pg_attribute_unused() on a label. That's pretty creative, but there's no good reason to suppose that it's portable, and there's absolutely no need to use goto's here in the first place. (This wasn't actually causing any buildfarm complaints, but it's new code in v12 so it has no portability track record.)
* Fix detection of passwords hashed with MD5 or SCRAM-SHA-256Michael Paquier2019-04-23
| | | | | | | | | | | | | | | | | | | | | | This commit fixes a couple of issues related to the way password verifiers hashed with MD5 or SCRAM-SHA-256 are detected, leading to being able to store in catalogs passwords which do not follow the supported hash formats: - A MD5-hashed entry was checked based on if its header uses "md5" and if the string length matches what is expected. Unfortunately the code never checked if the hash only used hexadecimal characters, as reported by Tom Lane. - A SCRAM-hashed entry was checked based on only its header, which should be "SCRAM-SHA-256$", but it never checked for any fields afterwards, as reported by Jonathan Katz. Backpatch down to v10, which is where SCRAM has been introduced, and where password verifiers in plain format have been removed. Author: Jonathan Katz Reviewed-by: Tom Lane, Michael Paquier Discussion: https://postgr.es/m/016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org Backpatch-through: 10
* Fix assorted minor bogosity in GSSAPI transport error messages.Tom Lane2019-04-17
| | | | | | | | | | | I noted that some buildfarm members were complaining about %ld being used to format values that are (probably) declared size_t. Use %zu instead, and insert a cast just in case some versions of the GSSAPI API declare the length field differently. While at it, clean up gratuitous differences in wording of equivalent messages, show the complained-of length in all relevant messages not just some, include trailing newline where needed, adjust random deviations from project-standard code layout and message style, etc.
generated by cgit v1.2.3 (git 2.25.1) at 2025-08-04 11:39:20 +0000