1257250 - PR_GetNameForIdentity threadsafety and bound check fixes

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Patrick McManus [:mcmanus]]

PR_GetUniqueIdentity() takes care to be threadsafe in expanding the identity_cache table.. however that table is also accessed by PR_GetNameForIdentity() which does not do any locking.

The latter could easily de-reference identity_cache.name[ident] after that memory had been freed by PR_GetUniqueIdentity()

the "ident" argument is also a signed int and while it is bounds checked against the max size of the array it is not bounds checked for < 0.. and there are a number of 'special' layers that have negative identifies that could be easily be passed into this function and would result in a segv so its best to runtime bounds check it.

Firefox doesn't currently use PR_GetUniqueIdentity() directly, but from bug reports at least one LSP does do so and this creates a crash risk.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: pr_getnameforidentity bounds check and locking

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8731339 [details] [diff] [review]
pr_getnameforidentity bounds check and locking

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

::: nsprpub/pr/src/io/prlayer.c
@@ +692,4 @@
>  
> +    if (ident < 0) {
> +      goto done;
> +    }

Is there a reason you chose to use goto here instead of just wrapping these lines in an if block? Seems like this could easily be:
if (PR_TOP_IO_LAYER != ident && ident >= 0) {
...
}

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: pr_getnameforidentity bounds check and locking

Comment 4

[Patrick McManus [:mcmanus]]

thanks!

how do I get this pushed to https://hg.mozilla.org/projects/nspr/ ?

Comment 5

[(not currently active) Ted Mielczarek]

I'll land it for you when I have a minute. Feel free to land this to inbound as well if you'd like.

Comment 6

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56d3ab31480a27c4e6063f635f78625b8ce4d220
Bug 1257250 - pr_getnameforidentity bounds check and locking r=ted.mielczarek

Comment 7

[Patrick McManus [:mcmanus]]

I've marked it leave open until ted gets a chance to check it into upstream. thanks!

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56d3ab31480a

Comment 9

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nspr/rev/18fbd3af342b06747878452c4164b05428ba5a92
Bug 1257250 - pr_getnameforidentity bounds check and locking r=ted.mielczarek