956147 - GetSubjectAltNames in TransportSecurityInfo.cpp should use a scoped PLArenaPool for san_arena

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) [:keeler]]

In theory, san_arena can leak in GetSubjectAltNames. In reality, it probably doesn't, but this is a good opportunity to start moving towards more scoped types in PSM where it's a clear win.

This requires:
1. Defining a scoped PLArenaPool type (see https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ScopedNSSTypes.h#125 for the sort of thing required)
2. Using it and getting rid of the PORT_FreeArena at the end of GetSubjectAltNames

Comment 1

[Mike Hoye [:mhoye]]

As much as I want to believe, bugs without well-understood resolutions in core-security aren't good first bugs.

Comment 2

[hpathak]

Attachment: Bug956147_scoped_PLArena.diff

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8441122 [details] [diff] [review]
Bug956147_scoped_PLArena.diff

Review of attachment 8441122 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I have some nits. I'll have another look when you've addressed those.
Also, the commit message should say something about how this change is specific to GetSubjectAltNames.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +599,5 @@
>  {
>    allNames.Truncate();
>    nameCount = 0;
>  
> +  mozilla::pkix::ScopedPLArenaPool san_arena;

nit: just call this arena
Also, move this down to where the arena is created, and do something like this:

mozilla::pkix::ScopedPLArenaPool san_arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));

(you might have to add a line break so it doesn't go over 80 characters)

@@ +609,5 @@
>    if (rv != SECSuccess)
>      return false;
>  
>    san_arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>    if (!san_arena)

Since the name is going to change, you'll touch this line - might as well add braces around the if body.

Comment 4

[hpathak]

Attachment: Bug956147_scoped_PLArena.diff

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8441844 [details] [diff] [review]
Bug956147_scoped_PLArena.diff

Review of attachment 8441844 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me with nits addressed (see comments).
I think the only tests that would exercise this code are in mochitest-bc, so this would be an appropriate try string: "try: -b do -p linux64 -u mochitest-bc -t none"

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +604,5 @@
>    CERTGeneralName *sanNameList = nullptr;
>  
>    SECStatus rv = CERT_FindCertExtension(nssCert, SEC_OID_X509_SUBJECT_ALT_NAME,
>                                          &altNameExtension);
> +  if (rv != SECSuccess){

nit: have a space between the final ) and {

@@ +611,3 @@
>  
> +  mozilla::pkix::ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
> +  if (!arena){

here too

@@ +616,3 @@
>  
> +  sanNameList = CERT_DecodeAltNameExtension(arena.get(), &altNameExtension);
> +  if (!sanNameList){

here too

Comment 6

[hpathak]

Attachment: Bug956147_scoped_PLArena.diff

https://tbpl.mozilla.org/?tree=Try&rev=2ac679cb58f5

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4e9ad0824e

Comment 8

[Ed Morley [:emorley]]

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