1338897 - Avoid using NSS Base64 functions in PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[:Cykesiopka]

The NSS Base64 functions are less safe and convenient to use than the XPCOM ones.
They're also an unnecessary dependency on NSS.

Comment 1

[:Cykesiopka]

Attachment: Bug 1338897 - Avoid using NSS Base64 functions in PSM.

The NSS Base64 functions are less safe and convenient to use than the XPCOM ones.
They're also an unnecessary dependency on NSS.

The NSS Base64 functions behave slightly differently than the XPCOM ones:
1. ATOB_ConvertAsciiToItem() / NSSBase64_DecodeBuffer() silently ignore invalid
   characters like CRLF, space and so on. Base64Decode() will return an error
   if these characters are encountered.
2. BTOA_DataToAscii() will produce output that has CRLF inserted every 64
   characters. Base64Encode() doesn't do this.

For the reasons listed below, no unexpected compatibility issues should arise:
1. AppSignatureVerification.cpp already filters out CRLF and spaces for Manifest
   and Signature values before decoding.
2. ExtendedValidation.cpp is only given what should be valid hard-coded input to
   decode.
3. ContentSignatureVerifier.cpp already splits on CRLF for when it needs to
   decode PEM certs. Spaces shouldn't be likely.
   For Content-Signature header verification, examination of real input to a
   running instance of Firefox suggests CRLF and spaces will not be present in
   the header to decode.
4. nsCryptoHash.cpp encode is affected, but we actually don't want the CRLF
   behaviour.
5. nsDataSignatureVerifier.cpp decode is affected, but we add whitespace
   stripping to maintain backwards compatibility.
6. nsKeygenHandler.cpp encode is affected, but the previous CRLF behaviour was
   arguably a bug, since neither WHATWG or W3C specs specified this.

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

Comment 2

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8847155 [details]
Bug 1338897 - Avoid using NSS Base64 functions in PSM.

https://reviewboard.mozilla.org/r/120172/#review123060

Looks great. Sorry this took a while to review.

::: security/apps/AppSignatureVerification.cpp:50
(Diff revision 1)
>  extern mozilla::LazyLogModule gPIPNSSLog;
>  
>  namespace {
>  
> +inline nsDependentCSubstring
> +DigestToString(const Digest& digest)

It might be nice to call this "DigestToDependentString" to be a bit more clear that the result depends on the input living longer than it (also might be nice to document this).

::: security/nss.symbols:19
(Diff revision 1)
>  #endif
>  #include ../db/sqlite3/src/sqlite.symbols
>  ATOB_AsciiToData
>  ATOB_AsciiToData_Util
> -ATOB_ConvertAsciiToItem
>  ATOB_ConvertAsciiToItem_Util

I'd imagine we can get rid of this line as well now, right?

::: security/nss.symbols:510
(Diff revision 1)
>  SECITEM_AllocArray
>  SECITEM_AllocItem
>  SECITEM_AllocItem_Util
>  SECITEM_ArenaDupItem_Util
> -SECITEM_CompareItem
>  SECITEM_CompareItem_Util

Same here.

Comment 3

[:Cykesiopka]

Comment on attachment 8847155 [details]
Bug 1338897 - Avoid using NSS Base64 functions in PSM.

https://reviewboard.mozilla.org/r/120172/#review123060

No worries, thanks for the review!

> I'd imagine we can get rid of this line as well now, right?

Sadly, getting rid of the _Util symbols broke my try builds the last time I tried. Looks like we still need to keep this here as long as NSS itself uses the function.

Comment 4

[:Cykesiopka]

Comment on attachment 8847155 [details]
Bug 1338897 - Avoid using NSS Base64 functions in PSM.

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

Comment 5

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3addf5adadbd72dec2ee8a72fb556754684cc19

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/312bdabd35b8
Avoid using NSS Base64 functions in PSM. r=keeler

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/312bdabd35b8

Comment 8

[:Cykesiopka]

The changes here may impact add-ons that fit the following criteria:
1. Use nsICryptoHash or nsICryptoHMAC directly or indirectly.
2. Init either of the above with SHA-512 as the choice of hash algorithm.
3. Call `finish(true)` to get back base 64 output.

If these add-ons previously depended on the base 64 output having CRLF characters inserted every 64 characters, they need to stop doing that - the output will no longer have such characters.