1748361 - PR_GetAddrInfoByName: glibc getaddrinfo uses getenv without being protected by NSPR environment lock

Status: NEW

(NSPR :: NSPR, defect, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See https://github.com/rust-lang/rust/issues/27970 and https://sourceware.org/bugzilla/show_bug.cgi?id=13271. Because getaddrinfo may access the environment without using PR_GetEnv, it will race with any call to PR_SetEnv, possibly resulting in memory corruption in the thread calling PR_GetAddrInfoByName.

Marking this as a security bug but probably it should be open since this issue affecting Firefox has already been discussed extensively in the public Rust issue linked above. (Sorry!)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In general there's no safe way to use PR_SetEnv in a multi-threaded application like Firefox. In Firefox, all the uses of PR_SetEnv should be removed, and static analysis should be added to the build system to prevent new uses, if at all practical.

Comment 2

[:Gijs (he/him)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)

In general there's no safe way to use PR_SetEnv in a multi-threaded application like Firefox. In Firefox, all the uses of PR_SetEnv should be removed, and static analysis should be added to the build system to prevent new uses, if at all practical.

Uh, so from a cursory read of the rust issue, this would include removing uses of setenv generally, right? That's going to be "fun" because we expose that functionality to JS through XPCOM ( https://searchfox.org/mozilla-central/rev/fbf1e796ecead9484deced4d99f199f327ba25ab/xpcom/threads/nsIEnvironment.idl#15-21 ) and use it for e.g. restarting ourselves and modifying the behaviour of the resulting process (for troubleshoot mode, firefox reset/refresh, etc.).

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

e.g. restarting ourselves and modifying the behaviour of the resulting process (for troubleshoot mode, firefox reset/refresh, etc.).

posix_spawn and the like provide a way to pass a modified environment to the spawned child, and those should be used instead. That's mentioned in the Rust issue; see the references to Command::{env,envs}.

Comment 4

[Daniel Veditz [:dveditz]]

Seems like this issue is public in other places so unhiding.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:KaiE, could you have a look please?

For more information, please visit auto_nag documentation.