PR_GetNameForIdentity threadsafety and bound check fixes

RESOLVED FIXED in 4.13

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

other
4.13

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
See Also: → 698882
Attachment #8731339 - Flags: review?(ted)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
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) {
...
}
Attachment #8731339 - Flags: review?(ted)
Attachment #8731339 - Attachment is obsolete: true
Attachment #8733429 - Flags: review?(ted) → review+
thanks!

how do I get this pushed to https://hg.mozilla.org/projects/nspr/ ?
Flags: needinfo?(ted)
I'll land it for you when I have a minute. Feel free to land this to inbound as well if you'd like.
Whiteboard: [leave open]
I've marked it leave open until ted gets a chance to check it into upstream. thanks!
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(ted)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → 4.13
You need to log in before you can comment on or make changes to this bug.