1301469 - `PushCrypto.decodeMsg` should reject instead of throwing for bad crypto headers

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler (ex-Mozilla)]

Passing an invalid Base64-encoded sender public key to `decodeMsg` will cause `ChromeUtils.base64URLDecode` to throw, which bubbles up to http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/dom/push/PushService.jsm#817-818 instead of being handled by http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/dom/push/PushService.jsm#911-915.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1301469 - Handle exceptions thrown by `PushCrypto.decodeMsg` as decryption errors.

Previously, errors thrown by `decodeMsg` and `getCryptoParams` would
bubble up to the catch handler in `receivedPushMessage`, causing us to
report "ACK_NOT_DELIVERED" instead of "ACK_DECRYPTION_ERROR" in the ack
sent to the server.

Review commit: https://reviewboard.mozilla.org/r/83670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83670/

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1301469 - Add localized decryption errors for invalid headers and padding.

Web Crypto returns an unhelpful "operation failed for an
operation-specific reason" error if the actual decryption fails, but
we can report more useful errors for missing and invalid header
values.

Review commit: https://reviewboard.mozilla.org/r/83672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83672/

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

https://reviewboard.mozilla.org/r/83672/#review82338

::: dom/locales/en-US/chrome/dom/dom.properties:272
(Diff revision 1)
> +PushMessageBadEncodingHeader=The ServiceWorker for scope ‘%1$S’ failed to decrypt a push message. The ‘Content-Encoding‘ header must be ‘aesgcm‘. ‘aesgcm128‘ is allowed, but deprecated and will soon be removed.
> +# LOCALIZATION NOTE: This error is reported when a push message fails to decrypt
> +# because the "dh" parameter is not valid base64url. Do not translate
> +# "ServiceWorker", "dh", "Crypto-Key", and "base64url". %1$S is the
> +# ServiceWorker scope URL.
> +PushMessageBadSenderKey=The ServiceWorker for scope ‘%1$S’ failed to decrypt a push message. The ‘dh‘ parameter in the ‘Crypto-Key‘ header must be the app server's Diffie-Hellman public key, base64url-encoded (‘+‘ and ‘/‘ replaced with ‘-‘ and ‘_‘; no trailing ‘=‘) and in "uncompressed" or "raw" form (65 bytes before encoding).

For base64url, you could just point to RFC 7515 instead.

::: dom/push/PushCrypto.jsm:42
(Diff revision 1)
>  var DEFAULT_KEYID = '';
>  
> +/** Localized error property names. */
> +
> +// `Encryption` header missing or malformed.
> +var BAD_ENCRYPTION_HEADER = 'PushMessageBadEncryptionHeader';

maybe we can use const for these

