fix(pgp): prevent plaintext fallback in PGP/OX encryption #143

Manually merged

jabber.developer merged 1 commits from fix/unencrypted-send into master 2026-06-29 15:09:30 +00:00

Conversation 1 Commits 1 Files Changed 3 +38 -12

jabber.developer commented 2026-06-29 14:14:29 +00:00

Owner

Copy Link

Introduced change

Block all code paths in message_send_chat_pgp that previously fell back to sending unencrypted plaintext when encryption failed, no key was configured, or LibGPGME was unavailable. Each path now displays a specific error in the chat window and returns NULL so the caller skips logging and display.

Capabilities

• No plaintext fallback: PGP encryption failure, missing PGP key, and missing LibGPGME all produce a visible error and abort the send
• Explicit error messages: p_gpg_encrypt now returns a detailed error string via gchar** err — missing recipient key, missing sender key, GPGME context failure, or encryption failure
• Visible errors: All error messages use win_println on the current chat window instead of cons_show, so users see them even when the chat is not focused
• OX signcrypt error: OX encryption failure now also uses win_println on the current window

Reasoning behind the change

Previously, if p_gpg_encrypt returned NULL (e.g. recipient had no key, or GPGME operation failed), the function silently constructed a message with the plaintext body and sent it over the wire. This defeats the purpose of PGP encryption entirely and could leak sensitive messages.

The same issue existed for the other two branches: when no PGP key was configured for the account, or when LibGPGME was not compiled in, the function still sent the message as plaintext.

Users need to know why encryption failed so they can fix the root cause — add a recipient key, configure their own key, or check their GPG setup.

Implementation details

p_gpg_encrypt API change (src/pgp/gpg.c, src/pgp/gpg.h)

Added gchar** err out-parameter. Each failure path sets a descriptive message:

Failure	Error message
No pubkey in hash	"No PGP key found for recipient"
Pubkey has no ID	"No key ID found for recipient"
GPGME context creation	"Failed to create GPGME context: <source> <strerror>"
Receiver key lookup	"Failed to get receiver key '<id>': <source> <strerror>"
Sender key lookup	"Failed to get sender key '<fp>': <source> <strerror>"
Encryption operation	"Encryption failed: (<source>) <strerror>"

message_send_chat_pgp (src/xmpp/message.c)

Three error paths replaced plaintext fallback with win_println + return NULL:

1. Encryption fails — shows "Unable to encrypt message for <jid>: <err>"
2. No PGP key configured — shows "No PGP key configured for this account."
3. LibGPGME not compiled in — shows "LibGPGME not available, message not sent."

All paths free id (allocated via connection_create_stanza_id()) before returning to avoid a memory leak.

message_send_chat_ox (src/xmpp/message.c)

Replaced cons_show with win_println on the current window for OX signcrypt failure, consistent with the PGP changes.

Memory leak fix

gpgme_key_unref(receiver_key) was missing from the sender_key failure path in p_gpg_encrypt.

Testing

• Send a message via PGP with a recipient that has no configured key — should show error, no plaintext sent
• Send a message via PGP when LibGPGME is not compiled in — should show error, no plaintext sent
• Send a message via OX with a broken GPG setup — should show error in chat window

Resolves #142

