The Pageant transact path trusted the 32-bit length in the shared memory
mapping and could memcpy past the mapped view. It also treated
a non-positive SendMessage(WM_COPYDATA) result as success.
Changes:
Reject replies when SendMessage returns ≤ 0 and report
LIBSSH2_ERROR_AGENT_PROTOCOL.
Bound the copy by validating response_len <= PAGEANT_MAX_MSGLEN - 4
(accounting for the length prefix) to avoid OOB reads.
Impact: prevents potential out-of-bounds read and use of uninitialized
mapping contents when Pageant misbehaves or is malicious.
In `_libssh2_mbedtls_pub_priv_key()` on a NON-error code path, a stack
variable was checked without initializing it first.
I found it interesting that clang-tidy did not find this when building
against the system mbedtls (2.x) with 2.x compatibility code still in.
Then it did find it when using a manual build of mbedtls 3.1.0 with
2.x compatibility code deleted from libssh2. Being such a trivial error
I wonder why no compiler ever detected it as a regular warning.
linux (clang-tidy, amd64, mbedTLS-prev [3.1.0], cmake, ON):
```
src/mbedtls.c:744:8: error: Branch condition evaluates to a garbage value [clang-analyzer-core.uninitialized.Branch,-warnings-as-errors]
744 | if(ret) {
| ^
```
Ref: https://github.com/libssh2/libssh2/actions/runs/18620615649/job/53091295760#step:22:44
Follow-up to 186f1a2d75#132
Cherry-picked from #1727Closes#1729
Use `HAVE_ECC` as an indicator for ECDSA when building with wolfSSL.
Before this patch the OpenSSL macros were used, in particular
`OPENSSL_NO_EC`, which made ECDSA support disabled with certain
wolfSSL build configurations, e.g. the Ubuntu 24.04 one.
ECDSA is necessary to run tests with OpenSSH v10, e.g. on Debian Trixie.
Follow-up to b95e758239#666
Ref: #1720Closes#1723
Also:
- rename a spellcheck file to match curl.
- editorconfig: fix line width.
- editorconfig: make it use UTF-8.
- editroconfig: apply some rules to all files.
- .gitignore: drop dupe, drop `.DS_Store` (not created by this repo),
sort.
- .gitignore: add for tests executables.
Closes#1718
This also means that we no longer pass any picky warning option to
ossfuzz. It's probably not worth maintaining picky C++ options for this
single, small target.
Silencing:
```
cc1plus: warning: command-line option '-Wbad-function-cast' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wdeclaration-after-statement' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wenum-int-mismatch' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wjump-misses-init' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wmissing-parameter-type' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wmissing-prototypes' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wnested-externs' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wold-style-declaration' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wold-style-definition' is valid for C/ObjC but not for C++
cc1plus: warning: command-line option '-Wstrict-prototypes' is valid for C/ObjC but not for C++
```
Ref: https://github.com/libssh2/libssh2/actions/runs/18063134305/job/51402236388#step:7:15Closes#1686
- use `cmake_path()` to query filenames, with CMake 3.20 or upper.
https://cmake.org/cmake/help/v4.1/command/cmake_path.html#query
- also quote the value passed to `get_filename_component()` where
missing. (Could not cause an actual issue as used in the code.)
Closes#1673
- userauth: fix NULL dereference when out-of-memory.
Also fix indentation.
Follow-up to 3a6ab70dcf#1314
- openssl: drop redundant NULL check and logic.
Follow-up to ed439a29bb#698
Pointed out by CodeQL
Closes#1656
Notes:
Added additional base64 decoding validation when parsing known_hosts and no longer assume what is going into _libssh2_base64_encode() is a null terminated C string, input now must have a length and buffer.
Reported by:
Dhiraj Mishra mishra.dhiraj95@gmail.com
Credit:
Will Cosgrove
Reviewed by:
Michael Buckley
In `kex_method_diffie_hellman_group_exchange_sha256_key_exchange`,
`p` and `g` are later initialized with `_libssh2_bn_from_bin`, so they
should be initially created using `_libssh2_bn_init_from_bin` rather
than `_libssh2_bn_init`, as is done in
`kex_method_diffie_hellman_group_exchange_sha1_key_exchange`.
Fixing memory leaks when using the libgcrypt backend.
Follow-up to 09c5e59933
Ref: https://web.archive.org/web/trac.libssh2.org/ticket/168Closes#1599
- Reworking the `Libs.private` collector logic for INTERFACE targets,
broke the original lib order. Fix it by going back a single loop
to retain order.
Follow-up to df0563a857#1535
- Implement the above with one change: move implicit CMake libs
to the end of the list (was: the beginning).
I expect these to be libs that any custom libs may depend on,
like system libs, C runtime, compiler runtime lib.
Follow-up to c87f129630#1466Closes#1623
`LIBSSH2_PC_LIBS_PRIVATE` ends up in `Libs.private` in `libssh2.pc`.
The order and duplication may be significant for linkers that rely on
strict lib order and unable to resolve symbols without it. Such linker
is binutils `ld`. De-duplication can break it.
As of now there is no purposeful duplication in libs in libssh2, thus
the de-duplication most likely did not affect actual builds.
It was originally introduced to avoid a repeat `-lz` (with
a zlib-enabled OpenSSL or wolfSSL build.) To keep this feature, this
patch makes sure to only delete duplicates that are next to each other.
Follow-up to 6464301820#1131Closes#1621
The `libssh2.pc` generator logic automatically adds `-lws2_32` while
parsing `LIBSSH2_LIBS`, which contains this lib already. Then discard
the duplicate.
This patch introduces a change in the position of `ws2_32` within
the lib list advertised via `libssh2.pc` for static builds.
This order might in cases by significant, but:
- libssh2 no longer links against `libssl`, which was the library
also referencing `ws2_32` and breaking picky binutils `ld` linker
when not passed in strict dependency order.
Ref: c84745e34e#1128
- since switching to INTERFACE targets, cmake messes up the lib order
anyway, adding `OpenSSL:Crypto` last, instead of `ws2_32`. This did
not seem to cause an issue so far.
Ref: df0563a857#1535
For these reasons it seems unlikely this position change could break
Windows OpenSSL static gcc/ld builds relying on `libssh2.pc` for their
lib list. It least no more than it was before this patch.
Turns out these theories don't stand in practice and the order is broken
possibly by introducing INTERFACE targets, with or without this patch.
`libcrypto` (tested with LibreSSL) is in fact depending on `ws2_32`, and
the `bcrypt` dependency is also causing breakage with picky binutils ld.
I may try addressing it in a separate PR.
Ref: 33b6d5f89d#827
Ref: 31fb8860db#811Closes#1619
It looks like the incorrect length is used to copy the public key method
into the session in `libssh2_agent_sign()` and while the public key type
at the start of a identity blob is often identical to the public key
method it might not always be such as when method is `rsa-sha2-256` for
`ssh-rsa` keys.
Closes#1603
- cmake: sync `-ftree-vrp` behavior with autotools.
- build: enable `-Wjump-misses-init` for GCC 4.5+.
Credits-to: Marcel Raad
- packet: fix `-Wjump-misses-init` warnings.
```
src/packet.c: In function ‘_libssh2_packet_add’:
src/packet.c:671:9: error: jump skips variable initialization [-Werror=jump-misses-init]
src/packet.c:920:31: note: ‘want_reply’ declared here
src/packet.c:671:9: error: jump skips variable initialization [-Werror=jump-misses-init]
src/packet.c:919:26: note: ‘len’ declared here
src/packet.c:669:9: error: jump skips variable initialization [-Werror=jump-misses-init]
src/packet.c:1121:31: note: ‘want_reply’ declared here
src/packet.c:669:9: error: jump skips variable initialization [-Werror=jump-misses-init]
src/packet.c:1120:26: note: ‘len’ declared here
src/packet.c:669:9: error: jump skips variable initialization [-Werror=jump-misses-init]
src/packet.c:1119:26: note: ‘channel’ declared here
```
- build: enable gcc-12/13+, clang-10+ picky warnings
- acinclude.m4: sync formatting/comments with curl.
- autotools: fix `-Wtrampolines` picky warning for gcc 4.x versions.
Follow-up to 854cfa8292#1524
- cmake: enable `-Wall` for MSVC when `PICKY_COMPILER=ON`.
- MSVC: fix `-Wall` warnings.
Seen on VS2015. Not seen on VS2022. Unknown for other versions.
```
tests\test_simple.c(60): warning C4777: 'fprintf' : format string '%d' requires an argument of type 'int', but variadic argument 1 has type 'std::size_t'
tests\test_simple.c(60): warning C4777: 'fprintf' : format string '%.*s' requires an argument of type 'int', but variadic argument 2 has type 'std::size_t'
```
- mbedtls: stop silencing warnings in 3rd-party header.
Follow-up to a3aa6b4ca8#1525
- cmake: stop deleting `-W<n>` from `CMAKE_C_FLAGS` (MSVC)
1. `CMAKE_C_FLAGS` may apply to other projects, and deleting/altering it
may be unexpected.
2. We pass `-W4`/`-Wall` internally now, which do override custom
`-W<n>` options as tested with VS2008 and newer VS generators.
Closes#1588
- fix `add_subdirectory` builds for old CMake versions.
- libssh2-config.cmake: fix to set CMP0099 for CMake 3.17+ only.
- libssh2-config.cmake: generalize code to support any number of deps.
(mainly to sync with curl.)
- libssh2-config.cmake: bind dependencies to the static libssh2 only.
Follow-up to a0d8529b08#1571
Follow-up to df0563a857#1535Closes#1581
- replace `CMAKE_C_FLAGS*` and `CMAKE_CXX_FLAGS` with `COMPILE_OPTIONS`.
- replace `CMAKE_SHARED_LINKER_FLAGS_DEBUG` with
`LINK_OPTIONS`/`LINK_FLAGS`.
- make it explicit to pass these C flags to feature checks.
- enable `-pedantic-errors` picky option for GCC with CMake <3.23.
- drop redundant condition when stripping existing MSVC `/Wn` options.
CMake passes `CMAKE_C_FLAGS` to targets, feature checks and raw
`try_compile()` calls. With `COMPILE_OPTIONS`, this is limited to
targets, and we must explicitly pass them to feature checks. This
makes the build logic clearer, and offers more control. It also
reduces log noise by omitting these options from linker commands,
and from `CMAKE_C_FLAGS` dumps in feature checks.
Closes#1575
- drop `VERSION` target property for cmake <3.19 compatibility
```
CMake Error at CMake/Find*.cmake:90 (set_target_properties):
INTERFACE_LIBRARY targets may only have whitelisted properties.
The property "VERSION" is not allowed.
```
- move custom target property to the `INTERFACE_` namespace
for cmake <3.19 compatibility. (To avoid same error as above)
- fix forwarding multiple `CFLAGS`, when detected via `pkg-config`.
- restore support for `-framework` and raw libs when processing
the internal lib list for generating `libssh2.pc`. For good measure,
at the moment libssh2 doesn't depend on a Framework.
- limit `libssh2_dumptargetprops()` to cmake 3.19+. It doesn't work with
older versions.
Issues found while applying this change to curl. They did not surface in
libssh2 CI.
Follow-up to df0563a857#1535Closes#1571
- kex: drop unused assigment.
- knownhost: error when salt is NULL.
- mbedtls: avoid unnecessary inline assigments, that were ignored for
the second block and replaceable with a `ret = 0` initialization for
the first one.
- mbedtls: fix ignoring an API failure and ending up calling
`mbedtls_rsa_check_privkey()` unconditionally.
- misc: initialize datalen on error in `_libssh2_base64_decode()`.
- openssl: drop unused assigments.
- openssl: fix unused static function.
- packet: avoid NULL deref.
- packet: avoid NULL in `memcpy` src.
- publickey: optimize struct layout to avoid padding.
- sftp: replace ignored `rc` error assigment with `_libssh2_error()` call.
- transport: fix potential NULL ptr dereferences.
- transport: silence uninitialized value warnings.
- userauth: drop unused assigment.
- userauth: possible use of unitialized pointer.
- userauth: replace `rewind()` with `fseek()`.
`rewind()` returns an error condition in `errno`. `errno` is
problematic and reduces portability. Use `fseek()` to avoid it.
- userauth: replace potential NULL deref by returning error from
`sign_frommemory()`. Possible false positive. `rc` should be set
upstream if the callback is NULL.
- userauth: replace potential NULL deref by returning error from
`sign_fromfile()`. clang-tidy did not warn about this one, but
let's match `sign_frommemory()` anyway.
- wincng: fix potentially unused macros.
- wincng: make sure bignum is not NULL before use.
tests:
- openssh_fixture: drop unused assignment.
- session_fixture: exit if `username` not set, to avoid `strlen(NULL)`.
- session_fixture: replace `rewind()` with `fseek()`.
`rewind()` returns an error condition in `errno`. `errno` is
problematic and reduces portability. Use `fseek()` to avoid it.
- test_read: exit if `username` not set, to avoid `strlen(NULL)`.
examples:
- scp_write_nonblock: fix file handle leak.
- sftp_write_nonblock: file handle leak on error.
- sftp_write_sliding: file handle leak on error.
- ssh2_agent_forwarding: fix unused error codes.
Details in the subcommits under the PR.
Thanks-to: Michael Buckley
Thanks-to: Will Cosgrove
Closes#1561
