1836559 - PBKDF2 does not accept CK_PKCS5_PBKD2_PARAMS2

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[joachim.vandersmissen]

pkcs11t.h states:

/* CK_PKCS5_PBKD2_PARAMS is new for v2.10.
 * CK_PKCS5_PBKD2_PARAMS is a structure that provides the
 * parameters to the CKM_PKCS5_PBKD2 mechanism. */
/* this structure is kept for compatibility. use _PARAMS2. */

However, actually trying to use CK_PKCS5_PBKD2_PARAMS2 as intended results in a Segmentation fault. This is because of https://github.com/nss-dev/nss/blob/master/lib/softoken/pkcs11c.c#L4021-L4028:

    if (pMechanism->mechanism == CKM_PKCS5_PBKD2) {
        if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PKCS5_PBKD2_PARAMS))) {
            return CKR_MECHANISM_PARAM_INVALID;
        }
        pbkd2_params = (CK_PKCS5_PBKD2_PARAMS *)pMechanism->pParameter;
        pwitem.data = (unsigned char *)pbkd2_params->pPassword;
        /* was this a typo in the PKCS #11 spec? */
        pwitem.len = *pbkd2_params->ulPasswordLen;

As you can see, no distinction is made between CK_PKCS5_PBKD2_PARAMS and CK_PKCS5_PBKD2_PARAMS2. Then the last line will happily dereference ulPasswordLen, even though this is not a pointer in PARAMS2.

Comment 1

[BugBot [:suhaib / :marco]]

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

For more information, please visit BugBot documentation.

Comment 2

[joachim.vandersmissen]

Some more info: CK_ULONG and CK_ULONG_PTR are identical lengths on 64-bit platforms:

(gdb) p sizeof(CK_PKCS5_PBKD2_PARAMS)
$1 = 72
(gdb) p sizeof(CK_PKCS5_PBKD2_PARAMS2)
$2 = 72
(gdb) ptype CK_PKCS5_PBKD2_PARAMS
type = struct CK_PKCS5_PBKD2_PARAMS {
    CK_PKCS5_PBKDF2_SALT_SOURCE_TYPE saltSource;
    CK_VOID_PTR pSaltSourceData;
    CK_ULONG ulSaltSourceDataLen;
    CK_ULONG iterations;
    CK_PKCS5_PBKD2_PSEUDO_RANDOM_FUNCTION_TYPE prf;
    CK_VOID_PTR pPrfData;
    CK_ULONG ulPrfDataLen;
    CK_UTF8CHAR_PTR pPassword;
    CK_ULONG_PTR ulPasswordLen;
}
(gdb) ptype CK_PKCS5_PBKD2_PARAMS2
type = struct CK_PKCS5_PBKD2_PARAMS2 {
    CK_PKCS5_PBKDF2_SALT_SOURCE_TYPE saltSource;
    CK_VOID_PTR pSaltSourceData;
    CK_ULONG ulSaltSourceDataLen;
    CK_ULONG iterations;
    CK_PKCS5_PBKD2_PSEUDO_RANDOM_FUNCTION_TYPE prf;
    CK_VOID_PTR pPrfData;
    CK_ULONG ulPrfDataLen;
    CK_UTF8CHAR_PTR pPassword;
    CK_ULONG ulPasswordLen;
}
(gdb) p sizeof(CK_ULONG_PTR)
$3 = 8
(gdb) p sizeof(CK_ULONG)
$4 = 8

This also causes the sizes of _PARAMS and _PARAMS2 structs to be identical, which means that BAD_PARAM_CAST can't distinguish between the two.

Comment 3

[Benjamin Beurdouche [:beurdouche]]

@john, bob. I have marked this as a sec bug because nobody noticed it mentioned causing a sec-fault.
Any opinion on priority/severity ?

Comment 4

[Daniel Veditz [:dveditz]]

/* this structure is kept for compatibility. use _PARAMS2. */

Despite that comment, no code inside NSS itself uses _PARAMS2, only the original version.

Comment 5

[John Schanck [:jschanck]]

As Dan mentioned, NSS and softoken use CK_PKCS5_PBKD2_PARAMS consistently. However this is a footgun for other applications that use softoken alone (assuming they exist..).

Softoken exposes multiple PKCS#11 interfaces. Some are PKCS#11 version 3.0 and others are version 2.40. Version 3.0 no longer includes the _PARAMS variant, so that case is easy (softoken shouldn't use _PARAMS). Unfortunately, early versions of 2.40 did include _PARAMS. It was only removed in an errata in 2016. So there's some small risk of confusion there.

