1926256 - AddressSanitizer: heap-use-after-free [@ SECMOD_DestroyModule] with READ of size 8 at shutdown

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 133.0a1-20241015135709-https://hg.mozilla.org/mozilla-central/rev/efb1596265e9da9bbccbcbf3f4f796564780ccec.

For detailed crash information, see attachment.

Looks like a use-after-free on shutdown.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

I'm marking this sec-moderate because a main process shutdown UAF doesn't seem easy to exploit.

Comment 3

[John Schanck [:jschanck]]

I took a quick look at calls to nssToken_Destroy and I found a double free. I'm attaching a patch, but we're really close to the end of the cycle for this NSS release, so I think we should wait until the next cycle to land it. In the mean time, we should audit other calls to nssToken_Destroy.

The get_token_objects_for_cache function constructs an array called objects. Each object in objects owns a reference to a particular token. The objects are converted, one by one, into cache entries using a function called create_object. Inside create_object, the object's token reference is dropped with a call to nssToken_Destroy(object->token).

A double free can occur if the i'th call to create_object fails. In that case, get_token_objects_for_cache will

1. restore object j's token reference for all 0 ≤ j < i,
2. call nssCryptokiObjectArray_Destroy(objects), which calls nssToken_Destroy(object->token) for each object in objects.

This is problematic for several reasons, but in particular it assumes that create_object only calls nssToken_Destroy(object->token) on success. However, there are two error paths in create_object that occur after the call to nssToken_Destroy. Hitting either one of these error paths will cause the token's reference count to be decremented through an invalid reference in nssCryptokiObjectArray_Destroy.

Since it is safe to call nssToken_Destroy(object->token) when object->token == NULL. We should just null out the reference immediately after calling nssToken_Destroy(object->token) in create_object. We can then remove the reference juggling in get_token_objects_for_cache completely.
`

Comment 4

[John Schanck [:jschanck]]

Attachment: Bug 1926256 - simplify error handling in get_token_objects_for_cache. r=rrelyea

Comment 5

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/f1ef72bb14a484ac823fafddaad349b10b4f5875

Comment 6

[John Schanck [:jschanck]]

The patch from Comment 5 was backed out of NSS 3.107 for causing Bug 1932518.

https://hg.mozilla.org/projects/nss/rev/d16826fc40d0cf4601f84030f7e4617e3eabd23d

Comment 7

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/9505f79d5be0137aa344f92219e89e3f7a81819e
3.108

Comment 8

[Anna Weine]

Attachment: Bug 1926256 - fix build error from 9505f79d

Comment 9

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)