Comment 5 for bug 1991650

Revision history for this message
Nishit Majithia (0xnishit) wrote (last edit ):

I reviewed libssh2 1.10.0-3 as checked into kinetic. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

libssh2 is a client-side C library implementing the SSH2 protocol

- CVE History:
  - CVE-2015-1782
  - CVE-2016-0787
  - CVE-2019-13115
  - CVE-2019-17498
  - CVE-2019-3855
  - CVE-2019-3856
  - CVE-2019-3857
  - CVE-2019-3858
  - CVE-2019-3859
  - CVE-2019-3860
  - CVE-2019-3861
  - CVE-2019-3862
  - CVE-2019-3863
- Build-Depends?
  - Optional dependency on GnuPG, libgcrypt and OpenSSH
  - Build produces many deprecation warnings because of openssl, but upstream
    hasn't decided how to deal with it.
- pre/post inst/rm scripts?
  - none
- init scripts?
  - none
- systemd units?
  - none
- dbus services?
  - none
- setuid binaries?
  - none
- binaries in PATH?
  - none
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- unit tests / autopkgtests?
  - yes
- cron jobs?
  - none
- Build logs:
  - Many compailer warnings mentioned in log.txt
  - Lintian failed
- Processes spawned?
  - Yes, libssh2 client is using libssh2_channel_exec() and
    libssh2_channel_subsystem() calls in libssh2.h file
- Memory management?
  - Yes, libssh2 is mainly using [c/m/re]alloc(), memcpy(), memmove() and
    memory functions of mbedtls_. Use of these functions looks safe
- File IO?
  - Few file IO calls to readand write contect in channel.c, sftp.c, packet.c,
    openssl.c, libcrypt.c, userauth.c and scp.c
- Logging?
  - logging looks fine and logs are informative
- Environment variable usage?
  - Just one at /include/libssh2.h:762
- Use of privileged functions?
  - ioctl function used in src/session.c, looks fine
- Use of cryptography / random number sources etc?
  - Majority of the use in openssl.c
- Use of temp files?
  - none, just in examples
- Use of networking?
  - Use multiple places to start/stop socket connections.
- Use of WebKit?
  - none
- Use of PolicyKit?
  - none
- Any significant cppcheck results?
  - none
- Any significant Coverity results?
  - Few deadcode issues, few buffer overrun issues in userauth.c,
    bcrypt_pbkdf.c, knownhost.c and transport.c
- Any significant shellcheck results?
  - none
- Any significant bandit results?
  - none

Found NULL pointer dereference issue in openssl.c when session object is NULL
and calling \_libssh2_error() method will cause the null pointer dereference
resulting crash of the client application.
They fixed the issue in no time: https://github.com/libssh2/libssh2/pull/796

There are 50+ open bugs and 25 PRs pending on upstream to fix the code.

Security team ACK for promoting libssh2 to main conditional upon improving code
quality(remove dead codes and improve code comments) reported by coverity and fix this possible issue

1. NULL pointer derefernce issue in openssl.c of `decrypted` pointer
src/openssl.c:3082:29:
  deref_ptr_in_call: Dereferencing pointer "decrypted".
src/misc.c:772:5:
  deref_parm_in_call: Function "_libssh2_get_u32" dereferences "buf".
src/misc.c:737:5:
  deref_parm_in_call: Function "_libssh2_check_length" dereferences "buf".
src/misc.c:849:5:
  deref_parm: Directly dereferencing parameter "buf".
src/openssl.c:3148:8:
  check_after_deref: Null-checking "decrypted" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

I will report this issue to upstream and wait for them to fix this
thanks