198090 - PK11SDR_Encrypt and PK11SDR_Decrypt hardcode 3DES - add support for AES

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

The encrypted "blob" of data produced by PK11SDR_Encrypt and input to
PK11SDR_Decrypt includes an Algorithm ID and a key ID.  Therefore, it should be
possible to decrypt blobs that were encrypted with algorithms other than 3DES,
such as AES.  However, PK11SDR_Decrypt ignores the algorithm ID and always uses
3DES.  The Algorithm ID is used only as a source of the IV for the block cipher.  

PK11SDR_Encrypt takes a Key ID as input, but not an algorithm identifier of any
kind.  It always uses 3DES.  

I propose that the algorithm limitation be removed.  I propose that
PK11SDR_Decrypt should actually determine the algorithm from the Algorithm 
ID in the blob.  This is backwards compatible and requires no API change.  

I also propose that a new encrypt function be created, similar to
PK11SDR_Encrypt but which takes a different or additional parameter that
enables keys for other algorithms to be found and/or used.  PK11SDR_Encrypt
could then be reimplemented as a call to this new function that uses only
3DES.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P3. No one is asking for this, AFAIK.

Comment 2

[Kai Engert [:KaiE:]]

Benjamin, is it time to get this done? Nelson's suggestion seems reasonable, and 3DES probably should no longer be used for new encrypted data in 2023?

Comment 3

[Anna Weine]

I think it would be great to replace with AES.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 198090 - Implement PK11SDR_EncryptAES.

Comment 5

[Robert Relyea]

I've made some comments on the bug. I think the patch is a good first step toward a solution.

Comment 6

[Malte Jürgens [:maltejur]]

Attachment: Bug 198090 - Part 1: Use AES in the SDR (NSS) r?simonf,#nss-reviewers

This adds AES to the SDR to replace 3DES. There also was previous but abandoned
work on this in D185145.

This includes a breaking change to PK11SDR_Encrypt, adding a new second
parameter CK_MECHANISM_TYPE type.

Comment 7

[Malte Jürgens [:maltejur]]

Attachment: Bug 198090 - Part 2: Use AES in the SDR (m-c) r?simonf

Comment 8

[Malte Jürgens [:maltejur]]

Attachment: WIP: Bug 198090 - Part 3: Add migration to reencrypt all logins with AES r?simonf

Comment 9

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

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D251031	Bug 198090 - Part 2: Use AES in the SDR (m-c) r?simonf	maltejur	keeler: Back Jun 16, 2025

:maltejur, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/971f65b2253f
https://hg.mozilla.org/integration/autoland/rev/ca2f55c89a21
Part 2: Use AES in the SDR (m-c) r=simonf,nss-reviewers,rrelyea

Comment 11

[amarc]

https://hg.mozilla.org/mozilla-central/rev/ca2f55c89a21

Comment 13

[Phabricator Automation]

Comment on attachment 9490461 [details]
WIP: Bug 198090 - Part 3: Add migration to reencrypt all logins with AES r?simonf

Revision D251032 was moved to bug 1974184. Setting attachment 9490461 [details] to obsolete.