proton-bridge

proton-bridge copied to clipboard

Trailing whitespaces are removed from messages, meaning that the messages sent are not exactly similar to the messages written. This is a problem in some cases (see context) and should not happen.

In the bridge, writeEncryptedTextPart is calling crypto.NewPlainMessage on line 129.

Now the problem is that crypto.NewPlainMessage calls CanonicalizeAndTrim which itself call strings.TrimRight(lines[i], " \t\r"), removing the whitespaces.

Context

I am trying to use Protonmail with git-send-email, to use e.g. on SourceHut but other people may use it to contribute on other projects (such as the Linux kernel). Unfortunately, sending git patches requires the content to be exactly what it's supposed to be, whitespaces and all. So automatic removal of whitespaces leads the patches to be seen as invalid by git.

Expected Behavior

Messages should be sent exactly as they are written.

Current Behavior

Whitespaces are removed in (otherwise empty) newlines.

Possible Solution

I would recommend just that: do not trim or add whitespaces. If there are some strong reasons to do that by default, then please add an option to the bridge so that it can be manually turned off.

Problem encountered on versions <= 1.8.7

Note: I know that there is a simple workaround passing whitespace=fix to git, however the patch is not applied locally and this option is not passed on the remote server, so there's no other solution than fixing the bridge for me and anyone else wanting to use protonmail with git patches.

hydroxide shouldn't change the contents of the emails. The comment in #18 is about header fields, not the body.

FWIW, I've been using git-send-email with hydroxide and never had whitespace issues.

OK, thanks, guess I'll test hydroxide in the meantime

Oh. Sorry, I thought this was an hydroxide bug report, I didn't expect to get a notification for the official protonmail bridge.

I think it would be helpful to protonmail devs if you could provide an example of a mungled email.

Thanks for the report. In general, bridge does not modify whitespace. One exception to this could be if you have changed your account settings to send plaintext email, or if you have specified for a given contact to only send plaintext to them, and yet you send text/html instead. In this case, bridge will render it as plaintext, breaking stuff. An example would be indeed helpful.

Thanks for the reply. Git send-email indeed relies on plaintext but text/html should not be used anywhere so this should not be the problem. Unfortunately I already switched to Hydroxide, which so far works fine, so I cannot provide new examples but you can check some of the patches that failed:

• you can access the patch here
• see the result of the apply patch there

To reproduce the issue locally you can do:

git clone https://git.sr.ht/~tfardet/NNGT
git checkout b351ebbdbab22b54a469051a281076936b07be72
curl -s https://lists.sr.ht/~tfardet/nngt-developers/patches/20470/mbox | git am -3

this fails.

Then do instead curl -s https://lists.sr.ht/~tfardet/nngt-developers/patches/20470/mbox | git am -3 --whitespace=fix which enables to merge the patch without issue (showing that it is a whitespace problem)

Of course you have to trust me on the fact that I did not manually edit the patch...

Actually comparing the diff between the actual applied patch and the proposed patch, one can see a missing whitespace on the newline after u = GraphView(u, efilt=label_parallel_edges(u).fa == 0)

Hi there, sorry to bump the issue but I was wondering whether there had been any progress: bug is still present on 1.8.7 and since you are now checking for a x-pm-appversion header, hydroxide broke and I'm left without a working git-sendemail with my protonmail address...

EDIT: as a reminder, the problem is that the bridge removes the whitespaces in (otherwise empty) newlines but these whitespaces are an intrinsic part of the git patch.

Also, since a example was asked:

1. Send an email to yourself:

This is a test email with whitespaces on newlines.
 
There is a whitespace on the lines above and below.
 
 

2. Send it via e.g. Thunderbird (in text-only) with the bridge.
3. Check that the email that arrives is missing whitespaces.

@andrzejsza after reading through most of the code I thought could be relevant, I believe the bug comes from gopenpgp:

In the bridge, writeEncryptedTextPart is calling crypto.NewPlainMessage on line 129.

Now the problem is that crypto.NewPlainMessage calls CanonicalizeAndTrim which itself call strings.TrimRight(lines[i], " \t\r"), removing the whitespaces.

AFAIK, canonicalization should only happen when computing the message signature. The payload shouldn't get canonicalized.

OK, I don't know, then... could anyone please just check whether they can actually reproduce the bug and, if so, could someone who knows what they're doing (unlike me) check where it comes from?

What I mean is that you've probably found the cause of the bug, and I'm trying to explain how to fix it: don't canonicalize the payload when encrypting, only canonicalize the data passed to the hashing function for signing.

Yep, that would be it.

Hey, finally got a chance to look into this.

