1271501 - Avoid using reinterpret_cast in PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

Of the casts allowed to be used in Mozilla code (static_cast, const_cast, and reinterpret_cast), reinterpret_cast is the most dangerous because it allows completely unrelated types to be cast to each other. While this sort of cast is necessary in various places, it should be avoided if possible.

In PSM, uses of reinterpret_cast fall into these categories:
 1) Completely pointless
 2) Can be removed by refactoring
 3) Can be replaced by a less strong cast
 4) Necessary

For cases that fall under category 4, we can replace reinterpret_cast with mozilla::BitwiseCast instead. BitwiseCast does the same thing, but provides static asserts that mitigate some of the risk of using reinterpret_cast.

Note: Old C-style casts are not really in scope for this bug, since those are less easy to find, and the changes required for just reinterpret_cast are already numerous enough. We can convert the old style casts as we notice them.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1271501 - Remove unnecessary uses of reinterpret_cast in PSM. r=keeler

These uses of reinterpret_cast are either pointless, or can be removed via
refactoring.

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

Comment 2

[:Cykesiopka]

Attachment: MozReview Request: Bug 1271501 - Downgrade unnecessarily strong reinterpret_casts in PSM. r=keeler

These reinterpret_casts can be static_casts or const_casts instead.

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

Comment 3

[:Cykesiopka]

Attachment: MozReview Request: Bug 1271501 - Use mozilla::BitwiseCast instead of reinterpret_cast in PSM. r=keeler

mozilla::BitwiseCast does the same thing, but provides static asserts that
mitigate some of the risk of using reinterpret_cast.

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

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/52858/#review49736