## Introduced change Block all code paths in `message_send_chat_pgp` that previously fell back to sending unencrypted plaintext when encryption failed, no key was configured, or LibGPGME was unavailable. Each path now displays a specific error in the chat window and returns `NULL` so the caller skips logging and display. ### Capabilities - **No plaintext fallback**: PGP encryption failure, missing PGP key, and missing LibGPGME all produce a visible error and abort the send - **Explicit error messages**: `p_gpg_encrypt` now returns a detailed error string via `gchar** err` — missing recipient key, missing sender key, GPGME context failure, or encryption failure - **Visible errors**: All error messages use `win_println` on the current chat window instead of `cons_show`, so users see them even when the chat is not focused - **OX signcrypt error**: OX encryption failure now also uses `win_println` on the current window ## Reasoning behind the change Previously, if `p_gpg_encrypt` returned `NULL` (e.g. recipient had no key, or GPGME operation failed), the function silently constructed a message with the plaintext body and sent it over the wire. This defeats the purpose of PGP encryption entirely and could leak sensitive messages. The same issue existed for the other two branches: when no PGP key was configured for the account, or when LibGPGME was not compiled in, the function still sent the message as plaintext. Users need to know *why* encryption failed so they can fix the root cause — add a recipient key, configure their own key, or check their GPG setup. ## Implementation details ### `p_gpg_encrypt` API change (`src/pgp/gpg.c`, `src/pgp/gpg.h`) Added `gchar** err` out-parameter. Each failure path sets a descriptive message: | Failure | Error message | |---|---| | No pubkey in hash | `"No PGP key found for recipient"` | | Pubkey has no ID | `"No key ID found for recipient"` | | GPGME context creation | `"Failed to create GPGME context: <source> <strerror>"` | | Receiver key lookup | `"Failed to get receiver key '<id>': <source> <strerror>"` | | Sender key lookup | `"Failed to get sender key '<fp>': <source> <strerror>"` | | Encryption operation | `"Encryption failed: (<source>) <strerror>"` | ### `message_send_chat_pgp` (`src/xmpp/message.c`) Three error paths replaced plaintext fallback with `win_println` + `return NULL`: 1. **Encryption fails** — shows `"Unable to encrypt message for <jid>: <err>"` 2. **No PGP key configured** — shows `"No PGP key configured for this account."` 3. **LibGPGME not compiled in** — shows `"LibGPGME not available, message not sent."` All paths free `id` (allocated via `connection_create_stanza_id()`) before returning to avoid a memory leak. ### `message_send_chat_ox` (`src/xmpp/message.c`) Replaced `cons_show` with `win_println` on the current window for OX signcrypt failure, consistent with the PGP changes. ### Memory leak fix `gpgme_key_unref(receiver_key)` was missing from the sender_key failure path in `p_gpg_encrypt`. ## Testing - Send a message via PGP with a recipient that has no configured key — should show error, no plaintext sent - Send a message via PGP when LibGPGME is not compiled in — should show error, no plaintext sent - Send a message via OX with a broken GPG setup — should show error in chat window Resolves #142

jabber.developer added 1 commit 2026-06-29 14:14:30 +00:00

fix(pgp): prevent plaintext fallback in PGP/OX encryption ... 622054fc6d

Block all three code paths in message_send_chat_pgp that previously
fell back to sending unencrypted messages: encryption failure, missing
PGP key, and missing LibGPGME. Each path now shows a specific error
in the chat window and returns NULL so the caller skips logging and
display.

Also replace the generic cons_show for OX signcrypt failure with a
win_println on the current window, ensuring the user sees the error
even when the chat window is not focused.

Improve p_gpg_encrypt error reporting by adding a gchar** err
out-parameter. Each failure path now sets a descriptive message
(e.g. missing recipient key, missing sender key, GPGME context
failure, encryption failure) so the caller can display the exact
reason to the user.

Fix a memory leak: gpgme_key_unref(receiver_key) was missing from
the sender_key failure path.

jabber.developer2 commented 2026-06-29 14:46:47 +00:00

Collaborator

Copy Link

Maybe worth a look before merge

• p_gpg_encrypt can return NULL without setting *err — src/pgp/gpg.c, the else of if (cipher_str). If gpgme_data_release_and_get_mem() ever returns NULL after a successful encrypt, the function returns NULL but never assigns *err, and the caller then does win_println(..., "%s", err) with err == NULL (prints (null)). Not a crash or a plaintext leak — just a confusing message — but it seems to be the one path that breaks this PR's own "NULL ⇒ err set" contract. Perhaps something like *err = g_strdup("Encryption produced no ciphertext");?

Minor — and possibly pre-existing

• cipher looks leaked on the encrypt-error branch — src/pgp/gpg.c, the if (error) right after gpgme_op_encrypt. plain/ctx/keys are released, but cipher doesn't appear to be. Pre-existing — this PR only added the *err = … line into that branch — so not introduced here, just adjacent. A gpgme_data_release(cipher); before the return would cover it, if it's worth sweeping up.
• OX failure return may leak a few allocations — src/xmpp/message.c, the signcrypt_e == NULL early return. id, account, message, openpgp, signcrypt and the c buffer don't look freed there. Also pre-existing (the PR only swapped cons_show → win_println on this path), but since failing-closed is now a normal outcome it might be worth tidying.

