Log more useful information to be able to troubleshoot sftp server

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Pavol Žáčik <pzacik@redhat.com>

doc/CMakeLists.txt

@@ -108,7 +108,6 @@ if (DOXYGEN_FOUND)
         packet_struct,
         pem_get_password_struct,
         ssh_tokens_st,
-        sftp_attributes_struct,
         sftp_client_message_struct,
         sftp_dir_struct,
         sftp_ext_struct,

include/libssh/blf.h

@@ -31,6 +31,8 @@
 #ifndef _BLF_H_
 #define _BLF_H_
 
+#include <stdint.h>
+
 //#include "includes.h"
 
 #if !defined(HAVE_BCRYPT_PBKDF) && !defined(HAVE_BLH_H)

include/libssh/chacha.h

@@ -9,6 +9,8 @@ Public domain.
 #ifndef CHACHA_H
 #define CHACHA_H
 
+#include <stdint.h>
+
 struct chacha_ctx {
     uint32_t input[16];
 };

include/libssh/chacha20-poly1305-common.h

@@ -29,6 +29,8 @@
 #ifndef CHACHA20_POLY1305_H
 #define CHACHA20_POLY1305_H
 
+#include <stdint.h>
+
 #define CHACHA20_BLOCKSIZE 64
 #define CHACHA20_KEYLEN 32
 

include/libssh/poly1305.h

@@ -5,6 +5,10 @@
 
 #ifndef POLY1305_H
 #define POLY1305_H
+
+#include <stddef.h>
+#include <stdint.h>
+
 #include "libssh/chacha20-poly1305-common.h"
 
 #ifdef __cplusplus

include/libssh/sc25519.h

@@ -9,6 +9,8 @@
 #ifndef SC25519_H
 #define SC25519_H
 
+#include <stdint.h>
+
 #define sc25519                  crypto_sign_ed25519_ref_sc25519
 #define shortsc25519             crypto_sign_ed25519_ref_shortsc25519
 #define sc25519_from32bytes      crypto_sign_ed25519_ref_sc25519_from32bytes

include/libssh/scp.h

@@ -21,6 +21,9 @@
 #ifndef _SCP_H
 #define _SCP_H
 
