1619959 - NSS: SEED_ECB decryption leaves output buffer partially uninitialized (data leak)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Guido Vranken]

Attachment: poc_seed_ecb.cpp

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

Compile and run the attached C++ file.

Actual results:

Output is:

92 10 a1 6d 2c 97 46 5f 8d 99 67 91 fb 75 d7 bc ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab

Expected results:

Output should be:

92 10 a1 6d 2c 97 46 5f 8d 99 67 91 fb 75 d7 bc ca 50 65 2d cc ac e4 70 8e e2 32 11 83 d2 56 a0 fe 6a 6f a9 47 10 7b a9 3b d7 77 e9 75 1b e7 fb 64 13 2c f8 eb 21 06 54 40 45 e9 2a b4 57 bc 13 71 44 12 10 63 1c 66 be 38 39 e1 ce fa 2c e5 94 bb 8d 58 62 54 fb f5 d5 db 62 19 6a 14 5d d8 c8 bc c8 7b 57 e4 54 17 07 5d e3 5b ab 1e d9 c1 38 f4 9e bd dc 2a 8c ca 9b 83 7b a6 e4 2f ea 38 12

Comment 1

[Daniel Veditz [:dveditz]]

I'm surprised SEED code is still in NSS. Not accessible to Gecko products.

jcj: is SEED enabled by default in NSS? Still supported?

Comment 2

[J.C. Jones [:jcj] (he/him)]

It's not enabled for TLS, but it's built by default and exposed via PKCS11. I presume some customers of NSS still use it. Adding in the NSS team.

Comment 3

[J.C. Jones [:jcj] (he/him)]

Daiki,

Any chance we can unship SEED instead of fixing this?

Comment 4

[Daiki Ueno [:ueno]]

Thanks for letting us know. We discussed internally and found that none of our components (including libreswan) is using SEED, and it is disabled by default since RHEL-6.8. However, it is still possible that there can be external use of that cipher. So our recommendation is to first deprecate it with a compile time option (e.g., #ifdef), and then to completely remove it. Would that work for you?

Comment 5

[J.C. Jones [:jcj] (he/him)]

I think deprecation makes sense to me. We should do it soon and default to disabled.

This bug should probably be specific to fixing the implementation. I'll open a non-sec bug to disable SEED by default.

Comment 6

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

The priority flag is not set for this bug.
:jcj, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[J.C. Jones [:jcj] (he/him)]

Guido,

As we continue to reason through this one a bit, just a clarifying question -

This is mostly padding in the ciphertext, right? And you believe the outLen is being set wrong?

Comment 8

[Guido Vranken]

Only the first 16 bytes of the output buffer are written to, while indicating that the entire output buffer is written to (PK11_CipherOp sets outLen to 128).

As far as I can tell, the correct behavior would be:

• Encrypt the entire cleartext (128 bytes)
• Set outLen to 128

For reference, you can change 'CKM_SEED_ECB' to 'CKM_AES_ECB' in my proof of concept and see what happens.

I think the reason this happens with SEED_ECB is because it lacks a looping mechanism: https://github.com/nss-dev/nss/blob/10ba9251ece47e7b076d864d29247c642e600580/lib/freebl/seed.c#L417

Eg. it encrypts only one block at a time.

Compare to:

Camellia ECB: https://github.com/nss-dev/nss/blob/10ba9251ece47e7b076d864d29247c642e600580/lib/freebl/camellia.c#L1606
DES ECB: https://github.com/nss-dev/nss/blob/10ba9251ece47e7b076d864d29247c642e600580/lib/freebl/desblapi.c#L37
AES ECB: https://github.com/nss-dev/nss/blob/10ba9251ece47e7b076d864d29247c642e600580/lib/freebl/rijndael.c#L742

So, the solution is to implement a loop in SEED_ecb_encrypt which processes multiple blocks, similar to the other ciphers in ECB mode.

Comment 9

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1619959 - Properly handle multi-block SEED ECB inputs.

Iterate on the input when needed, instead of only handling the first block.

Will likely need a rebase atop the SEED disable-by-default patch.

Comment 10

[Kevin Jacobs [:kjacobs]]

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

Thanks!

Comment 11

[Guido Vranken]

Great, thanks.