Just as a note, I purposefully used the two template argument version of BitwiseCast because I prefer maximum safety (casting something that doesn't match the second argument will cause a compilation failure), and because I think it's useful to be able to visually inspect that a cast makes sense.

However, I'm not particularly tied to doing this, so I would be OK with using the single argument version as well, e.g. to reduce verbosity.

::: security/manager/ssl/nsNSSCertificateDB.cpp:188
(Diff revision 1)
> -  uint32_t serialNumberLen = ntohl(*reinterpret_cast<const uint32_t*>(reader));
> +  // Note: We surround the ntohl() argument with parentheses to stop the macro
> +  //       from thinking two arguments were passed.

I actually forgot to send the updated version of this patch to try again, so this might not even work (I don't know why, but my local Linux toolchain works fine even without the parentheses, but the machines in automation aren't happy).

I'll make sure to find the appropriate solution before checking this in of course.

Comment 5

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

Comment on attachment 8752912 [details]
MozReview Request: Bug 1271501 - Remove unnecessary uses of reinterpret_cast in PSM. r=keeler

https://reviewboard.mozilla.org/r/52854/#review50062

LGTM.

::: security/manager/ssl/nsNSSCallbacks.h:98
(Diff revision 1)
>    mozilla::ThreadSafeAutoRefCnt mRefCount;
>  
>  public:
>    typedef mozilla::pkix::Result Result;
>  
>    static Result createFcn(SEC_HTTP_SERVER_SESSION session,

Might as well update this to be nsNSSHttpServerSession\* while we're here.

::: security/manager/ssl/nsNSSCallbacks.h:155
(Diff revision 1)
> -                                 SEC_HTTP_SERVER_SESSION* pSession)
> +                         /*out*/ nsNSSHttpServerSession** pSession)
>    {
>      return nsNSSHttpServerSession::createSessionFcn(host, portnum, pSession);
>    }
>  
>    static Result createFcn(SEC_HTTP_SERVER_SESSION session,

Here too.

Comment 6

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

Comment on attachment 8752913 [details]
MozReview Request: Bug 1271501 - Downgrade unnecessarily strong reinterpret_casts in PSM. r=keeler

https://reviewboard.mozilla.org/r/52856/#review50098

::: security/apps/AppSignatureVerification.cpp:636
(Diff revision 1)
>    // TODO: null pinArg is tolerated.
>    if (NS_WARN_IF(!signerCert) || NS_WARN_IF(!voidContext)) {
>      return NS_ERROR_INVALID_ARG;
>    }
>    const VerifyCertificateContext& context =
> -    *reinterpret_cast<const VerifyCertificateContext*>(voidContext);
> +    *static_cast<const VerifyCertificateContext*>(voidContext);

I think the underlying reason for this cast is a poorly defined API (VerifyCMSDetachedSignatureIncludingCertificate). Maybe in another bug we could do some cleanup work to rework this so it can actually be typechecked by the compiler.

::: security/manager/ssl/nsNTLMAuthModule.cpp:575
(Diff revision 1)
>    {
>  #ifdef IS_BIG_ENDIAN
>      ucsDomainBuf = domain;
>      domainPtr = ucsDomainBuf.get();
>      domainLen = ucsDomainBuf.Length() * 2;
> -    WriteUnicodeLE((void *) domainPtr, reinterpret_cast<const char16_t*> (domainPtr),
> +    WriteUnicodeLE(const_cast<void*>(domainPtr),

It looks like domainPtr, userPtr, and hostPtr (as well as userUpperPtr and domainUpperPtr) shouldn't have been declared const (they also shouldn't all be declared on the same line, probably shouldn't be void*s, and WriteUnicodeLE can probably be replaced with mozilla::NativeEndian::swapToLittleEndianInPlace (or similar if I've misunderstood what's going on here), but maybe that's for another cleanup bug).

Comment 7

[:Cykesiopka]

https://reviewboard.mozilla.org/r/52856/#review50098

> I think the underlying reason for this cast is a poorly defined API (VerifyCMSDetachedSignatureIncludingCertificate). Maybe in another bug we could do some cleanup work to rework this so it can actually be typechecked by the compiler.

I filed Bug 1273740 for this.

> It looks like domainPtr, userPtr, and hostPtr (as well as userUpperPtr and domainUpperPtr) shouldn't have been declared const (they also shouldn't all be declared on the same line, probably shouldn't be void*s, and WriteUnicodeLE can probably be replaced with mozilla::NativeEndian::swapToLittleEndianInPlace (or similar if I've misunderstood what's going on here), but maybe that's for another cleanup bug).

I filed Bug 1273747 for this. I took a look at making changes to get rid of the casts, but it looks like the changes required would be better off being made in another bug - nsNTLMAuthModule has some other problems as well and AFAICT zero automated tests.

Comment 8

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

Comment on attachment 8752914 [details]
MozReview Request: Bug 1271501 - Use mozilla::BitwiseCast instead of reinterpret_cast in PSM. r=keeler

https://reviewboard.mozilla.org/r/52858/#review50412

Good stuff.

::: security/manager/ssl/ScopedNSSTypes.h:48
(Diff revision 1)
>  // It is very common to cast between char* and uint8_t* when doing crypto stuff.
>  // Here, we provide more type-safe wrappers around reinterpret_cast so you don't
>  // shoot yourself in the foot by reinterpret_casting completely unrelated types.
>  
> -inline char *
> -char_ptr_cast(uint8_t * p) { return reinterpret_cast<char *>(p); }
> +inline char*
> +char_ptr_cast(uint8_t* p) { return BitwiseCast<char*>(p); }

These aren't used all that often, it seems. Can we just remove/replace them? (This can be a follow-up.)

::: security/manager/ssl/nsDataSignatureVerifier.cpp:297
(Diff revision 1)
>      return rv;
>    }
>  
>    SECItem buffer = {
>      siBuffer,
> -    reinterpret_cast<uint8_t*>(const_cast<char*>(aRSABuf)),
> +    BitwiseCast<uint8_t*, const char*>(aRSABuf),

Isn't SECItem's data member an unsigned char\*? (I guess that's what uint8_t\* expands to, but maybe we should use the type as written in the definition) (Oh, I guess this was a preexisting issue. Well, anyway.)

::: security/manager/ssl/nsNSSCallbacks.cpp:871
(Diff revision 1)
>    // Get the NPN value.
>    SSLNextProtoState state;
>    unsigned char npnbuf[256];
>    unsigned int npnlen;
>  
> +  // XXX: Use ArrayLength(npnbuf) instead of a literal |256|.

Maybe TODO(a specific bug number)? Or just address it now?

::: security/manager/ssl/nsNSSShutDown.cpp:119
(Diff revision 1)
>    for (auto iter = singleton->mPK11LogoutCancelObjects.Iter();
>         !iter.Done();
>         iter.Next()) {
>      auto entry = static_cast<ObjectHashEntry*>(iter.Get());
> -    nsOnPK11LogoutCancelObject *pklco =
> -      reinterpret_cast<nsOnPK11LogoutCancelObject*>(entry->obj);
> +    nsOnPK11LogoutCancelObject* pklco =
> +      BitwiseCast<nsOnPK11LogoutCancelObject*, nsNSSShutDownObject*>(entry->obj);

This actually seems like a bad situation we should fix (in a follow-up). I'm sure we can use a higher-level hash table that's parameterized or something.

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.h:68
(Diff revision 1)
>                const Host *hosts)
>  {
>    for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
>      for (const Host *host = hosts; host->mHostName; ++host) {
>        SECItem hostName;
> -      hostName.data = reinterpret_cast<uint8_t*>(const_cast<char*>(host->mHostName));
> +      hostName.data = BitwiseCast<uint8_t*, const char*>(host->mHostName);

Again, looks like a preexisting use of uint8_t for a SECItem.data.

Comment 9

[:Cykesiopka]

https://reviewboard.mozilla.org/r/52858/#review50412

Thanks for the review!

> These aren't used all that often, it seems. Can we just remove/replace them? (This can be a follow-up.)

Yes - I filed Bug 1274135 for this.

> Isn't SECItem's data member an unsigned char\*? (I guess that's what uint8_t\* expands to, but maybe we should use the type as written in the definition) (Oh, I guess this was a preexisting issue. Well, anyway.)

Yes, you're correct. Not really sure why I didn't correct it in the first place. I'll make the change.

> Maybe TODO(a specific bug number)? Or just address it now?

I'll just address it now.

> This actually seems like a bad situation we should fix (in a follow-up). I'm sure we can use a higher-level hash table that's parameterized or something.

Yeah, it looks at least one of the hash table classes under xpcom/glue/ will work. I filed Bug 1274138 for this.

> Again, looks like a preexisting use of uint8_t for a SECItem.data.

Will fix.

Comment 10

[:Cykesiopka]

Comment on attachment 8752912 [details]
MozReview Request: Bug 1271501 - Remove unnecessary uses of reinterpret_cast in PSM. r=keeler

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

Comment 11

[:Cykesiopka]

Comment on attachment 8752913 [details]
MozReview Request: Bug 1271501 - Downgrade unnecessarily strong reinterpret_casts in PSM. r=keeler

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

Comment 12

[:Cykesiopka]

Comment on attachment 8752914 [details]
MozReview Request: Bug 1271501 - Use mozilla::BitwiseCast instead of reinterpret_cast in PSM. r=keeler

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

Comment 13

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=617e688f1b114ef783e9e81d8ac5e0ba44a35d17
(Please ignore the Android X3 failures - I included a WIP patch for Bug 1273749 as part of the push, and the test that was included in the patch failed. Every other test passed, so this patch set should be fine on Android.)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7503f7ee2446
https://hg.mozilla.org/integration/mozilla-inbound/rev/4525aaa132ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef294cb3b47

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7503f7ee2446
https://hg.mozilla.org/mozilla-central/rev/4525aaa132ff
https://hg.mozilla.org/mozilla-central/rev/1ef294cb3b47