Personally, I think we should completely remove _PARAMS and switch all existing uses to _PARAMS2. I'll post a patch.

Comment 6

[John Schanck [:jschanck]]

Attachment: Bug 1836559 - remove CK_PKCS5_PBKD2_PARAMS. r=rrelyea

Comment 7

[Robert Relyea]

Sigh we need to think about this a bit. There's a binary compatibility isssue here with both softoken and with NSS. The problem is we have the old version out it the wild and the there's the move to the new version. If we can't distinguish on the size of the structure, we need to do something else. On the softoken size we can probably verify whether the length looks like a pointer or whether it looks like an actual length so softoken can accept both. I'd bet that Java has followed softoken's bug here when it tries to do PKCS 5v2 PBEs through softoken.

On the NSS side, it's less easy. How do we know which way the token is coded? NOTE: this call can call into another PKCS #11 module. This is used whenever we do PKCS #12, so only token that supports exporting private keys will have to deal with whatever NSS does here.

It's possible that they already do what I suggest we do for softoken, in which case the NSS side should be fine.

Comment 8

[John Schanck [:jschanck]]

How do we know which way the token is coded?

If it's a v3.0 token then CK_PKCS5_PBKD2_PARAMS is definitely wrong.

I followed up on what Java does. They use CK_PKCS5_PBKD2_PARAMS for v2.40 tokens and for NSS softoken regardless of version.
https://github.com/openjdk/shenandoah/blob/c584b481e9cd14258f1297cb835815c6a9023ca3/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L406-L416

Comment 9

[John Schanck [:jschanck]]

For what it's worth, that java code was only added 4 months ago. Maybe it's not too late to fix this...

Comment 10

[Robert Relyea]

So the first case, Fix Softoken to be bi-lingual.

If size of CK_ULONG and size of CK_ULONG_PTR are different, we are golden, we can use the size of the PARAMS. to determine who's who. (we do that for other cases of multiple possible parameters passed in). Unfortunately that's not the case for the majority of our current platforms.

If they sizes are identical, then we'll have to hueristically determine if what's what.
Softoken can look at the sizes if the sizes are identical, it would look at the length and see if it's reasonable. The only issue if if user buffers can be in the first several K bytes of the address space. There are two options:

1. check that the length looks reasonable (isn't too big).
   Cons:
   1. This works if the low pages in virtual memory aren't accessible to the application. In most of our systems this is true, at least of address '0', or our NULL pointer checks wouldn't work, but it's not guaranteed since we are talking about a virtual address space for our application. Looking at various maps, it looks like this is part of the user's address space, but it's usually the text space. I've seen some address spaces, like arm, that puts thread local memory here, though (sigh).
   2. Whatever value we choose will limit the maximum size of the password. If the limit is too bit, then we have a bigger chance of violating case 1.
2. check if pointer is a validly 'mapped'.
   Cons:
   1) This is highly system dependent. There's a lot of stuff out there explaining why you shouldn't do this. Windows has it's own challenges (https://devblogs.microsoft.com/oldnewthing/20060927-07/?p=29563).

I think option 1 is probably the best. What should the length be. 1K, 2K, 16K (I don't think it should be more thand 16K. A 1K password isn't a human password, but the PBE can be used with non-human passwords...).

