There's a race window between the accept loop's call to
accept(2) and it checking `ctx.keep_going`. Forcefully
close the server socket such that any raced `accept` ends
up failing.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Sometimes, but not always, the pkd tests will fail because they
close the socket at hand a bit too early for the client. The
client in turn may exit non-zero when that happens.
Split up the final close loop so that pkd waits to receive a
channel close from the client, and then socket close, before
finally returning.
With this change I observe that tests are now passing in
environments that would previously tickle the above race
and fail.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Fix a memory leak in the path where parsing returns early due
to seeing a repeated opcode. A testcase is added which
demonstrates the leak and fix with valgrind.
Resolves CID 1374267.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Summary:
Based on Dirkjan's original patch series here:
* https://www.libssh.org/archive/libssh/2015-08/0000029.html
Here the changes are adapted for the current master
branch, and expanded to include libgcrypt support.
Co-Authored-By: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Jon Simons <jon@jonsimons.org>
Test Plan:
* Ran pkd tests for libcrypto and libgcrypt builds.
* Ran client torture_algorithms.c tests for libcrypto and libgcrypt builds.
* Tested across multiple libgcrypts ("1.6.3" and "1.7.6-beta").
Reviewers: aris, asn
Reviewed By: asn
Tags: #libssh
Differential Revision: https://bugs.libssh.org/D7
Summary:
Based on Dirkjan's original patch series here:
* https://www.libssh.org/archive/libssh/2015-08/0000029.html
Here the changes are adapted for the current master
branch, and expanded to include libgcrypt support.
Co-Authored-By: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Test Plan:
* Ran pkd tests for libcrypto and libgcrypt builds.
* Ran client torture_algorithms.c tests for libcrypto and libgcrypt builds.
* Tested across multiple libgcrypts ("1.6.3" and "1.7.6-beta").
Reviewers: aris, asn
Tags: #libssh
Differential Revision: https://bugs.libssh.org/D7
That way, we will not fail later on key exchange phase when something
unknown is negotiated.
Fixes T37
Signed-off-by: Nikos Mavrogiannopoulos <nmav@redhat.com>
Reviewed-by: Andreas Schneider <asn@samba.org>
Summary:
Hello, this is a resend for a quick memory leak fix for one of the unit
tests, originally sent to the mailing list here:
* https://www.libssh.org/archive/libssh/2017-07/0000017.html
Test Plan:
* Before the fix and running the test with valgrind:
```
[simonsj@simonsj-lx5 : unittests] valgrind --leak-check=full ./torture_options >/dev/null
==93134== Memcheck, a memory error detector
==93134== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==93134== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==93134== Command: ./torture_options
==93134==
[ PASSED ] 10 test(s).
[ PASSED ] 1 test(s).
==93134==
==93134== HEAP SUMMARY:
==93134== in use at exit: 80 bytes in 1 blocks
==93134== total heap usage: 977 allocs, 976 frees, 75,029 bytes allocated
==93134==
==93134== 80 bytes in 1 blocks are definitely lost in loss record 1 of 1
==93134== at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==93134== by 0x41BAB0: ssh_key_new (pki.c:107)
==93134== by 0x40DF90: torture_bind_options_import_key (torture_options.c:222)
==93134== by 0x4E3AA3A: cmocka_run_one_test_or_fixture (cmocka.c:2304)
==93134== by 0x4E3ACEA: cmocka_run_one_tests (cmocka.c:2412)
==93134== by 0x4E3B036: _cmocka_run_group_tests (cmocka.c:2517)
==93134== by 0x40E9E3: torture_run_tests (torture_options.c:276)
==93134== by 0x40DE68: main (torture.c:1100)
==93134==
==93134== LEAK SUMMARY:
==93134== definitely lost: 80 bytes in 1 blocks
==93134== indirectly lost: 0 bytes in 0 blocks
==93134== possibly lost: 0 bytes in 0 blocks
==93134== still reachable: 0 bytes in 0 blocks
==93134== suppressed: 0 bytes in 0 blocks
==93134==
==93134== For counts of detected and suppressed errors, rerun with: -v
==93134== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
```
* And after:
```
[simonsj@simonsj-lx5 : unittests] valgrind --leak-check=full ./torture_options >/dev/null
==93294== Memcheck, a memory error detector
==93294== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==93294== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==93294== Command: ./torture_options
==93294==
[ PASSED ] 10 test(s).
[ PASSED ] 1 test(s).
==93294==
==93294== HEAP SUMMARY:
==93294== in use at exit: 0 bytes in 0 blocks
==93294== total heap usage: 977 allocs, 977 frees, 75,029 bytes allocated
==93294==
==93294== All heap blocks were freed -- no leaks are possible
==93294==
==93294== For counts of detected and suppressed errors, rerun with: -v
==93294== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```
Reviewers: asn
Reviewed By: asn
Differential Revision: https://bugs.libssh.org/D3
Summary:
Hello, resending this patch series for the `pkd` tests, originally
sent to the mailing list here:
* https://www.libssh.org/archive/libssh/2017-07/0000011.html
Here are a few improvements and fixups for the `pkd` tests, including
a new flag `-m` that can be used to run only certain subsets of the
test passes.
Jon Simons (5):
pkd: rename AES192 cipher suite -> OPENSSHONLY
pkd_daemon.c: mark `pkd_ready` field as volatile
pkd: fixups for updated CMocka CMUnitTest struct
pkd: refactor -t testname lookup-by-name
pkd: support -m to match multiple tests
tests/pkd/pkd_daemon.c | 2 +-
tests/pkd/pkd_daemon.h | 1 +
tests/pkd/pkd_hello.c | 84 +++++++++++++++++++++++++++++++++-----------------
3 files changed, 58 insertions(+), 29 deletions(-)
--
Test Plan:
* I've been using the new `-m` mode locally for a long time to run
only certain groups of tests.
* The CMocka struct fixes can be seen in the pkd output before and
after: after, there are no more extraneous test output strings.
* The fix for the `pkd_ready` field can be observed when building
the libssh tests with `-Os` on a Debian system (before the fix,
pkd would hang, after the fix, it runs as intended).
Reviewers: asn
Reviewed By: asn
Tags: #libssh
Differential Revision: https://bugs.libssh.org/D2
On OpenSSL versions prior to 1.1.0, `EVP_CIPHER_CTX_cleanup` will
dereference its argument regardless of whether it is NULL. This
is not a problem on OpenSSL at or beyond 1.1.0, where
`EVP_CIPHER_CTX_cleanup` (macro to `EVP_CIPHER_CTX_reset`) returns
early upon NULL input.
Move the call to `EVP_CIPHER_CTX_cleanup` under the existing NULL
check in `evp_cipher_cleanup` to avoid the problem.
Introduced with this build-break fix:
* e66f370682927ca8bd7ae0e7544754c6f4ac4969
Found in manual testing in an environment with an older OpenSSL.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Relax the cases where `ssh_analyze_banner` fails to extract a
major and minor version from banners which appear like OpenSSH
banners.
Update the tests to demonstrate that now a banner as might be
sent by `ssh-keyscan(1)` ("SSH-2.0-OpenSSH-keyscan") no longer
returns failure.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
With this change, a HAVE_LIBCRYPTO #ifdef is removed from wrapper.c.
Now, the libcrypto-specific logic for EVP_CIPHER_CTX_free is moved
into the ssh_cipher_struct cleanup callback handler for those
ciphers.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Fix a gcrypt build error introduced with
48e7b098f86207f8596651b5ba8242a3b834a868.
The ssh_cipher_struct only contains a `ctx` field on
the libcrypto builds, so it can't be referenced unless
within HAVE_LIBCRYPTO.
This build fix preserves the original spirit of the
change in 48e7b098f86207f8596651b5ba8242a3b834a868:
only call `EVP_CIPHER_CTX_free` when `cipher->ctx`
is non-NULL.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Fix error-checking for `strtoul` in `ssh_analyze_banner`, and
enable some tests which demonstrate the fix before-and-after.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
No changes to code, only whitespace indentation and
an update to the function docs.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Fix a resource leak in `hmac_final`: say `HMAC_CTX_free` instead
of `HMAC_CTX_reset`. This matches the error handling as done in
`hmac_init`. Introduced with cf1e808e2ffa1f26644fb5d2cb82a919f323deba.
The problem is reproducible running the `pkd_hello` test with:
valgrind --leak-check=full ./pkd_hello -i1 -t torture_pkd_openssh_dsa_rsa_default
Resolves https://red.libssh.org/issues/252.
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
On older OpenSSL versions, the EVP_MD_CTX fields within an HMAC_CTX
structure are contained inlined (change here [1]): be sure to not
try to free those fields on those builds.
Found running the `pkd_hello` test with:
valgrind ./pkd_hello -i1 -t torture_pkd_openssh_dsa_rsa_default
^ valgrind will cite "Invalid free() ..." errors which are present
before this fix and absent after, when building with OpenSSL 1.0.1.
[1] 6e59a892db
Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
libssh fails to read the configuration from a config file due to a
wrong check in 'ssh_config_parse_line' procedure in 'config.c'; it's
effectively skipping every opcode (and therefore every option) from
the file. The change fixes that behaviour.
Signed-off-by: Artyom V. Poptsov <poptsov.artyom@gmail.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
