Closed Bug 1237232 Opened 6 years ago Closed 6 years ago

Fix Vector MOZ_WARN_UNUSED_RESULT warnings in security/

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

I'm working on this in bug 1237201. The attached patch fixes 3 warnings in security/

Brian, I've no idea if any of these OOMs are really security-sensitive. If they are not, please open this up.
Attached patch PatchSplinter Review
Attachment #8704574 - Flags: review?(brian)
Attachment #8704574 - Flags: review?(brian) → review?(dkeeler)
Duplicate of this bug: 1237231
Comment on attachment 8704574 [details] [diff] [review]
Patch

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

Thanks for working on this. I think we'll have to do something a bit more involved for MakeMostRecentlyUsed - see the comment below (and let me know if it isn't clear).

::: security/certverifier/OCSPCache.cpp
@@ +148,5 @@
>    Entry* entry = mEntries[aIndex];
>    // Since mEntries is sorted with the most-recently-used entry at the end,
>    // aIndex is likely to be near the end, so this is likely to be fast.
>    mEntries.erase(mEntries.begin() + aIndex);
> +  MOZ_ALWAYS_TRUE(mEntries.append(entry));

If this fails in a non-debug build, I think we'll leak the Entry (and lose it to future lookups). We should probably make this failure more fatal and bubble it up to the caller.

Here's what I'm thinking:

* have MakeMostRecentlyUsed return a Result
** Success if everything went fine
** Result::FATAL_ERROR_NO_MEMORY if appending failed (and delete entry)

* change the cases of:

MakeMostRecentlyUsed(...);
return Success;

to

return MakeMostRecentlyUsed(...);

* change OCSPCache::Get to return a Result
** basically, the Result from MakeMostRecentlyUsed
** the bool it had been returning can be an out parameter like aResult and aValidThrough
** NSSCertDBTrustDomain::CheckRevocation will need to be updated to handle this - if ::Get != Success, return the failing Result
Attachment #8704574 - Flags: review?(dkeeler) → review-
Thanks for the fast review.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> >    mEntries.erase(mEntries.begin() + aIndex);
> > +  MOZ_ALWAYS_TRUE(mEntries.append(entry));
> 
> If this fails in a non-debug build, I think we'll leak the Entry (and lose
> it to future lookups). We should probably make this failure more fatal and
> bubble it up to the caller.

Hm I was thinking the erase() will always leave space for another element, so the append will always succeed. Hence the MOZ_ALWAYS_TRUE. I can also make that a MOZ_RELEASE_ASSERT with a comment?
Flags: needinfo?(dkeeler)
Sure - that sounds reasonable. I think you're probably right about there being space for another element, but I didn't want to rely on that (there might be a scenario where the erase causes the vector to shrink the memory it's using and then another thread uses up all available memory before the append can happen, but I think it would be extremely rare).
Flags: needinfo?(dkeeler)
Comment on attachment 8704574 [details] [diff] [review]
Patch

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

r=me with using a MOZ_RELEASE_ASSERT and a comment in MakeMostRecentlyUsed.
Attachment #8704574 - Flags: review- → review+
Thanks! Yeah, erase() currently just moves/destroys elements without doing any realloc/shrinking, and if that ever changes the release assert should catch it.

Last question: what kind of security rating would you give this? In particular the other append calls, in nsNSSCertListFakeTransport::Read and OCSPCache::Put.

I don't think we usually treat these things as security sensitive, but this is security-related code and I'm not familiar with it at all.
I don't think this is security-sensitive (doesn't look like I can remove the bit, though).
Group: core-security → dom-core-security
Group: dom-core-security
https://hg.mozilla.org/mozilla-central/rev/f1a9021e3893
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.