1296219 - Use the Mozilla Base64 functions instead of the NSPR ones in PSM

Status: RESOLVED FIXED

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

Description

[:Cykesiopka]

NSPR should generally be avoided in favour of modern C++ code.

Comment 1

[:Cykesiopka]

Attachment: Bug 1296219 - Use the Mozilla Base64 functions instead of the NSPR ones in PSM.

NSPR should generally be avoided in favour of modern C++ code.

This patch does not convert uses of the NSS Base64 functions. It does however
take the opportunity to switch over some IDL functions to use the safer Mozilla
string classes.

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

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8783256 [details]
Bug 1296219 - Use the Mozilla Base64 functions instead of the NSPR ones in PSM.

https://reviewboard.mozilla.org/r/73128/#review71064

LGTM with comments addressed

::: security/manager/ssl/nsIX509CertDB.idl:96
(Diff revision 1)
>     *  the certificate.
>     *
>     *  @param aDBkey Database internal key, as obtained using
>     *                attribute dbkey in nsIX509Cert.
>     */
>    nsIX509Cert findCertByDBKey(in string aDBkey);

Would this be a good time to update this API as well or should this be a follow-up?

::: security/manager/ssl/nsIX509CertDB.idl:126
(Diff revision 1)
>     *  @param aEmailAddress The email address to be used as the key
>     *                       to find the certificate.
>     *
>     *  @return The matching certificate if found.
>     */
>    nsIX509Cert findCertByEmailAddress(in string aEmailAddress);

This one too?

::: security/manager/ssl/nsIX509CertDB.idl:266
(Diff revision 1)
>     *  @param certDER The raw representation of a certificate,
>     *                 encoded as raw DER.
>     *  @param length  The length of the DER string.
>     *  @return The new certificate object.
>     */
>    nsIX509Cert constructX509(in string certDER, in unsigned long length);

Here too? (also, we can probably just get rid of the length argument)