The parts in the code you linked to are unrelated (they're used for importing messages, not sending/building).

However, the root cause is definitely related to what you discovered: we should be calling crypto.NewPlainMessage(...) instead of crypto.NewPlainMessageFromString(...) at pkg/pmapi/keyring.go:328, because the FromString alternative of the method includes a canonicalization step.

This should fix it:

diff --git a/pkg/pmapi/keyring.go b/pkg/pmapi/keyring.go
index 61552b9..546fe98 100644
--- a/pkg/pmapi/keyring.go
+++ b/pkg/pmapi/keyring.go
@@ -325,7 +325,7 @@ func encryptSymmDecryptKey(
 		return
 	}
 
-	pgpMessage, err := firstKey.Encrypt(crypto.NewPlainMessageFromString(textToEncrypt), kr)
+	pgpMessage, err := firstKey.Encrypt(crypto.NewPlainMessage([]byte(textToEncrypt)), kr)
 	if err != nil {
 		return
 	}

OK, thanks for checking, but in my previous message I also mentioned that crypto.NewPlainMessage is calling CanonicalizeAndTrim so I'm not sure why you expect this change to solve the issue... could you explain?

crypto.NewPlainMessage does not call internal.CanonicalizeAndTrim. I think you may have misread the function(s) you linked to.

Here the two functions (the first just calls clone, the second calls CanonicalizeAndTrim):

func NewPlainMessage(data []byte) *PlainMessage {
	return &PlainMessage{
		Data:     clone(data),
		TextType: false,
		Filename: "",
		Time:     uint32(GetUnixTime()),
	}
}

func NewPlainMessageFromString(text string) *PlainMessage {
	return &PlainMessage{
		Data:     []byte(internal.CanonicalizeAndTrim(text)),
		TextType: true,
		Filename: "",
		Time:     uint32(GetUnixTime()),
	}
}

Ah, then it's not the one from gopenpgp I guess, because that one does call CanonicalizeAndTrim... Could you link where that version of the function comes from?

Right there, follow the link you just posted and read the implementation of NewPlainMessage (few lines above what you linked to). It doesn't call CanonicalizeAndTrim. The fix is to use it.

Right, must be tired, thanks!

Hey, finally got a chance to look into this.

The parts in the code you linked to are unrelated (they're used for importing messages, not sending/building).

However, the root cause is definitely related to what you discovered: we should be calling crypto.NewPlainMessage(...) instead of crypto.NewPlainMessageFromString(...) at pkg/pmapi/keyring.go:328, because the FromString alternative of the method includes a canonicalization step.

This should fix it:

diff --git a/pkg/pmapi/keyring.go b/pkg/pmapi/keyring.go
index 61552b9..546fe98 100644
--- a/pkg/pmapi/keyring.go
+++ b/pkg/pmapi/keyring.go
@@ -325,7 +325,7 @@ func encryptSymmDecryptKey(
 		return
 	}
 
-	pgpMessage, err := firstKey.Encrypt(crypto.NewPlainMessageFromString(textToEncrypt), kr)
+	pgpMessage, err := firstKey.Encrypt(crypto.NewPlainMessage([]byte(textToEncrypt)), kr)
 	if err != nil {
 		return
 	}

Hi again, may I ask what's the plan for this? Is someone handling it, should I make the PR myself?

James's fix is already merged - it will be out with the next Bridge's release (most likely as 1.8.8).

so we were too optimistic. in internal discussions it turned out that it's not as simple as just using crypto.NewPlainMessage(...) everywhere:

Need to use crypto.NewPlainMessage(...) for unsigned and crypto.NewPlainMessageFromString(...) for signed stuff. Should separate recipients by clearsigned and non-clearsigned, and make a separate package for them (because right now you can have clearsigned and non-clearsigned recipients in the same package but that doesn't work anymore if we use crypto.NewPlainMessage(...) sometimes).

keeping the issue open but the fix won't be that soon.

OK, thanks for the notice.

Out of curiosity, why is the removal of trailing whitespaces important? Could it not simply be fixed by stopping this removal altogether in the library, at least for the message content? Like including an argument to the function so that it skips trimming and making sure that this is done for the message content.

It's only important for clearsigned messages. The spec requires trimming trailing whitespace from each line; that's just an unfortunate limitation of the format.

See for instance Inline PGP in E-mail is bad, Mm'kay?:

Why is it bad?
- [...]
- Sending 'diff' patches via inline PGP signed e-mail trigger space stuffing, which break cut'n'paste into 'patch'.

The proper "fix" in bridge is to distinguish between clearsigned and non-clearsigned messages while sending so that trimming whitespace only occurs when sending clearsigned (which is the only place where it must be done).

Hi, has there since been any updates on possible fixes for this bug?

sorry, no progress made here yet. for internal reference GODT-1224

This is a huge blocker for using ProtonMail with mailing lists fyi. Contributing to mailing-list based open source projects is a huge pain without this. And having that ability is very important to me as an open source contributor.

I know that point is originally stated in the OP, but as a company that seems to do a lot of open source work, hopefully you can empathize.

This is a big issue for anyone that wants to contribute to mailing list based projects. Alternatively provide an smtp endpoint to send unencrypted emails (they're patches, encryption isn't really needed).

FWIW, this is the main reason why I'm using/maintaining hydroxide. Contributing to mailing list based projects like Linux is part of my daily workflow, and is not possible with the official bridge.

@emersion will definitely try hydroxide, thanks for the information. Its existence is not an excuse for Proton to abandon this issue though. I think the intersection of privacy caring people and mailing list projects contributors could be bigger.