1275197 - nsNSSU2FToken.cpp GetSymKeyByNickname() can theoretically leak

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

(David Keeler [:keeler] (use needinfo?) from Bug 1271496 comment #3)
> ::: security/manager/ssl/nsNSSU2FToken.cpp:114
> (Diff revision 1)
> >      if (aNickname == freeKeyName.get()) {
> >        MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key found!"));
> > -      return freeKey.forget();
> > +      return freeKey;
> >      }
> >  
> >      keyList = PK11_GetNextSymKey(keyList);
> 
> Unfortunately, it looks like we have a preexisting leak here - if it happens
> that we don't return the last element in the list, all remaining keys in the
> list will be leaked. This can be addressed in a follow-up, however.

Indeed, it looks like PK11_ListFixedKeysInSlot() will return the head of the list as a PK11SymKey*, and it is the responsibility of the caller to follow the |next| pointer via PK11_GetNextSymKey() until all the keys are freed.

Comment 1

[:Cykesiopka]

I'll probably get this fixed in a few days time (and after Bug 1271496 lands), but if anyone would like to fix this before then, feel free.

Comment 2

[:Cykesiopka]

I chatted with :jcj about this code briefly over IRC, and he noted that in theory only one key should be found in practice, which matches what I'm seeing during tests. He also mentioned that the nickname check can probably just be an assertion instead.

So, this bug is more about fixing a theoretical issue.

Comment 3

[:Cykesiopka]

Attachment: MozReview Request: Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler

Prior to these changes, GetSymKeyByNickname() could theoretically leak. This
should not happen in practice, so the changes here just ensure that the code
doesn't cause leaks.

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

Comment 4

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

Comment on attachment 8757564 [details]
MozReview Request: Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler

https://reviewboard.mozilla.org/r/55988/#review53356

Great - r=me.

::: security/manager/ssl/nsNSSU2FToken.cpp:88
(Diff revision 1)
>  {
>    mWrappingKey = nullptr;
>  }
>  
> +/**
> + * Gets the first key with the given nickname from the given slot. Any other

nit: let's use //-style comments here

Comment 5

[:Cykesiopka]

https://reviewboard.mozilla.org/r/55988/#review53356

Thanks!

> nit: let's use //-style comments here

Hmm, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices says we should use JavaDoc-style comments, which is what this comment is formatted as. I don't really mind using //-style comments though.

Comment 6

[:Cykesiopka]

Comment on attachment 8757564 [details]
MozReview Request: Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler

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

Comment 7

[:Cykesiopka]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d048e177ab26
Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler

Comment 9

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

(In reply to :Cykesiopka from comment #5)
> https://reviewboard.mozilla.org/r/55988/#review53356
> 
> Thanks!
> 
> > nit: let's use //-style comments here
> 
> Hmm,
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#General_C.2FC.2B.2B_Practices says we should use JavaDoc-style
> comments, which is what this comment is formatted as. I don't really mind
> using //-style comments though.

Ah, I suppose you're right. My thought process was: a) match existing style and b) /**/-style comments can make it harder to temporarily comment out large blocks of code for debugging purposes, but there are other ways to do that and that kind of situation is probably rare enough that it doesn't matter.

Comment 10

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #9)
> Ah, I suppose you're right. My thought process was: a) match existing style
> and b) /**/-style comments can make it harder to temporarily comment out
> large blocks of code for debugging purposes, but there are other ways to do
> that and that kind of situation is probably rare enough that it doesn't
> matter.

True - I forgot to mention that my preference for /**-style JavaDoc comments is just for functions, where e.g. my IDE makes it easy to generate the correct template and make changes.

Comment 11

[Carsten Book [:Tomcat]]

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