::: dom/push/PushCrypto.jsm:386
(Diff revision 1)
>      }
>      return decoded.slice(pad + padSize);
>    },
>  };
> +
> +class CryptoError extends Error {

I'd put this up top, but that's a style thing.

::: dom/push/PushService.jsm:920
(Diff revision 1)
> +          let cryptoError = error.isCryptoError ? error :
> +                            new CryptoError("Failed to decrypt message",
> +                                            "PushMessageBadCryptoError");

If this is your backstop, maybe it's better to put this in PushCrypto.jsm.  Then you don't need to export CryptoError; you just guarantee that any error thrown by those functions IS a CryptoError.

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8798127 [details]
Bug 1301469 - Handle exceptions thrown by `PushCrypto.decodeMsg` as decryption errors.

https://reviewboard.mozilla.org/r/83670/#review82340

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

https://reviewboard.mozilla.org/r/83672/#review82338

> For base64url, you could just point to RFC 7515 instead.

Done.

> maybe we can use const for these

Done.

> If this is your backstop, maybe it's better to put this in PushCrypto.jsm.  Then you don't need to export CryptoError; you just guarantee that any error thrown by those functions IS a CryptoError.

Good call! I can move `getCryptoParams` here, too, and then I don't need to export either.

Comment 6

[Lina Butler (ex-Mozilla)]

Comment on attachment 8798127 [details]
Bug 1301469 - Handle exceptions thrown by `PushCrypto.decodeMsg` as decryption errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83670/diff/1-2/

Comment 7

[Lina Butler (ex-Mozilla)]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83672/diff/1-2/

Comment 8

[Lina Butler (ex-Mozilla)]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83672/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74d4c026cca8
Handle exceptions thrown by `PushCrypto.decodeMsg` as decryption errors. r=mt
https://hg.mozilla.org/integration/autoland/rev/82ceed71dac7
Add localized decryption errors for invalid headers and padding. r=mt

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74d4c026cca8
https://hg.mozilla.org/mozilla-central/rev/82ceed71dac7

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

https://reviewboard.mozilla.org/r/83672/#review82862

::: dom/locales/en-US/chrome/dom/dom.properties:289
(Diff revision 3)
> +# LOCALIZATION NOTE: This error is reported when a push message fails to decrypt
> +# because an encrypted record is shorter than the pad size, the pad is larger
> +# than the record, or any of the padding bytes are non-zero. Do not translate
> +# "ServiceWorker". %1$S is the ServiceWorker scope URL. %2$S is the pad size
> +# (1 for aesgcm128, 2 for aesgcm).
> +PushMessageBadPaddingError=The ServiceWorker for scope ‘%1$S’ failed to decrypt a push message. Each record in the encrypted message must be padded with a %2$S byte big-endian unsigned integer, followed by that number of zero-valued bytes.

byte -> bytes?

Can someone give a practical example or reference for this? It's really hard to localize without technical background. It also makes me wonder if "that number" should be "the same number".

Comment 12

[Lina Butler (ex-Mozilla)]

Comment on attachment 8798128 [details]
Bug 1301469 - Add localized decryption errors for invalid headers and padding.

https://reviewboard.mozilla.org/r/83672/#review82862

> byte -> bytes?
> 
> Can someone give a practical example or reference for this? It's really hard to localize without technical background. It also makes me wonder if "that number" should be "the same number".

"byte" is correct; the sentence will read "2 byte big-endian unsigned integer" (or "1 byte big-endian unsigned integer" for the deprecated encoding, which makes "big-endian" redundant). I'm worried "the same number" could be confusing: if the pad is 4, there should be 4 zeros after it (00 04 00 00 00 00), not 4 fours.

You have a good point, though; this is a very technical detail. We could make it more generic and say something like "A record in the encrypted message was not padded correctly. For help with padding, please see the explanation in https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-02#section-2". Would that be better?

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> "byte" is correct; the sentence will read "2 byte big-endian unsigned
> integer"

It really sounds strange, I guess it's my non-English brain having a hard time putting all the pieces together. 2 bytes would be the size of the unsigned integer, wouldn't it?

> We could make it more generic and say something like "A record in the encrypted
> message was not padded correctly. For help with padding, please see the
> explanation in
> https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-
> 02#section-2". Would that be better?

Yes, I think it would be easier for localizers, and probably delivers more info even for developers.

Comment 14

[Lina Butler (ex-Mozilla)]

(In reply to Francesco Lodolo [:flod] from comment #13)
> (In reply to Kit Cambridge [:kitcambridge] from comment #12)
> > "byte" is correct; the sentence will read "2 byte big-endian unsigned
> > integer"
> 
> It really sounds strange, I guess it's my non-English brain having a hard
> time putting all the pieces together. 2 bytes would be the size of the
> unsigned integer, wouldn't it?

Yeah, 2 bytes is the size. I don't know the grammatical explanation for why it works here...maybe the fact that I'm using it as an adjective has something to do with it? Or maybe I'm totally wrong. :-) "2 bytes big-endian" sounds "off," but I don't know enough to say why.

> > We could make it more generic and say something like "A record in the encrypted
> > message was not padded correctly. For help with padding, please see the
> > explanation in
> > https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-
> > 02#section-2". Would that be better?
> 
> Yes, I think it would be easier for localizers, and probably delivers more
> info even for developers.

Will do!