The second case, is what we send. I like John's idea of checking the PKCS #11 version. I'd probably set it to something > than the current spec as people may have already released modules with the current spec (PKCS #11 3.1). There can be an argument for using 3.0, but I'm not sure we'll break someone who has already specifically tested with us earliear (unless they've done the softoken trick to accept both values).

bob

Comment 11

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

Comment 12

[fkrenzel]

FYI: I am looking into this (can't assign myself as a assignee). Will go with Bobs solution (I doubt that I will find any other).

Comment 13

[Robert Relyea]

I've added you as the assignee. I'd probably go with 16K as the limit.

Comment 14

[Robert Relyea]

OK John beat me to it, so I won't clobber his changes:).

Comment 15

[fkrenzel]

What about taking into account some likeability that it is a actual password length like it being the power of 2 +- 5 digits e.g. 2048, 2045, 1024... I think it is very likely that this would be the actual password length and that it is unlikely that we have hit such a memory address ?

Comment 16

[fkrenzel]

Attachment: (secure)

Comment 17

[Robert Relyea]

The algorithm works because we don't expect pointers to point to very low memory. The lower the limit the better the algorithm works on platforms that don't block off the top page of memory or have something other than kernel text (or data) there. If it weren't for some platforms (like arm IIRC), we are probably safe up to a page size, so 2048 should be fine from that perspective. The only case where it could be longer if someone used a machine generated password (no human is going to type in 2048 digits by hand), so that should be ok (until Alicja creates a test case with the password being the complete text of Moby Dick:). We do have cases where the passwords would be hundreds of bytes (we ran into issues where NSS and openssl disagreed on how many null bytes to include in the password, which is only noticed when the password length was greater than the hash length (because when it was smaller it was zero padded anyway), so 60 some odd bytes.

Upshot between 1k and 32k seem to be the optimal range, so 2k is a fine decision. Make a a #define in a header so we can change it if we run into problems.

Comment 18

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/0612f81c50970ad63fd19d5e5d242adb667c78ee

There's already a 3.108 branch, I don't know if it will be in the 3.108 release or the 3.109 release.

Comment 19

[Kai Engert [:KaiE:]]

The tests run into crashes in pk12util. I think we should backout until the issue can be investigated and fixed. Search for "core dumped" in this log.
https://firefoxci.taskcluster-artifacts.net/ZX_hQ3pwTaas6-K_PSbIyg/0/public/logs/live_backing.log
The cryptofuzz task shows a stack of a crash:
https://treeherder.mozilla.org/logviewer?job_id=492201413&repo=nss&lineNumber=15218

I'll backout.

Comment 20

[Kai Engert [:KaiE:]]

backout:
https://hg.mozilla.org/projects/nss/rev/6372fb942ed79b31f8fee824f159fe96f5724e7e

Comment 21

[Robert Relyea]

Frentesek, can you take a look at this make sure any new patch runs through CI before you submit it again.
Thanks (looks like something is up with our test of whether or not the length value is a length and not a pointer?).

Comment 22

[fkrenzel]

Already looking into this.

Comment 23

[fkrenzel]

https://firefoxci.taskcluster-artifacts.net/eAks8gcVRxaL4zH_SwsB3Q/0/public/logs/live_backing.log
I suppose we can't handle this(In reply to Robert Relyea from comment #17)

The algorithm works because we don't expect pointers to point to very low memory. The lower the limit the better the algorithm works on platforms that don't block off the top page of memory or have something other than kernel text (or data) there. If it weren't for some platforms (like arm IIRC), we are probably safe up to a page size, so 2048 should be fine from that perspective. The only case where it could be longer if someone used a machine generated password (no human is going to type in 2048 digits by hand), so that should be ok (until Alicja creates a test case with the password being the complete text of Moby Dick:). We do have cases where the passwords would be hundreds of bytes (we ran into issues where NSS and openssl disagreed on how many null bytes to include in the password, which is only noticed when the password length was greater than the hash length (because when it was smaller it was zero padded anyway), so 60 some odd bytes.

Upshot between 1k and 32k seem to be the optimal range, so 2k is a fine decision. Make a a #define in a header so we can change it if we run into problems.

And we hit the fuzzer...
I have no what to do here.

Comment 24

[Robert Relyea]

We should probably add a check at the NSS level for passwords > 2048 and reject them. That way at no one will hit this issue from an NSS API.

If someone makes a softoken call, then we will crash...

pk11_RawPBEKeyGenWithKeyType is probably the right place. PK11_KeyGenWithTemplate is a public function, but it generates arbitrary key types, so If someone is bypassing the PBE code, the they need to follow softoken rules.

Comment 25

[Robert Relyea]

If the fuzzer already has a different limit that bigger than 2048, but still reasonably small (< 16k) we can use that limit as well.

Comment 26

[fkrenzel]

I will add the password length check

If the fuzzer already has a different limit that bigger than 2048, but still reasonably small (< 16k) we can use that limit as well.
It has hit on some 4200 so maybe 8k? I think even generate password above this is a overkill.

Comment 27

[John Schanck [:jschanck]]

In the past we have used the UNSAFE_FUZZER_MODE compile-time macro to get around code that would otherwise block fuzzing. You could do that here. I'd also be interested in a compile-time option to always use CK_PKCS5_PBKD2_PARAMS2.

Comment 28

[Robert Relyea]

In this case, I think the FUZZER caught something. If someone manages got get some data the causes our PBE code to use >2k password we would crash. Adding the check in the PBE code isolates us from that case.

As far as compile time flags, we would probably need 2 separate flags... one to always assume CK_PKCS5_PBKD2_PARAMS2 in softoken, the second to always send CK_PCKS5_PBKD2_PARAMS in NSS. I think the former is the one that would safer to turn on in the future (once Java knows it can use CK_PKCS5_PBKD2_PARAMS2). The latter would probably stay awhile until all PKCS #11 v2 PKCS #11 modules are out of service, which could be decades. Fortunately the latter is the more problematic check.

Comment 29

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/e874633283473b50565a5586377214f4a565f267
3.111

Comment 30

[John Schanck [:jschanck]]

We're going to have to back this out because of test failures on Windows (See Bug 1957519). I haven't been able to track down the issue, but given that the failures are windows-only I assume it's in the code that handles structs of different size.

For what it's worth, there were gtest failures in this nss-try push: https://treeherder.mozilla.org/jobs?repo=nss-try&selectedTaskRun=FKdCdp8rSHSJaE3Y7ZNlDQ.0&revision=6fb5de50cd0a11dcb3c06880b44286124460bb9c&searchStr=Windows%2CGtest. Unfortunately it seems that our windows builders stopped working around the time when this landed, so no one noticed.

Comment 31

[Benjamin Beurdouche [:beurdouche]]

Attachment: Bug 1836559 - Backed out changeset e87463328347 for breaking Firefox Windows opt builds. r=djackson

Comment 32

[BugBot [:suhaib / :marco]]

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)

Comment 33

[fkrenzel]

Looking into this.

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #32)

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)

