From 59aa9d64df8b3d54feb5fac37566bac13013b729 Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Mon, 17 Mar 2025 17:00:17 +0100 Subject: [PATCH 1/5] Check return values of libgcrypt initialisation. Signed-off-by: Steffen Jaeckel --- src/omemo/crypto.c | 19 ++++++++++++++----- src/omemo/omemo.c | 5 +++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/omemo/crypto.c b/src/omemo/crypto.c index 0423d9da..8653b1d1 100644 --- a/src/omemo/crypto.c +++ b/src/omemo/crypto.c @@ -49,16 +49,25 @@ int omemo_crypto_init(void) { if (!gcry_check_version(GCRYPT_VERSION)) { - return -1; + return GPG_ERR_UNKNOWN_VERSION; } - gcry_control(GCRYCTL_SUSPEND_SECMEM_WARN); + gcry_error_t ret; + ret = gcry_control(GCRYCTL_SUSPEND_SECMEM_WARN); + if (ret != 0) + return ret; - gcry_control(GCRYCTL_INIT_SECMEM, 16384, 0); + ret = gcry_control(GCRYCTL_INIT_SECMEM, 16384, 0); + if (ret != 0) + return ret; - gcry_control(GCRYCTL_RESUME_SECMEM_WARN); + ret = gcry_control(GCRYCTL_RESUME_SECMEM_WARN); + if (ret != 0) + return ret; - gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0); + ret = gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0); + if (ret != 0) + return ret; /* Ask for a first random buffer to ensure CSPRNG is initialized. * Thus we control the memleak produced by gcrypt initialization and we can diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index 5b512c19..ac134e28 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -133,8 +133,9 @@ omemo_init(void) prof_add_shutdown_routine(_omemo_close); - if (omemo_crypto_init() != 0) { - cons_show("Error initializing OMEMO crypto: gcry_check_version() failed"); + gcry_error_t crypt_res = omemo_crypto_init(); + if (crypt_res != 0) { + cons_show("Error initializing OMEMO crypto: %s", gcry_strerror(crypt_res)); } pthread_mutexattr_init(&omemo_static_data.attr); From cffd3107449f81026fb4037607b614c6dc4e4560 Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Tue, 18 Mar 2025 10:28:05 +0100 Subject: [PATCH 2/5] Fix shadowing of `msg` resulting in invalid free. ``` ==500714== Invalid free() / delete / delete[] / realloc() ==500714== at 0x484A78B: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==500714== by 0x5BDCD58: g_free (in /usr/lib64/libglib-2.0.so.0.8200.2) ==500714== by 0x49016B: http_file_put (http_upload.c:175) ==500714== by 0x5F613B1: start_thread (in /usr/lib64/libc.so.6) ==500714== by 0x5FE63D3: clone (in /usr/lib64/libc.so.6) ==500714== Address 0x86f6510 is 0 bytes inside a block of size 53 free'd ==500714== at 0x484A78B: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==500714== by 0x5BDCD58: g_free (in /usr/lib64/libglib-2.0.so.0.8200.2) ==500714== by 0x490299: http_file_put (http_upload.c:324) ==500714== by 0x5F613B1: start_thread (in /usr/lib64/libc.so.6) ==500714== by 0x5FE63D3: clone (in /usr/lib64/libc.so.6) ==500714== Block was alloc'd at ==500714== at 0x48477C4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==500714== by 0x5F548A7: __vasprintf_internal (in /usr/lib64/libc.so.6) ==500714== by 0x5C2DD1D: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.8200.2) ==500714== by 0x5BF8D97: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.8200.2) ==500714== by 0x5BF8E58: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.8200.2) ==500714== by 0x490264: http_file_put (http_upload.c:318) ==500714== by 0x5F613B1: start_thread (in /usr/lib64/libc.so.6) ==500714== by 0x5FE63D3: clone (in /usr/lib64/libc.so.6) ``` Signed-off-by: Steffen Jaeckel --- src/tools/http_upload.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/tools/http_upload.c b/src/tools/http_upload.c index 2d051023..a1c31b27 100644 --- a/src/tools/http_upload.c +++ b/src/tools/http_upload.c @@ -298,39 +298,36 @@ http_file_put(void* userdata) pthread_mutex_lock(&lock); if (err) { - gchar* msg; + auto_gchar gchar* err_msg = NULL; if (upload->cancel) { - msg = g_strdup_printf("Uploading '%s' failed: Upload was canceled", upload->filename); - if (!msg) { - msg = g_strdup(FALLBACK_MSG); + err_msg = g_strdup_printf("Uploading '%s' failed: Upload was canceled", upload->filename); + if (!err_msg) { + err_msg = g_strdup(FALLBACK_MSG); } } else { - msg = g_strdup_printf("Uploading '%s' failed: %s", upload->filename, err); - if (!msg) { - msg = g_strdup(FALLBACK_MSG); + err_msg = g_strdup_printf("Uploading '%s' failed: %s", upload->filename, err); + if (!err_msg) { + err_msg = g_strdup(FALLBACK_MSG); } - win_update_entry_message(upload->window, upload->put_url, msg); + win_update_entry_message(upload->window, upload->put_url, err_msg); } - cons_show_error(msg); - g_free(msg); + cons_show_error(err_msg); } else { if (!upload->cancel) { - msg = g_strdup_printf("Uploading '%s': 100%%", upload->filename); - if (!msg) { - msg = g_strdup(FALLBACK_MSG); + auto_gchar gchar* status_msg = g_strdup_printf("Uploading '%s': 100%%", upload->filename); + if (!status_msg) { + status_msg = g_strdup(FALLBACK_MSG); } - win_update_entry_message(upload->window, upload->put_url, msg); + win_update_entry_message(upload->window, upload->put_url, status_msg); win_mark_received(upload->window, upload->put_url); - g_free(msg); char* url = NULL; if (format_alt_url(upload->get_url, upload->alt_scheme, upload->alt_fragment, &url) != 0) { - gchar* msg = g_strdup_printf("Uploading '%s' failed: Bad URL ('%s')", upload->filename, upload->get_url); - if (!msg) { - msg = g_strdup(FALLBACK_MSG); + auto_gchar gchar* fail_msg = g_strdup_printf("Uploading '%s' failed: Bad URL ('%s')", upload->filename, upload->get_url); + if (!fail_msg) { + fail_msg = g_strdup(FALLBACK_MSG); } - cons_show_error(msg); - g_free(msg); + cons_show_error(fail_msg); } else { switch (upload->window->type) { case WIN_CHAT: From 422f5fa6f962e649cabe851c0390402f2b87e0f0 Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Tue, 18 Mar 2025 13:00:01 +0100 Subject: [PATCH 3/5] Slightly improve http_download related allocations. * use `calloc()` to allocate zero-initialized structs. * order the allocation of members by their declaration. * order the freeing of members in reverse order. * move init of `silent` flag in plugin case. * fix double free of `tmpname` in `aesgcm_file_get()`. * make some functions static. Signed-off-by: Steffen Jaeckel --- src/command/cmd_funcs.c | 25 ++++++++++++------------- src/tools/aesgcm_download.c | 5 ++--- src/tools/http_download.c | 4 ++-- src/tools/plugin_download.c | 2 +- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 51439237..5a3c2307 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -9435,17 +9435,16 @@ _url_aesgcm_method(ProfWin* window, const char* cmd_template, gchar* url, gchar* auto_gchar gchar* filename = _prepare_filename(window, url, path); if (!filename) return; - auto_char char* id = get_random_string(4); - AESGCMDownload* download = malloc(sizeof(AESGCMDownload)); - download->window = window; + AESGCMDownload* download = calloc(1, sizeof(AESGCMDownload)); + download->id = get_random_string(4); download->url = strdup(url); download->filename = strdup(filename); - download->id = strdup(id); if (cmd_template != NULL) { download->cmd_template = strdup(cmd_template); } else { download->cmd_template = NULL; } + download->window = window; pthread_create(&(download->worker), NULL, &aesgcm_file_get, download); aesgcm_download_add_download(download); @@ -9458,38 +9457,38 @@ _download_install_plugin(ProfWin* window, gchar* url, gchar* path) auto_gchar gchar* filename = _prepare_filename(window, url, path); if (!filename) return FALSE; - HTTPDownload* download = malloc(sizeof(HTTPDownload)); - download->window = window; + HTTPDownload* download = calloc(1, sizeof(HTTPDownload)); + download->id = get_random_string(4); download->url = strdup(url); download->filename = strdup(filename); - download->id = get_random_string(4); download->cmd_template = NULL; + download->window = window; + download->silent = TRUE; pthread_create(&(download->worker), NULL, &plugin_download_install, download); plugin_download_add_download(download); return TRUE; } -void +static void _url_http_method(ProfWin* window, const char* cmd_template, gchar* url, gchar* path) { auto_gchar gchar* filename = _prepare_filename(window, url, path); if (!filename) return; - auto_char char* id = get_random_string(4); - HTTPDownload* download = malloc(sizeof(HTTPDownload)); - download->window = window; + HTTPDownload* download = calloc(1, sizeof(HTTPDownload)); + download->id = get_random_string(4); download->url = strdup(url); download->filename = strdup(filename); - download->id = strdup(id); download->cmd_template = cmd_template ? strdup(cmd_template) : NULL; + download->window = window; download->silent = FALSE; pthread_create(&(download->worker), NULL, &http_file_get, download); http_download_add_download(download); } -void +static void _url_external_method(const char* cmd_template, const char* url, gchar* filename) { auto_gcharv gchar** argv = format_call_external_argv(cmd_template, url, filename); diff --git a/src/tools/aesgcm_download.c b/src/tools/aesgcm_download.c index 6aa56013..4e10c286 100644 --- a/src/tools/aesgcm_download.c +++ b/src/tools/aesgcm_download.c @@ -77,7 +77,7 @@ aesgcm_file_get(void* userdata) // Create a temporary file used for storing the ciphertext that is to be // retrieved from the https:// URL. - auto_gchar char* tmpname = NULL; + auto_gchar gchar* tmpname = NULL; auto_gfd gint tmpfd = 0; if ((tmpfd = g_file_open_tmp("profanity.XXXXXX", &tmpname, NULL)) == -1) { http_print_transfer_update(aesgcm_dl->window, aesgcm_dl->id, @@ -101,7 +101,7 @@ aesgcm_file_get(void* userdata) // We wrap the HTTPDownload tool and use it for retrieving the ciphertext // and storing it in the temporary file previously opened. - HTTPDownload* http_dl = malloc(sizeof(HTTPDownload)); + HTTPDownload* http_dl = calloc(1, sizeof(HTTPDownload)); http_dl->window = aesgcm_dl->window; http_dl->worker = aesgcm_dl->worker; http_dl->id = strdup(aesgcm_dl->id); @@ -128,7 +128,6 @@ aesgcm_file_get(void* userdata) http_dl->bytes_received, fragment); remove(tmpname); - g_free(tmpname); if (crypt_res != GPG_ERR_NO_ERROR) { http_print_transfer_update(aesgcm_dl->window, aesgcm_dl->id, diff --git a/src/tools/http_download.c b/src/tools/http_download.c index 9058ae28..b9f47837 100644 --- a/src/tools/http_download.c +++ b/src/tools/http_download.c @@ -236,9 +236,9 @@ out: download_processes = g_slist_remove(download_processes, download); pthread_mutex_unlock(&lock); - free(download->id); - free(download->url); free(download->filename); + free(download->url); + free(download->id); free(download); return NULL; diff --git a/src/tools/plugin_download.c b/src/tools/plugin_download.c index 99176ec2..d2eae004 100644 --- a/src/tools/plugin_download.c +++ b/src/tools/plugin_download.c @@ -62,9 +62,9 @@ plugin_download_install(void* userdata) { HTTPDownload* plugin_dl = (HTTPDownload*)userdata; + /* We need to dup those, because they're free'd inside `http_file_get()` */ auto_char char* path = strdup(plugin_dl->filename); auto_char char* https_url = strdup(plugin_dl->url); - plugin_dl->silent = TRUE; http_file_get(plugin_dl); From 4405f33884187cc2146098e9ee10875678d559eb Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Tue, 18 Mar 2025 14:24:59 +0100 Subject: [PATCH 4/5] Optionally return bytes received from `http_file_get()` `http_dl->bytes_received` was already free'd when it was accessed inside `aesgcm_file_get()`. Change `http_file_get()` to optionally return the number of bytes received so we know how much data we have to decrypt. This fixes #1994 Signed-off-by: Steffen Jaeckel --- src/tools/aesgcm_download.c | 10 ++++++++-- src/tools/http_download.c | 7 ++++++- src/tools/http_download.h | 1 + 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/tools/aesgcm_download.c b/src/tools/aesgcm_download.c index 4e10c286..366a0be4 100644 --- a/src/tools/aesgcm_download.c +++ b/src/tools/aesgcm_download.c @@ -109,9 +109,15 @@ aesgcm_file_get(void* userdata) http_dl->filename = strdup(tmpname); http_dl->cmd_template = NULL; http_dl->silent = FALSE; + http_dl->return_bytes_received = TRUE; aesgcm_dl->http_dl = http_dl; - http_file_get(http_dl); // TODO(wstrm): Verify result. + ssize_t* p_bytes_received = http_file_get(http_dl); + if (!p_bytes_received) { + return NULL; + } + ssize_t bytes_received = *p_bytes_received; + free(p_bytes_received); auto_FILE FILE* tmpfh = fopen(tmpname, "rb"); if (tmpfh == NULL) { @@ -125,7 +131,7 @@ aesgcm_file_get(void* userdata) gcry_error_t crypt_res; crypt_res = omemo_decrypt_file(tmpfh, outfh, - http_dl->bytes_received, fragment); + bytes_received, fragment); remove(tmpname); diff --git a/src/tools/http_download.c b/src/tools/http_download.c index b9f47837..0753141b 100644 --- a/src/tools/http_download.c +++ b/src/tools/http_download.c @@ -105,6 +105,7 @@ void* http_file_get(void* userdata) { HTTPDownload* download = (HTTPDownload*)userdata; + ssize_t* ret = NULL; char* err = NULL; @@ -208,6 +209,10 @@ http_file_get(void* userdata) "Downloading '%s': done\nSaved to '%s'", download->url, download->filename); win_mark_received(download->window, download->id); + if (download->return_bytes_received) { + ret = malloc(sizeof(*ret)); + *ret = download->bytes_received; + } } } @@ -241,7 +246,7 @@ out: free(download->id); free(download); - return NULL; + return ret; } void diff --git a/src/tools/http_download.h b/src/tools/http_download.h index f75ba4d7..99a9fc06 100644 --- a/src/tools/http_download.h +++ b/src/tools/http_download.h @@ -58,6 +58,7 @@ typedef struct http_download_t pthread_t worker; int cancel; gboolean silent; + gboolean return_bytes_received; } HTTPDownload; void* http_file_get(void* userdata); From 6d3f20fe8ec67c562c6471486e76de678aa1416c Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Tue, 18 Mar 2025 14:25:41 +0100 Subject: [PATCH 5/5] Close `FILE*` before removing the filename. Signed-off-by: Steffen Jaeckel --- src/tools/aesgcm_download.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/aesgcm_download.c b/src/tools/aesgcm_download.c index 366a0be4..e12b6987 100644 --- a/src/tools/aesgcm_download.c +++ b/src/tools/aesgcm_download.c @@ -119,7 +119,7 @@ aesgcm_file_get(void* userdata) ssize_t bytes_received = *p_bytes_received; free(p_bytes_received); - auto_FILE FILE* tmpfh = fopen(tmpname, "rb"); + FILE* tmpfh = fopen(tmpname, "rb"); if (tmpfh == NULL) { http_print_transfer_update(aesgcm_dl->window, aesgcm_dl->id, "Downloading '%s' failed: Unable to open " @@ -132,7 +132,7 @@ aesgcm_file_get(void* userdata) gcry_error_t crypt_res; crypt_res = omemo_decrypt_file(tmpfh, outfh, bytes_received, fragment); - + fclose(tmpfh); remove(tmpname); if (crypt_res != GPG_ERR_NO_ERROR) {