1117617 - Uninitialized return value in nss_dbm_db_set_label()

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nicholas Nethercote [inactive]]

cppcheck says:

> security/nss/lib/ckfw/dbm/db.c:141: error: Uninitialized variable: rv

As far as I can tell it's correct -- in the first |return| statement |rv| is undefined.

I think it should probably instead be more like nss_dbm_db_delete_object() and return the return value from the failed NSSCKFWMutex_Lock() call.

And someone who understands this code should audit all the other uses of NSSCKFWMutex_Lock() in this file...

- Some of them look ok -- e.g. on lock failure they return NULL or CK_FALSE, which might be ok.

- But nss_dbm_db_new_handle() looks like it doesn't set |*pError| in all the cases it should.

- And nss_dbm_db_get_object_attribute_count() returns 0 instead of |*pError| if locking fails -- and 0 is CKR_OK! Likewise with nss_dbm_db_get_object_attribute_size().

Comment 1

[Al Billings [:abillings - ex-MoCo]]

David, can you take a look at this?

Comment 2

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

Attachment: patch

The other cases looked ok (some of them don't return a CK_RV, and so aren't supposed to be error codes).
Wan-Teh, would you mind having a look? Thanks.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

David, what is the security severity of this bug?

Comment 4

[Kai Engert [:KaiE:]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> - And nss_dbm_db_get_object_attribute_count() returns 0 instead of |*pError|
> if locking fails -- and 0 is CKR_OK! Likewise with
> nss_dbm_db_get_object_attribute_size().

Because they don't return status error type CK_RV, they return a numeric result.
Returning 0 seems to be the right result code for the count/size functions.


The other places that call NSSCKFWMutex_Lock look correct to me, too.

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 8548379 [details] [diff] [review]
patch

r=kaie

Comment 6

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

(In reply to David Bolter [:davidb] from comment #3)
> David, what is the security severity of this bug?

As far as I can tell, NSSCKFWMutex_Lock never returns anything other than CKR_OK, so this isn't even a security problem with the current code. Still good to fix, though, in case the underlying implementation changes.

Comment 7

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

Kai, would you mind checking this patch in when appropriate? Thanks.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> As far as I can tell, NSSCKFWMutex_Lock never returns anything other than
> CKR_OK, so this isn't even a security problem with the current code. Still
> good to fix, though, in case the underlying implementation changes.

Does that mean we can open up this bug?


(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> Kai, would you mind checking this patch in when appropriate? Thanks.

Sure, I'll do. Once I check it in, it's probably no longer of any value to keep this bug security/hidden anyway.

Comment 9

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

Yeah, this isn't security-sensitive. Thanks!

Comment 10

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/7264a233c0cf

Comment 11

[Kai Engert [:KaiE:]]

mass change target milestone to 3.17.4

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for 37 as well since that is impacted.  Kai - do we need a bug for bumping NSS version in FF37 for this and bug 1117621?

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

keeler - Can you answer Lukas' question in comment 12?

Comment 14

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

Neither this bug nor bug 1117621 impact the security of Firefox (see comment 6 and bug 1117621 comment 5 for why), so it's not necessary to update NSS in FF37 for these bugs. However, there may be other bugs for which we would need to update. Kai would know the answer to that. I'll specifically ask in this week's NSS meeting if he doesn't respond here before then.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Clearing 37 tracking and status based on comment 14. If there are others bugs that require an NSS update, please nom those bugs for tracking.

Comment 16

[Kai Engert [:KaiE:]]

This bug is fixed in NSS 3.17.4, and Firefox 37 already uses NSS 3.17.4

Comment 17

[Sylvestre Ledru [:Sylvestre]]

I guess this is the same for 38.