What is the best approach here a new bug, a clone or a ..regressed by this issue ?

Do you by any chance have means to provide a debugging environment for this like a remote host or a qemu image, in case that I will be unable to track the issue in the code logic?

Comment 34

[Robert Relyea]

fkrenzel. This bug is already reopenned. You can just start a new phabricator patch.

Comment 35

[John Schanck [:jschanck]]

(In reply to fkrenzel from comment #33)

Do you by any chance have means to provide a debugging environment for this like a remote host or a qemu image, in case that I will be unable to track the issue in the code logic?

You should be able to run the gtests on windows in nss-try if you rebase on tip. The gtests reliably reproduce the issue on windows, where the two structs are different sizes.

Comment 36

[fkrenzel]

I have updated the original revision with the fix, should i create a new one or just keep it in the old one?

The problem with the windows(i.e. the structures being of different size) was that I haven't specified the params.len to sizeof(CK_PKCS5_PBKD2_PARAMS2) when this was the case.
By default it was max size of the two, so it tripped when CK_PKCS5_PBKD2_PARAMS2 was smaller then CK_PKCS5_PBKD2_PARAMS.

Please re-review and land if everything is ok.

Comment 37

[fkrenzel]

Ok I have just noticed that there are memory leaks, I will check if we can get rid of them.

Comment 38

[fkrenzel]

After many hours of investigating a memory leak I have taken yet another look at the log and figured it that the errors are caused by fuzzer using password that is over 8192 thus tripping over trying to de-reference the length.... I can't find a sensible place to limit the fuzzer input size. Do you have any suggestions?

Comment 39

[John Schanck [:jschanck]]

We should just configure fuzzer builds to use the _USE_PKCS5_PBKD2_PARAMS2_ONLY options.

Comment 40

[fkrenzel]

So I have fixed the issue with the fuzzer but it seems there is still a issue with pkcs12 memory leak https://treeherder.mozilla.org/jobs?repo=nss-try&selectedTaskRun=CkNKDCJiSzamJySKfIQ85A.0

What baffles me is that it also happens with Bob's latest revisions for example: https://treeherder.mozilla.org/jobs?repo=nss-try&selectedTaskRun=Et_fLCR4TTKjolMM0euoSw.0 the same exact issue, is it possible that modification to lib/softoken/pkcs11c.c triggers a memcheck mechanism and discovers old issues that weren't discovered?

Comment 41

[John Schanck [:jschanck]]

We're tracking the pkcs12 leak in Bug 1972054. You can land your patch for this bug.

Comment 42

[John Schanck [:jschanck]]

https://hg-edge.mozilla.org/projects/nss/rev/1f935d2e4e0039aefdf41dfcc44caf6d42b32b8e