1338701 - PublicKeyPinningService should never have a non-const handle on any preloaded pinning information

Status: RESOLVED FIXED

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

Description

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

This line casts a const TransportSecurityPreload to a non-const one: https://dxr.mozilla.org/mozilla-central/rev/511093a0d82882d4b20f91b0287ad4b610c5225f/security/manager/ssl/PublicKeyPinningService.cpp#197

We're just lucky all of its fields are (currently - see bug 1335294) const and that we didn't try to change any of them.

There should be no non-const TransportSecurityPreloads around anywhere.

Comment 1

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

Attachment: bug 1338701 - constify all TransportSecurityPreloads, use mozilla::BinarySearch over bsearch

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

Comment 2

[:Cykesiopka]

Comment on attachment 8836272 [details]
bug 1338701 - constify all TransportSecurityPreloads, use mozilla::BinarySearch over bsearch

https://reviewboard.mozilla.org/r/111750/#review113116

More const and less casting is always nice to see.

::: security/manager/ssl/PublicKeyPinningService.cpp:144
(Diff revision 1)
> -*/
> -static int
> -TransportSecurityPreloadCompare(const void* key, const void* entry) {
> -  auto keyStr = static_cast<const char*>(key);
> -  auto preloadEntry = static_cast<const TransportSecurityPreload*>(entry);
> -
> +public:
> +  explicit TransportSecurityPreloadBinarySearchComparator(
> +    const char* aTargetHost)
> +    : mTargetHost(aTargetHost) {}
> +
> +  int operator()(const TransportSecurityPreload& val) const {

Nit: I believe our style guide says the opening brace should go on the next time.

::: security/manager/ssl/PublicKeyPinningService.cpp:204
(Diff revision 1)
>        return NS_OK;
>      }
>  
> -    foundEntry = (TransportSecurityPreload *)bsearch(evalHost,
> -      kPublicKeyPinningPreloadList,
> -      sizeof(kPublicKeyPinningPreloadList) / sizeof(TransportSecurityPreload),
> +    size_t foundEntryIndex;
> +    if (BinarySearchIf(kPublicKeyPinningPreloadList, 0,
> +                       ArrayLength(kPublicKeyPinningPreloadList),

This should have a corresponding `#include "mozilla/ArrayUtils.h"`.

Comment 3

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

Comment on attachment 8836272 [details]
bug 1338701 - constify all TransportSecurityPreloads, use mozilla::BinarySearch over bsearch

https://reviewboard.mozilla.org/r/111750/#review113116

Thanks!

> Nit: I believe our style guide says the opening brace should go on the next time.

Ah, right.

> This should have a corresponding `#include "mozilla/ArrayUtils.h"`.

Good catch.

Comment 4

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

Comment on attachment 8836272 [details]
bug 1338701 - constify all TransportSecurityPreloads, use mozilla::BinarySearch over bsearch

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c09eb845f9be
constify all TransportSecurityPreloads, use mozilla::BinarySearch over bsearch r=Cykesiopka

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/c09eb845f9be