Closed Bug 1275197 Opened 4 years ago Closed 4 years ago

nsNSSU2FToken.cpp GetSymKeyByNickname() can theoretically leak

Categories

(Core :: Security: PSM, defect)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

()

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

(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.
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.
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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Summary: nsNSSU2FToken.cpp GetSymKeyByNickname() can leak → nsNSSU2FToken.cpp GetSymKeyByNickname() can theoretically leak
Whiteboard: [psm-backlog] → [psm-assigned]
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/
Attachment #8757564 - Flags: review?(dkeeler)
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
Attachment #8757564 - Flags: review?(dkeeler) → review+
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 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/
Attachment #8757564 - Attachment description: MozReview Request: Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. → MozReview Request: Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d048e177ab26
Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler
Keywords: checkin-needed
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/d048e177ab26
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.