1658839 - SyntaxError when searching for "outer" MIME delimiter(s)

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Kim A. Brandt]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

I try to open a GPG-encrypted message.

Actual results:

Only a blank page is shown.

The following debug- and error-logs are given:

Debug:

SyntaxError: nothing to repeat
    extractProtectedHeaders chrome://openpgp/content/modules/mime.jsm:268
    onStopRequest chrome://openpgp/content/modules/mimeVerify.jsm:439
    returnData chrome://openpgp/content/modules/mimeDecrypt.jsm:826
    onStopRequest chrome://openpgp/content/modules/mimeDecrypt.jsm:636

Error:

mimeDecrypt.jsm: returnData(): mimeSvc.onDataAvailable failed:
SyntaxError: nothing to repeat

The first line of the debug-message is pointing to this code:

  // search for "outer" MIME delimiter(s)
  let r = new RegExp("^--" + bound, "mg");

The boundary inside the encrypted message is:

=-EAsK5KN++4TRonUEX+gd

The whole Content-Type is:

Content-Type: multipart/encrypted; protocol="application/pgp-encrypted";
	boundary="=-EAsK5KN++4TRonUEX+gd"

If I understand correctly, then this code will be executed and result in the error given above:

let r = new RegExp("^--" + "=-EAsK5KN++4TRonUEX+gd", "mg");

It seams to be the ++ that causes the error.

Expected results:

The message should have been correctly decrypted and displayed in the Message Reader.

Comment 1

[Arvidt]

Looks for me like there is missing an escaping of the boundary string before searching for it with RegEx.
Something like

function escapeRegExp(string) {
  return string.replace(/[.*+\-?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
}

mentioned here.

Comment 2

[Kim A. Brandt]

(In reply to bugzilla0248 from comment #1)

Looks for me like there is missing an escaping of the boundary string before searching for it with RegEx.
Something like

function escapeRegExp(string) {
  return string.replace(/[.*+\-?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
}

mentioned here.

I'm no expert, but reading the mentioned documentation has this section in it:

Why isn't this built into JavaScript? There is a proposal to add such a function to RegExp, but it was rejected by TC39.

Which links here: https://github.com/benjamingr/RegExp.escape/issues/37
The gist of it is in the first comment:

...the committee declined to accept this proposal as-is. In the end, the concern (largely driven by @erights, although others were sympathetic) was that escaping cannot be done in a way that is totally safe in all cases, ...

My thought now is: is the use of a regular expression - for searching for "outer" MIME delimiter(s) - the right aproach?! It looks simple enough to search for a fixed string, such as:

--=-EAsK5KN++4TRonUEX+gd

at the beginning of every line!?

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1658839_pgp_mime_regex.patch

Not sure if all of these have a realistic chance of ever getting hit. But better safe than sorry.

Comment 4

[Patrick Brunschwig]

Comment on attachment 9169824 [details] [diff] [review]
bug1658839_pgp_mime_regex.patch

Review of attachment 9169824 [details] [diff] [review]:
-----------------------------------------------------------------

This is definitely reasonable. Better safe than sorry.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/21119ccba679
fix RegExp escaping where needed in OpenPGP code. r=PatrickBrunschwig

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 9169824 [details] [diff] [review]
bug1658839_pgp_mime_regex.patch

Review of attachment 9169824 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +2914,1 @@
>  

This seems incorrect. How can you assume that indentStr is always "> " ? Apparently that cannot be assumed in general, because the code above explicitly checks for that equality.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 9169824 [details] [diff] [review]
bug1658839_pgp_mime_regex.patch

Review of attachment 9169824 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +2909,5 @@
>          pgpBlock = pgpBlock.replace(/^>>/gm, "> >");
>        }
>  
>        // Delete indentation
> +      indentRegexp = new RegExp("^> ", "gm");

This was the line that I was referring to

Comment 8

[Kai Engert (:KaiE:)]

Also, variable indentStr will continue to be used further down inside the function.

I think the variable shouldn't be changed, but only the escaped string should be used for constructing the regular expression.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Bug 1658839 - Follow up to preserve indentStr for later use, and undo one change. r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c61996640a7f
Follow up to preserve indentStr for later use, and undo one change. r=mkmelin DONTBUILD

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 9169824 [details] [diff] [review]
bug1658839_pgp_mime_regex.patch

[Approval Request Comment]
Regression caused by (bug #): No
User impact if declined: Some OpenPGP messages handled incorrectly.
Testing completed (on c-c, etc.): Only manually.
Risk to taking this patch (and alternatives if risky): Low risk.

Note: Need the follow-up patch in this bug.

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9169824 [details] [diff] [review]
bug1658839_pgp_mime_regex.patch

[Triage Comment]
Approved for beta
Approved for esr78

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9170332 [details]
Bug 1658839 - Follow up to preserve indentStr for later use, and undo one change. r=mkmelin

[Triage Comment]
Approved for beta
Approved for esr78

Comment 14

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/releases/comm-beta/rev/87457c1ae27f073cdcc1e83d0e1b5ffa148c720c
https://hg.mozilla.org/releases/comm-beta/rev/8e28b024c8fe4bc5a0d9b232877a8d5320987501
https://hg.mozilla.org/releases/comm-esr78/rev/0d8ef0d03b83b3a23cbd222dcb897818522b6bd8
https://hg.mozilla.org/releases/comm-esr78/rev/0fb8434be46c8cf52481b3767b2dca8272d90151