Closed Bug 1619959 Opened 2 years ago Closed 2 years ago

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

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox-esr68 disabled, firefox73 disabled, firefox74 disabled, firefox75 disabled)

RESOLVED FIXED
Tracking Status
firefox-esr68 --- disabled
firefox73 --- disabled
firefox74 --- disabled
firefox75 --- disabled

People

(Reporter: guidovranken, Unassigned)

References

Details

(Keywords: sec-low, Whiteboard: not available in Gecko products)

Attachments

(2 files)

Attached file 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

Attachment #9130776 - Attachment mime type: text/x-c++src → text/plain

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

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

Flags: needinfo?(jjones)
Whiteboard: not available in Gecko products

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.

Flags: needinfo?(jjones)

Daiki,

Any chance we can unship SEED instead of fixing this?

Flags: needinfo?(dueno)

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?

Flags: needinfo?(dueno)

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.

Keywords: sec-low

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jjones)

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?

Flags: needinfo?(jjones) → needinfo?(guidovranken)

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.

Flags: needinfo?(guidovranken)

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.

Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.52

Great, thanks.

Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.