Nits — probably fine as-is

• *err is written on every error path with no NULL-guard and isn't initialized at entry; the sibling p_gpg_valid_key() guards each write with if (err_str). Not reachable today (the only caller passes a real pointer), so this is just a consistency thought.
• The failure messages are gated behind if (current), so if wins_get_current() were ever NULL the user would get only a log line. Almost certainly never NULL in an interactive send, so likely a non-issue.

Things that checked out

• A sibling MUC PGP plaintext path seemed possible, but message_send_groupchat() doesn't appear to do per-message PGP/OX at all here, so there's likely nothing to fix there.
• The account->jid deref in the OX path looks byte-identical to before this PR, so it seems out of scope.

## Maybe worth a look before merge - **`p_gpg_encrypt` can return `NULL` without setting `*err`** — `src/pgp/gpg.c`, the `else` of `if (cipher_str)`. If `gpgme_data_release_and_get_mem()` ever returns `NULL` after a *successful* encrypt, the function returns `NULL` but never assigns `*err`, and the caller then does `win_println(..., "%s", err)` with `err == NULL` (prints `(null)`). Not a crash or a plaintext leak — just a confusing message — but it seems to be the one path that breaks this PR's own "NULL ⇒ err set" contract. Perhaps something like `*err = g_strdup("Encryption produced no ciphertext");`? ## Minor — and possibly pre-existing - **`cipher` looks leaked on the encrypt-error branch** — `src/pgp/gpg.c`, the `if (error)` right after `gpgme_op_encrypt`. `plain`/`ctx`/keys are released, but `cipher` doesn't appear to be. *Pre-existing* — this PR only added the `*err = …` line into that branch — so not introduced here, just adjacent. A `gpgme_data_release(cipher);` before the return would cover it, if it's worth sweeping up. - **OX failure return may leak a few allocations** — `src/xmpp/message.c`, the `signcrypt_e == NULL` early return. `id`, `account`, `message`, `openpgp`, `signcrypt` and the `c` buffer don't look freed there. Also *pre-existing* (the PR only swapped `cons_show` → `win_println` on this path), but since failing-closed is now a normal outcome it might be worth tidying. ## Nits — probably fine as-is - `*err` is written on every error path with no NULL-guard and isn't initialized at entry; the sibling `p_gpg_valid_key()` guards each write with `if (err_str)`. Not reachable today (the only caller passes a real pointer), so this is just a consistency thought. - The failure messages are gated behind `if (current)`, so if `wins_get_current()` were ever `NULL` the user would get only a log line. Almost certainly never `NULL` in an interactive send, so likely a non-issue. ## Things that checked out - A sibling MUC PGP plaintext path seemed possible, but `message_send_groupchat()` doesn't appear to do per-message PGP/OX at all here, so there's likely nothing to fix there. - The `account->jid` deref in the OX path looks byte-identical to before this PR, so it seems out of scope.

jabber.developer manually merged commit 622054fc6d into master 2026-06-29 15:09:30 +00:00

jabber.developer referenced this pull request 2026-06-29 15:37:43 +00:00

PGP messages are not being encrypted if key is expired #142

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

Compat/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

1

The priority is critical

Priority

High

2

The priority is high

Priority

Low

4

The priority is low

Priority

Medium

3

The priority is medium

Reviewed

Confirmed

1

Issue has been confirmed

Reviewed

Duplicate

2

This issue or pull request already exists

Reviewed

Invalid

3

Invalid issue

Reviewed

Won't Fix

3

This issue won't be fixed

Status

Abandoned

3

Somebody has started to work on this but abandoned work

Status

Blocked

1

Something is blocking this issue or pull request

Status

Need More Info

2

Feedback is required to reproduce issue or to continue work

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

admin jabber.developer jabber.developer2

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: devs/cproof#143

No description provided.