::: security/manager/ssl/nsIX509CertDB.idl:380
(Diff revision 1)
>     */
>    int32_t /*PRErrorCode*/
>      verifyCertAtTime(in nsIX509Cert aCert,
>                       in int64_t /*SECCertificateUsage*/ aUsage,
>                       in uint32_t aFlags,
>                       in string aHostname,

For these APIs, it looks like we use a null hostname to indicate we want to skip hostname checking, so I guess we should leave this as-is.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1176
(Diff revision 1)
>  {
>    nsNSSShutDownPreventionLock locker;
>    if (isAlreadyShutDown()) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>    if (NS_WARN_IF(!_retval)) {

Let's remove this NS_WARN_IF while we're here (i.e. change it to just |if (!_retval) {|)

::: security/manager/ssl/nsNSSCertificateDB.cpp:1349
(Diff revision 1)
>      return NS_ERROR_FAILURE;
>    }
>  
>    nsCOMPtr<nsIX509Cert> newCert;
>    nsresult rv = ConstructX509FromBase64(aBase64, getter_AddRefs(newCert));
>    NS_ENSURE_SUCCESS(rv, rv);

If you feel like it, changing this to

if (NS_FAILED(rv)) {
  return rv;
}

would also fix bug 1251050 and probably mean less overall work, but it's not a big deal.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1381
(Diff revision 1)
>  NS_IMETHODIMP
> -nsNSSCertificateDB::AddCert(const nsACString & aCertDER, const char *aTrust,
> -                            const char *aName)
> +nsNSSCertificateDB::AddCert(const nsACString& aCertDER, const nsACString& aTrust,
> +                            const nsACString& aName)
>  {
>    nsCString base64;
>    nsresult rv = Base64Encode(aCertDER, base64);

This is definitely something for a follow-up, but it doesn't make a whole lot of sense to encode to base64 just to decode it again to make the certificate.

::: security/manager/ssl/nsNTLMAuthModule.cpp:219
(Diff revision 1)
>   * Print base64-encoded token to the NSPR Log.
>   * @param name Description of the token, will be printed in front
>   * @param token The token to print
>   * @param tokenLen length of the data in token
>   */
> -static void LogToken(const char *name, const void *token, uint32_t tokenLen)
> +template<size_t N>

I'm not sure templatizing this is worth it, since we're essentially just passing |name| on as a null-terminated c-string.

::: security/manager/ssl/nsNTLMAuthModule.cpp:233
(Diff revision 1)
> -  if (b64data)
> -  {
> -    PR_LogPrint("%s: %s\n", name, b64data);
> -    PR_Free(b64data);
> +  nsresult rv = mozilla::Base64Encode(static_cast<const char*>(token), tokenLen,
> +                                      &b64DataTemp);
> +  if (NS_FAILED(rv) || !b64DataTemp) {
> +    return;
>    }
> +  mozilla::UniqueFreePtr<char> b64Data(b64DataTemp);

I feel like we can avoid using the temporary pointer if we make a dependent string out of the token (we're already casting away the void*-ness) and use a different Base64Encode definition. Do the types not work out in that case?

::: security/manager/ssl/nsNTLMAuthModule.cpp:236
(Diff revision 1)
> -    PR_Free(b64data);
> +    return;
>    }
> +  mozilla::UniqueFreePtr<char> b64Data(b64DataTemp);
> +  b64DataTemp = nullptr;
> +
> +  PR_LogPrint("%s: %s\n", name, b64Data.get());

Hmmm - I thought this was going to be an easy fix, but it looks like a slightly larger issue. In any case, nsNTLMAuthModule.cpp is using both MOZ_LOG (via a macro called LOG) and PR_LogPrint. I think it would be best if it just used MOZ_LOG (perhaps with different log levels so the verbosity could be configured if necessary).

Comment 3

[:Cykesiopka]

Comment on attachment 8783256 [details]
Bug 1296219 - Use the Mozilla Base64 functions instead of the NSPR ones in PSM.

https://reviewboard.mozilla.org/r/73128/#review71064

Thanks!

> Would this be a good time to update this API as well or should this be a follow-up?

I've actually started to go through all the PSM IDL files to address use of (w)string.
Turns out there are a lot of places that still use (w)string, so I'll file bugs and post patches as I work through all the IDLs.

> This one too?

Same as above.

> Here too? (also, we can probably just get rid of the length argument)

I was tempted to do this earlier, but there are callers outside of PSM, and I didn't want to touch those yet.

Speaking of outside callers, this is how I plan to convert the various IDLs:
1. Change methods and attributes that don't require C++ or JS code changes outside of PSM, without introducing behaviour changes.
2. Change methods and attributes that require code changes outside PSM, without introducing behaviour changes.
3. Maybe change methods and attributes that would cause behaviour changes.

> For these APIs, it looks like we use a null hostname to indicate we want to skip hostname checking, so I guess we should leave this as-is.

Noted, thanks.

> If you feel like it, changing this to
> 
> if (NS_FAILED(rv)) {
>   return rv;
> }
> 
> would also fix bug 1251050 and probably mean less overall work, but it's not a big deal.

Sounds good, will do.

> This is definitely something for a follow-up, but it doesn't make a whole lot of sense to encode to base64 just to decode it again to make the certificate.

I filed Bug 1297357 for this.

> I'm not sure templatizing this is worth it, since we're essentially just passing |name| on as a null-terminated c-string.

Yeah, I guess not - I changed it back to ```const char*```.

> I feel like we can avoid using the temporary pointer if we make a dependent string out of the token (we're already casting away the void*-ness) and use a different Base64Encode definition. Do the types not work out in that case?

Yes, the types do work out - much nicer.

> Hmmm - I thought this was going to be an easy fix, but it looks like a slightly larger issue. In any case, nsNTLMAuthModule.cpp is using both MOZ_LOG (via a macro called LOG) and PR_LogPrint. I think it would be best if it just used MOZ_LOG (perhaps with different log levels so the verbosity could be configured if necessary).

Another thing to fix in Bug 1273747 I guess.

Comment 4

[:Cykesiopka]

Comment on attachment 8783256 [details]
Bug 1296219 - Use the Mozilla Base64 functions instead of the NSPR ones in PSM.

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

Comment 5

[:Cykesiopka]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78c914db015d
Use the Mozilla Base64 functions instead of the NSPR ones in PSM. r=keeler

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78c914db015d