Closed Bug 1301469 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(2 files)

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 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.
Attachment #8798128 - Flags: review?(martin.thomson) → review+
Comment on attachment 8798127 [details]
Bug 1301469 - Handle exceptions thrown by `PushCrypto.decodeMsg` as decryption errors.

https://reviewboard.mozilla.org/r/83670/#review82340
Attachment #8798127 - Flags: review?(martin.thomson) → review+
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.
Pushed by kcambridge@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/74d4c026cca8
https://hg.mozilla.org/mozilla-central/rev/82ceed71dac7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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".
Flags: needinfo?(kcambridge)
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?
Flags: needinfo?(kcambridge)
(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.
Blocks: 1308590
(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!
You need to log in before you can comment on or make changes to this bug.