1804640 - (CVE-2023-0767) Crash [@ sec_pkcs12_decoder_safe_bag_notify]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on nss revision a3669ed2c606+ (build with fuzzing and ASan).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Build NSS with fuzzing enabled: ./build.sh --asan --clang --fuzz (assuming mozbuild clang/clang++ is on PATH and matching NSPR with ASan is installed/used).
2. Run nssfuzz-pkcs12 test.bin

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

(In reply to Christian Holler (:decoder) from comment #0)

2. Run nssfuzz-pkcs12 test.bin

This requires the fuzzing target patch from bug 1804646.

Comment 4

[John Schanck [:jschanck]]

Attachment: WIP: Bug 1804640 - improve handling of unknown PKCS#12 safe bag types.

Comment 5

[John Schanck [:jschanck]]

I don't know this code well, and I don't know if the patch I've attached is correct. But I wanted to share it since it gets the fuzzer unstuck.

Comment 6

[Christian Holler (:decoder)]

This is a good example of something that I wouldn't call moderate because the required interaction is not obscure at all, especially not in a business context. Importing a PKCS#12 cert bundle might be a common operation in some use cases.

Comment 7

[Frederik Braun [:freddy]]

Hey John,
is there anyone who could help nudge this along either by taking the patch from you or reviewing it?

Comment 8

[John Schanck [:jschanck]]

My patch was not correct. Here's my rough understanding of what's wrong: The decoder is reading into an array of sec_PKCS12SafeBags. It fails to handle an error and leaves the output pointer in the middle of a sec_PKCS12SafeBag. It then proceeds to the next safe bag and writes the contents with the wrong alignment. In the example crash, the value 2 is written to sec_PKCS12SafeBag.bagTypeTag, which causes a null pointer check to fail. [EDIT: This was not accurate see Comment 15 below]

I'm not sure I can fix this in a reasonable amount of time. Bob, can you think of anyone that might be able to help with this? They'll need the fuzzing target from D164322.

Comment 9

[Robert Relyea]

Awgh, not that I can think of. There isn't a current expert in the der decoder code. Kai is most likely the one who has the most familiarity with it because he's handling Thunderbird, but I don't know his availability (and he's most likely familiar with the CMS side rather than the PKCS12 side.

How is your proposed patch failing? It looks pretty reasonable to me.

Comment 10

[Robert Relyea]

Hmm the error code processing should prevent this function from operating. There's an error code check at the beginning. I'm not sure how it can proceed to the next bag. The code that sets up current bag, Allocates The current bag from the arena, zeros it, puts it in the next slot on the array and then continues with processing. If there's a failure, nothing happens with the current bag until a new bug is added to the array. If we were walking the array, that could make sense (a partially initialized bag would be in the array), but there isn't a way for bagType to be set to anything other than a pointer. It seems that something is walking past it's bounds somewhere and stomping on the current bag tag value.

It might be worth setting a watch point on current bag tag adddress and seeing what's writing to it.

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Christian Holler (:decoder) from comment #6)

This is a good example of something that I wouldn't call moderate because the required interaction is not obscure at all, especially not in a business context.

It's borderline but CVSS seems to agree with you. I get a CVSS temporal score of 6.9—just under "high"—with a 7.3 base score that puts it at the low end of "high". Which isn't to say that I like CVSS any better than I ever did but it does force some black and white choices between a small number of choices.

Importing a PKCS#12 cert bundle might be a common operation in some use cases.

Maybe, but not importing bundles from any random person on the internet. I agree the potential impact accords with sec-high so we're just disagreeing over how likely a malicious person could socially engineer a victim into installing it, and how much the likely small number of potential victims moderates our rating. For instance, this could have broad impact on an enterprise if the attacker could replace a shared bundle everyone has to install. But then that lowers the CVSS score into definite moderate territory by changing the Privileges Required (PR) score from None (N) to probably High (H) (base score 6.1), but still does even if you use Low (L) (base score 6.8).

But sure, we can re-rate it.

Comment 12

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

One idea in terms of mitigating this in Firefox is to use PKI.js for decoding/importing PKCS#12 files and decoding CMS (for add-on verification). We already have the implementation in the tree and it probably wouldn't be too much work to hook up.

Comment 13

[John Schanck [:jschanck]]

Given the revised risk assessment, I'll also take another stab at fixing this.

Comment 14

[John Schanck [:jschanck]]

Attachment: Bug 1804640 - improve handling of unknown PKCS#12 safe bag types. r=#nss-reviewers

Comment 15

[John Schanck [:jschanck]]

The sec_pkcs12_choose_safe_bag_type function is used to select a template for decoding a PKCS#12 Safe Bag. When it is called, the parser's destination pointer is set to the address of the safeBagContent field of a sec_PKCS12SafeBag. This field is a union over pointer types. However, sec_pkcs12_choose_safe_bag_type function will return SEC_AnyTemplate when the safeBagType is an unknown OID. The SEC_AnyTemplate expects the parser's destination pointer to point to a SECItem (note: not a SECItem *). After deciding to parse the safe bag contents relative to SEC_AnyTemplate the parser 1) allocates memory at ((SECItem *)state->dest)->data, 2) copies arbitrary data from the input to that buffer, and 3) writes the length of the allocated buffer to ((SECItem *)state->dest)->len. Because of the type confusion, ((SECItem *)state->dest)->data is actually the attribs field of a sec_PKCS12SafeBag and ((SECItem *)state->dest)->len is actually the bagTypeTag field of the same struct.

This bug allows an attacker to write arbitrary data to the attribs field of a sec_PKCS12SafeBag and to set the bagTypeTag to an arbitrary value proportional to the length of the input. We should assume this is exploitable, but I think it would be hard to get a valid pointer into bagTypeTag, and having an invalid pointer there will cause the program to crash like in the provided example.

The changes I made to p12d.c are to fix a memory leak that I uncovered by fixing the primary bug here.

I reviewed other template selectors for similar type confusion bugs and did not find any. There are some similar problems in p12local.c but this appears to be dead code. I'll file another bug to remove the dead code.

Comment 16

[Pascal Chevrel:pascalc]

Should we have asked for sec-approval before landing?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

tip: https://hg.mozilla.org/projects/nss/rev/ad28ce3107eb137190ce83e10f346f696b17faed
3.88.1: https://hg.mozilla.org/projects/nss/rev/684586ec163ad4fbbf15ea2cd1ee5c2da43036ad
3.87.1: https://hg.mozilla.org/projects/nss/rev/62f6b3e9024dd72ba3af9ce23848d7573b934f18
3.79.4: https://hg.mozilla.org/projects/nss/rev/57c60a4da16503b7827ccd763354faf151514b57

Comment 18

[Ryan VanderMeulen [:RyanVM]]

John, can you please request sec-approval on this for posterity? Also, we're in a situation where we likely will need to respin the RC builds for 110 & 102.8esr - is this something we should include?

Comment 19

[John Schanck [:jschanck]]

Comment on attachment 9313349 [details]
Bug 1804640 - improve handling of unknown PKCS#12 safe bag types. r=#nss-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It will be easy to construct a crashing input---our example was found quickly through fuzzing, and the patch suggests a fuzzing target. But there are barriers to using the defect for reading arbitrary memory. I assume it's possible, but it would take some work.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. The modified code is not executed on valid inputs.
• Is Android affected?: Yes

Comment 20

[John Schanck [:jschanck]]

If we're respinning anyway, then we should include this, but this shouldn't drive a respin.

Comment 21

[Tom Ritter [:tjr]]

Comment on attachment 9313349 [details]
Bug 1804640 - improve handling of unknown PKCS#12 safe bag types. r=#nss-reviewers

This has already landed in NSS, so we can land it in m-whatever

Comment 22

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt