1760813 - PKCS12 enable cipher function cannot succeed

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Jesse P. Johnson]

Steps to reproduce:

Performed maintenance on python-nss and traced back function call stack by calling each child method individually. Then decided to do code review on nss function being called and determined there was no way the method could return successfully.

I believe this effects all versions from 3.44 onward.

Actual results:

Determined when SEC_PKCS12EnableCipher is called it will iterate an array of structs for each PKCS12 cipher. Once a match is found NSS_SetAlgorithmPolicy is called to enable the cipher and return SECSuccess or SECFailure. But the conditional logic within SEC_PKCS12EnableCipher will only pass if (SECSuccess != SECSuccess).

https://github.com/nss-dev/nss/blob/master/lib/pkcs12/p12plcy.c#L96
https://github.com/nss-dev/nss/blob/master/lib/util/secoid.c#L2272

Expected results:

The return types returned SEC_PKCS12EnableCipher should be evaluated to true when SECSuccess is returned instead of always assigning SEC_ERROR_INVALID_ALGORITHM and failing.

Comment 1

[Marcin Cieślak (:saper)]

Attachment: Bug 1760813 - Make SEC_PKCS12EnableCipher succeed

Comment 2

[Marcin Cieślak (:saper)]

With the patch above, I can do the following now with python-nss

>>> from nss import nss
>>> nss.nss_init_read_write("pki")
>>> nss.pkcs12_enable_cipher(0x20012)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: pkcs12_enable_cipher() takes exactly 2 arguments (1 given)
>>> nss.pkcs12_enable_cipher(0x20012, 1)
>>> nss.pkcs12_enable_cipher(0x20012, 0)
>>> nss.pkcs12_enable_all_ciphers()
>>> nss.pkcs12_enable_cipher(0x20000, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'PKCS12 cipher name not found: 131072'

Without this patch, it always ends up like this:

>>> from nss import nss
>>> nss.nss_init_read_write("pki")
>>> nss.pkcs12_enable_all_ciphers()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
nss.error.NSPRError: Failed to enable PKCS12_AES_CBC_128 (131097) pkcs12 cipher: (SEC_ERROR_INVALID_ALGORITHM) security library: invalid algorithm.
>>> nss.pkcs12_enable_cipher(0x20012, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
nss.error.NSPRError: Failed to enable PKCS12_DES_EDE3_168 (131090) pkcs12 cipher: (SEC_ERROR_INVALID_ALGORITHM) security library: invalid algorithm.
>>> nss.pkcs12_enable_cipher(0x20012, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
nss.error.NSPRError: Failed to disable PKCS12_DES_EDE3_168 (131090) pkcs12 cipher: (SEC_ERROR_INVALID_ALGORITHM) security library: invalid algorithm.

Comment 3

[Marcin Cieślak (:saper)]

Attachment: Bug 1760813 - Make SEC_PKCS12EnableCipher succeed

Comment 4

[Marcin Cieślak (:saper)]

What happens here next? Who can change the status of a bug?

Comment 5

[Dennis Jackson]

Hi Marcin,

We currently release NSS on a monthly schedule which you can see here. One week prior to release we enter a change freeze to allow for testing in Firefox Nightly, Thunderbird and other downstream consumers. We're currently in the freeze period prior to the release of 3.77 on Thursday. After Thursday, one of the team will land the patch in the NSS repository, mark the bug as fixed and it will be included in the 3.78 release builds.

Thank you for your help! :-)

Best,
Dennis

Comment 6

[Jesse P. Johnson]

Thank you Dennis for the update!

Comment 7

[Robert Relyea]

I've reviewed the patch (the review is in phabricator). While I'm OK with the patch as is, I did suggest some changes which I would prefer. It would be good for Marcin to indicate if he'd rather go forward with this version or update the patch as requested.

Thanks,
bob

Comment 8

[Marcin Cieślak (:saper)]

Thank you bob, this my first contribution to NSS ever and I am more than happy to improve the patch.

Comment 9

[Robert Relyea]

Dennis I've approved the patch. it's ready to land.

Comment 10

[Dennis Jackson]

Thanks Bob and Marcin! John has landed this:

https://hg.mozilla.org/projects/nss/rev/7706dfb0906b1403053bf35612659ea65492d173

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 9269683 [details] [diff] [review]
Bug 1760813 - Make SEC_PKCS12EnableCipher succeed

Review of attachment 9269683 [details] [diff] [review]:
-----------------------------------------------------------------

John's patch is better :)

Comment 12

[Robert Relyea]

To be fair John's patch is just the latest patch from saper:).