+#include <stddef.h>
+#include <stdint.h>
+
 enum ssh_scp_states {
   SSH_SCP_NEW,          //Data structure just created
   SSH_SCP_WRITE_INITED, //Gave our intention to write

include/libssh/sftp.h

@@ -177,28 +177,113 @@ struct sftp_status_message_struct {
     char *langmsg;
 };
 
+/**
+ * @brief SFTP file attributes structure.
+ *
+ * This type represents file attributes. It is used both for
+ * sending and receiving file attributes from the server.
+ *
+ * `flags` determines which of the struct fields are present.
+ *
+ * @see sftp_attributes_free()
+ */
 struct sftp_attributes_struct {
+    /** File name */
     char *name;
-    char *longname; /* ls -l output on openssh, not reliable else */
+
+    /** Extended name i.e output of `ls -l` (requires SFTP v3 with OpenSSH) */
+    char *longname;
+
+    /** Determines which of the struct fields are present */
     uint32_t flags;
+
+    /** File type */
     uint8_t type;
+
+    /** File size (requires flag `SSH_FILEXFER_ATTR_SIZE`) */
     uint64_t size;
+
+    /** User ID (requires SFTP v3 with flag `SSH_FILEXFER_ATTR_UIDGID`) */
     uint32_t uid;
+
+    /** Group ID (requires SFTP v3 with flag `SSH_FILEXFER_ATTR_UIDGID`) */
     uint32_t gid;
-    char *owner; /* set if openssh and version 4 */
-    char *group; /* set if openssh and version 4 */
+
+    /**
+     * File owner
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_OWNERGROUP`
+     * or SFTP v3 with OpenSSH)
+     */
+    char *owner;
+
+    /**
+     * File group
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_OWNERGROUP`
+     * or SFTP v3 with OpenSSH)
+     */
+    char *group;
+
+    /** File permissions (requires flag `SSH_FILEXFER_ATTR_PERMISSIONS`) */
     uint32_t permissions;
+
+    /**
+     * Access time
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_ACCESSTIME`)
+     */
     uint64_t atime64;
+
+    /**
+     * Access time
+     * (requires SFTP v3 with flag `SSH_FILEXFER_ATTR_ACMODTIME`)
+     */
     uint32_t atime;
+
+    /**
+     * Access time nanoseconds
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_SUBSECOND_TIMES`)
+     */
     uint32_t atime_nseconds;
+
+    /**
+     * Creation time
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_CREATETIME`)
+     */
     uint64_t createtime;
+
+    /**
+     * Creation time nanoseconds
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_SUBSECOND_TIMES`)
+     */
     uint32_t createtime_nseconds;
+
+    /**
+     * Modification time
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_MODIFYTIME`)
+     */
     uint64_t mtime64;
+
+    /**
+     * Modification time
+     * (requires SFTP v3 with flag `SSH_FILEXFER_ATTR_ACMODTIME`)
+     */
     uint32_t mtime;
+
+    /**
+     * Modification time nanoseconds
+     * (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_SUBSECOND_TIMES`)
+     */
     uint32_t mtime_nseconds;
+
+    /** ACL data (requires SFTP v4 with flag `SSH_FILEXFER_ATTR_ACL`) */
     ssh_string acl;
+
+    /** Unused */
     uint32_t extended_count;
+
+    /** Unused */
     ssh_string extended_type;
+
+    /** Unused */
     ssh_string extended_data;
 };
 
@@ -206,27 +291,55 @@ struct sftp_attributes_struct {
  * @brief SFTP statvfs structure.
  */
 struct sftp_statvfs_struct {
-  uint64_t f_bsize;   /** file system block size */
-  uint64_t f_frsize;  /** fundamental fs block size */
-  uint64_t f_blocks;  /** number of blocks (unit f_frsize) */
-  uint64_t f_bfree;   /** free blocks in file system */
-  uint64_t f_bavail;  /** free blocks for non-root */
-  uint64_t f_files;   /** total file inodes */
-  uint64_t f_ffree;   /** free file inodes */
-  uint64_t f_favail;  /** free file inodes for to non-root */
-  uint64_t f_fsid;    /** file system id */
-  uint64_t f_flag;    /** bit mask of f_flag values */
-  uint64_t f_namemax; /** maximum filename length */
+    /** file system block size */
+    uint64_t f_bsize;
+
+    /** fundamental fs block size */
+    uint64_t f_frsize;
+
+    /** number of blocks (unit f_frsize) */
+    uint64_t f_blocks;
+
+    /** free blocks in file system */
+    uint64_t f_bfree;
+
+    /** free blocks for non-root */
+    uint64_t f_bavail;
+
+    /** total file inodes */
+    uint64_t f_files;
+
+    /** free file inodes */
+    uint64_t f_ffree;
+
+    /** free file inodes for non-root */
+    uint64_t f_favail;
+
+    /** file system id */
+    uint64_t f_fsid;
+
+    /** bit mask of f_flag values */
+    uint64_t f_flag;
+
+    /** maximum filename length */
+    uint64_t f_namemax;
 };
 
 /**
  * @brief SFTP limits structure.
  */
 struct sftp_limits_struct {
-    uint64_t max_packet_length;   /** maximum number of bytes in a single sftp packet */
-    uint64_t max_read_length;     /** maximum length in a SSH_FXP_READ packet */
-    uint64_t max_write_length;    /** maximum length in a SSH_FXP_WRITE packet */
-    uint64_t max_open_handles;    /** maximum number of active handles allowed by server */
+    /** maximum number of bytes in a single sftp packet */
+    uint64_t max_packet_length;
+
+    /** maximum length in a SSH_FXP_READ packet */
+    uint64_t max_read_length;
+
+    /** maximum length in a SSH_FXP_WRITE packet */
+    uint64_t max_write_length;
+
+    /** maximum number of active handles allowed by server */
+    uint64_t max_open_handles;
 };
 
 /**

src/buffer.c

@@ -921,18 +921,10 @@ static int ssh_buffer_pack_allocate_va(struct ssh_buffer_struct *buffer,
             va_arg(ap, void *);
             count++; /* increase argument count */
             break;
-        case 'F':
         case 'B':
             b = va_arg(ap, bignum);
-            if (*p == 'F') {
-                /* For padded bignum, we know the exact length */
-                len = va_arg(ap, size_t);
-                count++; /* increase argument count */
-                needed_size += sizeof(uint32_t) + len;
-            } else {
-                /* The bignum bytes + 1 for possible padding */
-                needed_size += sizeof(uint32_t) + bignum_num_bytes(b) + 1;
-            }
+            /* The bignum bytes + 1 for possible padding */
+            needed_size += sizeof(uint32_t) + bignum_num_bytes(b) + 1;
             break;
         case 't':
             cstring = va_arg(ap, char *);
@@ -1062,16 +1054,9 @@ ssh_buffer_pack_va(struct ssh_buffer_struct *buffer,
             rc = ssh_buffer_add_data(buffer, o.data, (uint32_t)len);
             o.data = NULL;
             break;
-        case 'F':
         case 'B':
             b = va_arg(ap, bignum);
-            if (*p == 'F') {
-                len = va_arg(ap, size_t);
-                count++; /* increase argument count */
-                o.string = ssh_make_padded_bignum_string(b, len);
-            } else {
-                o.string = ssh_make_bignum_string(b);
-            }
+            o.string = ssh_make_bignum_string(b);
             if(o.string == NULL){
                 rc = SSH_ERROR;
                 break;
@@ -1127,8 +1112,6 @@ ssh_buffer_pack_va(struct ssh_buffer_struct *buffer,
  *                         'P': size_t, void * (len of data, pointer to data)
  *                              only pushes data.
  *                         'B': bignum (pushed as SSH string)
- *                         'F': bignum, size_t (bignum, padded to fixed length,
- *                              pushed as SSH string)
  * @returns             SSH_OK on success
  *                      SSH_ERROR on error
  * @warning             when using 'P' with a constant size (e.g. 8), do not

src/hybrid_mlkem.c

@@ -152,7 +152,7 @@ static int derive_hybrid_secret(ssh_session session,
 
     rc = ssh_buffer_pack(combined_secret,
                          "PP",
-                         MLKEM_SHARED_SECRET_SIZE,
+                         (size_t)MLKEM_SHARED_SECRET_SIZE,
                          mlkem_shared_secret,
                          ssh_string_len(ecdh_shared_secret),
                          ssh_string_data(ecdh_shared_secret));
@@ -244,7 +244,7 @@ int ssh_client_hybrid_mlkem_init(ssh_session session)
                              "PP",
                              ssh_string_len(crypto->mlkem_client_pubkey),
                              ssh_string_data(crypto->mlkem_client_pubkey),
-                             CURVE25519_PUBKEY_SIZE,
+                             (size_t)CURVE25519_PUBKEY_SIZE,
                              crypto->curve25519_client_pubkey);
         break;
     case SSH_KEX_MLKEM768NISTP256_SHA256:
@@ -768,7 +768,7 @@ static SSH_PACKET_CALLBACK(ssh_packet_server_hybrid_mlkem_init)
                              "PP",
                              ssh_string_len(crypto->mlkem_ciphertext),
                              ssh_string_data(crypto->mlkem_ciphertext),
-                             CURVE25519_PUBKEY_SIZE,
+                             (size_t)CURVE25519_PUBKEY_SIZE,
                              crypto->curve25519_server_pubkey);
         break;
     case SSH_KEX_MLKEM768NISTP256_SHA256:

src/kex.c

@@ -1688,11 +1688,6 @@ int ssh_make_sessionid(ssh_session session)
     switch (session->next_crypto->kex_type) {
     case SSH_KEX_SNTRUP761X25519_SHA512:
     case SSH_KEX_SNTRUP761X25519_SHA512_OPENSSH_COM:
-        rc = ssh_buffer_pack(buf,
-                             "F",
-                             session->next_crypto->shared_secret,
-                             SHA512_DIGEST_LEN);
-        break;
     case SSH_KEX_MLKEM768X25519_SHA256:
     case SSH_KEX_MLKEM768NISTP256_SHA256:
 #ifdef HAVE_MLKEM1024
@@ -1919,9 +1914,6 @@ int ssh_generate_session_keys(ssh_session session)
     switch (session->next_crypto->kex_type) {
     case SSH_KEX_SNTRUP761X25519_SHA512:
     case SSH_KEX_SNTRUP761X25519_SHA512_OPENSSH_COM:
-        k_string = ssh_make_padded_bignum_string(crypto->shared_secret,
-                                                 crypto->digest_len);
-        break;
     case SSH_KEX_MLKEM768X25519_SHA256:
     case SSH_KEX_MLKEM768NISTP256_SHA256:
 #ifdef HAVE_MLKEM1024

src/options.c

@@ -369,8 +369,8 @@ int ssh_options_set_algo(ssh_session session,
  *                default ssh directory.\n
  *                \n
  *                The ssh directory is used for files like known_hosts
- *                and identity (private and public key). It may include
- *                "%s" which will be replaced by the user home
+ *                and identity (private and public key). It may start
+ *                with ~ which will be replaced by the user home
  *                directory.
  *
  *              - SSH_OPTIONS_KNOWNHOSTS:

src/sftpserver.c

@@ -489,7 +489,10 @@ sftp_reply_name(sftp_client_message msg, const char *name, sftp_attributes attr)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending name %s", ssh_string_get_char(file));
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Sending name %s, ID %" PRIu32,
+            ssh_string_get_char(file),
+            msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_u32(out, htonl(1)) < 0 ||
@@ -532,6 +535,7 @@ int sftp_reply_handle(sftp_client_message msg, ssh_string handle)
     ssh_log_hexdump("Sending handle:",
                     (const unsigned char *)ssh_string_get_char(handle),
                     ssh_string_len(handle));
+    SSH_LOG(SSH_LOG_PROTOCOL, "packet ID %" PRIu32, msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_ssh_string(out, handle) < 0 ||
@@ -565,7 +569,7 @@ int sftp_reply_attr(sftp_client_message msg, sftp_attributes attr)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending attr");
+    SSH_LOG(SSH_LOG_PROTOCOL, "Sending attr, ID %" PRIu32, msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         buffer_add_attributes(out, attr) < 0 ||
@@ -653,7 +657,10 @@ int sftp_reply_names(sftp_client_message msg)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending %d names", msg->attr_num);
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Sending %d names, ID %" PRIu32,
+            msg->attr_num,
+            msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_u32(out, htonl(msg->attr_num)) < 0 ||
@@ -705,8 +712,11 @@ sftp_reply_status(sftp_client_message msg, uint32_t status, const char *message)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending status %d, message: %s", status,
-            ssh_string_get_char(s));
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Sending status %d, message: %s, ID %" PRIu32,
+            status,
+            ssh_string_get_char(s),
+            msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_u32(out, htonl(status)) < 0 ||
@@ -745,7 +755,10 @@ int sftp_reply_data(sftp_client_message msg, const void *data, int len)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending data, length: %d", len);
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Sending data, length: %d, ID %" PRIu32,
+            len,
+            msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_u32(out, ntohl(len)) < 0 ||
@@ -780,7 +793,7 @@ sftp_reply_statvfs(sftp_client_message msg, sftp_statvfs_t st)
         return -1;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending statvfs reply");
+    SSH_LOG(SSH_LOG_PROTOCOL, "Sending statvfs reply, ID %" PRIu32, msg->id);
 
     if (ssh_buffer_add_u32(out, msg->id) < 0 ||
         ssh_buffer_add_u64(out, ntohll(st->f_bsize)) < 0 ||
@@ -810,7 +823,9 @@ int sftp_reply_version(sftp_client_message client_msg)
     ssh_buffer reply;
     int rc;
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "Sending version packet");
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Sending version packet, ID %" PRIu32,
+            client_msg->id);
 
     version = sftp->client_version;
     reply = ssh_buffer_new();
@@ -1183,6 +1198,10 @@ process_read(sftp_client_message client_msg)
     ssh_log_hexdump("Processing read: handle:",
                     (const unsigned char *)ssh_string_get_char(handle),
                     ssh_string_len(handle));
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Offset %" PRIu64", length %" PRIu32,
+            client_msg->offset,
+            client_msg->len);
 
     h = sftp_handle(sftp, handle);
     if (h != NULL && h->type == SFTP_FILE_HANDLE) {
@@ -1241,6 +1260,10 @@ process_write(sftp_client_message client_msg)
     ssh_log_hexdump("Processing write: handle",
                     (const unsigned char *)ssh_string_get_char(handle),
                     ssh_string_len(handle));
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "Offset %" PRIu64", length %" PRIu32,
+            client_msg->offset,
+            client_msg->len);
 
     h = sftp_handle(sftp, handle);
     if (h != NULL && h->type == SFTP_FILE_HANDLE) {
@@ -1968,7 +1991,10 @@ dispatch_sftp_request(sftp_client_message sftp_msg)
     sftp_server_message_callback handler = NULL;
     uint8_t type = sftp_client_message_get_type(sftp_msg);
 
-    SSH_LOG(SSH_LOG_PROTOCOL, "processing request type: %u", type);
+    SSH_LOG(SSH_LOG_PROTOCOL,
+            "processing request type: %" PRIu8 ", ID %" PRIu32,
+            type,
+            sftp_msg->id);
 
     for (int i = 0; message_handlers[i].cb != NULL; i++) {
         if (type == message_handlers[i].type) {

src/sntrup761.c

@@ -28,7 +28,6 @@
 #include "libssh/sntrup761.h"
 #ifdef HAVE_SNTRUP761
 
-#include "libssh/bignum.h"
 #include "libssh/buffer.h"
 #include "libssh/crypto.h"
 #include "libssh/dh.h"
@@ -141,7 +140,7 @@ static int ssh_sntrup761x25519_build_k(ssh_session session)
 {
     unsigned char ssk[SNTRUP761_SIZE + CURVE25519_PUBKEY_SIZE];
     unsigned char *k = ssk + SNTRUP761_SIZE;
-    unsigned char hss[SHA512_DIGEST_LEN];
+    void *shared_secret_data = NULL;
     int rc;
 
     rc = ssh_curve25519_create_k(session, k);
@@ -216,22 +215,27 @@ static int ssh_sntrup761x25519_build_k(ssh_session session)
     ssh_log_hexdump("kem key", ssk, SNTRUP761_SIZE);
 #endif
 
-    sha512(ssk, sizeof ssk, hss);
-
-    bignum_bin2bn(hss, sizeof hss, &session->next_crypto->shared_secret);
-    if (session->next_crypto->shared_secret == NULL) {
+    ssh_string_burn(session->next_crypto->hybrid_shared_secret);
+    ssh_string_free(session->next_crypto->hybrid_shared_secret);
+    session->next_crypto->hybrid_shared_secret =
+        ssh_string_new(SHA512_DIGEST_LEN);
+    if (session->next_crypto->hybrid_shared_secret == NULL) {
+        ssh_set_error_oom(session);
         rc = SSH_ERROR;
         goto cleanup;
     }
+    shared_secret_data =
+        ssh_string_data(session->next_crypto->hybrid_shared_secret);
+
+    sha512(ssk, sizeof ssk, shared_secret_data);
 
 #ifdef DEBUG_CRYPTO
-    ssh_print_bignum("Shared secret key", session->next_crypto->shared_secret);
+    ssh_log_hexdump("Shared secret key", shared_secret_data, SHA512_DIGEST_LEN);
 #endif
 
     return 0;
 cleanup:
     ssh_burn(ssk, sizeof ssk);
-    ssh_burn(hss, sizeof hss);
 
     return rc;
 }

tests/unittests/torture_buffer.c

@@ -267,10 +267,10 @@ static void torture_ssh_buffer_bignum(void **state)
     bignum num = NULL;
     int rc;
     size_t len;
-    uint8_t verif[] = "\x00\x00\x00\x04" /* len 4 byte */
-                      "\x00\x00\x00\xff" /* padded 255 */
-                      "\x00\x00\x00\x02" /* len 2 byte */
-                      "\x00\xff";        /* padded 255 */
+    uint8_t verif[] = "\x00\x00\x00\x02" /* len */
+                      "\x00\xff"         /* pad, num */
+                      "\x00\x00\x00\x02" /* len */
+                      "\x00\xff";        /* pad, num */
 
     (void)state;
 
@@ -283,20 +283,13 @@ static void torture_ssh_buffer_bignum(void **state)
     rc = bignum_set_word(num, 255);
     assert_int_equal(rc, 1);
 
-    rc = ssh_buffer_pack(buffer, "FB", num, (size_t)4, num);
+    rc = ssh_buffer_pack(buffer, "BB", num, num);
     assert_int_equal(rc, SSH_OK);
 
     len = ssh_buffer_get_len(buffer);
     assert_int_equal(len, sizeof(verif) - 1);
     assert_memory_equal(ssh_buffer_get(buffer), verif, sizeof(verif) - 1);
 
-    /* negative test -- this number requires 3 bytes */
-    rc = bignum_set_word(num, 256 * 256);
-    assert_int_equal(rc, 1);
-
-    rc = ssh_buffer_pack(buffer, "FB", num, (size_t)2, num);
-    assert_int_equal(rc, SSH_ERROR);
-
     bignum_safe_free(num);
 
     SSH_BUFFER_FREE(buffer);