Detect Secure-Join request step using only Secure-Join-Invitenumber

[link2xt]

Some servers encrypt unencrypted incoming messages with the user-uploaded public OpenPGP key. We remove all protected headers such as secure-join when merging encrypted headers into unencrypted, but because encrypted payload does not have Secure-Join header, it gets erased.

To workaround this problem, detect key request (first step of secure-join protocol) using always unprotected Secure-Join-Invitenumber.

Closes #7639

[Hocuri]

Some servers encrypt unencrypted incoming messages with the user-uploaded public OpenPGP key. We remove all protected headers such as secure-join when merging encrypted headers into unencrypted, but because encrypted payload does not have Secure-Join header, it gets erased.

I didn't really understand how this can lead to the Secure-Join header going missing. But apart from that, this PR LGTM.

To be honest, all of this feels like a hack - mostly using the Secure-Join header to detect the step, except for the final step. But it is fine; we can revert once we're encrypting all securejoin messages.

[Hocuri]

Suggested change

	Some(SecureJoinStep::Request {
	// XXX We can revert this workaround once we're encrypting all Secure-Join messages.
	Some(SecureJoinStep::Request {

[link2xt]

I think we will have to keep it for compatibility until we stop processing unencrypted invites, so such comment will stay for a long time.

[link2xt]

I didn't really understand how this can lead to the Secure-Join header going missing.

Normally vc-request message is fully unencrypted and has Secure-Join: vc-request and Secure-Join-Invitenumber headers. When the server adds encrypted payload to the message (see the test how this looks like), this code runs and removes Secure-Join header because is_protected returns true for secure-join:

core/src/mimeparser.rs

Lines 530 to 540 in 9c883e6

	MimeMessage::merge_headers(
	context,
	&mut headers,
	&mut headers_removed,
	&mut recipients,
	&mut past_members,
	&mut inner_from,
	&mut list_post,
	&mut chat_disposition_notification_to,
	mail,
	);

To be honest, all of this feels like a hack - mostly using the Secure-Join header to detect the step, except for the final step.

s/final/first/

Secure-Join header that can be trusted when unprotected only for vc-request and vg-request values also looks like a hack.

Non-hacky solution would be to get rid of non-RFC9788 header protection, then always either take all headers from the outside if header protection is not used, or from the inside if header protection is used, without ever "merging" them. In this case headers from the inside will be ignored and headers from the outside accepted because the server does not use RFC 9788 and left all the headers on the outside.

[iequidoo]

Another possible solution is to not drop outer headers when merging them with encrypted-only (unsigned) headers. This is what https://www.rfc-editor.org/rfc/rfc9788.txt says:

• If the MUA detects that an incoming message has protected Header
  Fields:
  - For a Header Field that is present in the protected Header
  Section, the MUA SHOULD render the protected value and ignore
  any unprotected counterparts that may be present (with a
  special exception for the From Header Field (see Section 4.4)).
  - For a Header Field that is present only in the Outer Header
  Section, the MUA SHOULD NOT render that value. If it does
  render the value, the MUA SHOULD indicate that the rendered
  value is unprotected. [...]

Probably such an encrypted-only message doesn't have header protection and even if it does, we handle it as unencrypted.

A more on-point solution is to only aply this logic to messages w/o the standard header protection.

[iequidoo]

I think it's better to reorder the checks and first look at the protected Secure-Join header. This doesn't look as a security issue, but looking at Secure-Join first is more straightforward and closer to the current logic

[link2xt]

Then I would have to handle the case of vc-request without invite code.

The only issue is that anyone can take your encrypted message with protected Secure-Join step (or without it) and stamp Secure-Join-Invitenumber on it, then it will be processed only as an (invalid) invite. But the same effect can be achieved by dropping your message and sending invite instead.

[link2xt]

Another possible solution is to not drop outer headers when merging them with encrypted-only (unsigned) headers. This is what https://www.rfc-editor.org/rfc/rfc9788.txt says

For me it looks like RFC 9788 says that you should "render" only the protected headers if header protection is used, or only render outer headers without merging them. "Merging" exists in our code only because of pre-RFC9788 when it was considered ok to protect e.g. only Subject but not anything else and then merge protected Subject with unprotected other fields. Then we have is_protected with a list of headers that should never be merged into encrypted messages from the unprotected part, while RFC 9788 completely avoids this by saying that if you want the header from the unprotected part to be used, you need to copy it into protected part as otherwise it will be ignored.

Probably such an encrypted-only message doesn't have header protection and even if it does, we handle it as unencrypted.

If we strictly follow RFC 9788 only, we should just take all the headers from the outside and not from the inside, that would also be a solution. But it will likely break compatibility with pre-RFC 9788 header protection, so I implemented minimal change that fixes the problem.

[iequidoo]

If a message doesn't have the RFC 9788 header protection and it's encrypted-only, it's safe to merge the inner and outer headers, RFC 9788 says nothing about this explicitly, but then was_encrypted() should return false (and it already does so for encrypted-only messages). This may be a more generic solution if we hit other problems with such server-side encryption. For now this PR looks enough.

[link2xt]

If a message doesn't have the RFC 9788 header protection and it's encrypted-only, it's safe to merge the inner and outer headers

It is not safe, e.g. someone can stamp an unprotected Cc field on a message to look like it was also addressed to someone else, or add Auto-Submitted to make it appear as if it was sent by a bot (and even get the user marked as bot in DC).

This is why we have a hacky is_protected function for such merging.

Safe solution is to drop all non-RFC 9788 header protection and always either take all outer or all protected headers, but then we don't have compatibility even with K-9/Thunderbird subject protection and our own old clients.

[iequidoo]

It is not safe, e.g. someone can stamp an unprotected Cc field on a message to look like it was also addressed to someone else, or add Auto-Submitted to make it appear as if it was sent by a bot (and even get the user marked as bot in DC).

It's not safe for encrypted and signed messages, but an encrypted-only message will be displayed as unencrypted anyway (and assigned to an unencrypted chat) and the same effect can be achieved by sending an unencrypted message with Cc or Auto-Submitted.

What is wrong currently is that we don't respect "6. Replying and Forwarding Guidance" from RFC 9788 which tells that we shouldn't leak data from headers in encrypted-only messages in unencrypted replies, but we do so because we ignore that the message is encrypted if it doesn't have valid signatures.