Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to set CBC padding behavior? #209

Closed
3 tasks done
lovetodream opened this issue Nov 14, 2023 · 6 comments · Fixed by #210
Closed
3 tasks done

How to set CBC padding behavior? #209

lovetodream opened this issue Nov 14, 2023 · 6 comments · Fixed by #210

Comments

@lovetodream
Copy link
Contributor

Question Checklist

Question Subject

I'm currently using CryptoSwift for CBC de-/encryption and I'd like to move that to swift-crypto.
For my use-case I have to disable automatic padding, is that possible?

Question Description

Currently I have the following code to decrypt a CBC encrypted payload sent from an Oracle db server during authenticating:

func decryptCBC(_ key: [UInt8], _ encryptedText: [UInt8]) throws -> [UInt8] {
    let iv = [UInt8](repeating: 0, count: 16)
    let aes = try AES(key: key, blockMode: CBC(iv: iv), padding: .noPadding)
    return try aes.decrypt(encryptedText)
}

I tried migrating it to swift-crypto and now it does automatic padding and fails with CryptoKitError.incorrectParameterSize, when trying to trim the padding:

func decryptCBC(_ key: [UInt8], _ encryptedText: [UInt8]) throws -> Data {
    let iv = [UInt8](repeating: 0, count: 16)
    return try AES._CBC.decrypt(encryptedText, using: .init(data: key), iv: AES._CBC.IV(ivBytes: iv)) // noPadding equivalent?
}

I guess I'll have the same problem on encryption.

// current (CryptoSwift):
func encryptCBC(_ key: [UInt8], _ plainText: [UInt8], zeros: Bool = false) throws -> [UInt8] {
    var plainText = plainText
    let blockSize = 16
    let iv = [UInt8](repeating: 0, count: blockSize)
    let n = blockSize - plainText.count % blockSize
    if n != 0, !zeros {
        plainText += Array<UInt8>(repeating: UInt8(n), count: n)
    }
    let aes = try AES(key: key, blockMode: CBC(iv: iv), padding: zeros ? .zeroPadding : .noPadding)
    var encryptor = try aes.makeEncryptor()
    return try encryptor.update(withBytes: plainText, isLast: true) + encryptor.finish()
}

// new (swift-crypto):
func encryptCBC(_ key: [UInt8], _ plainText: [UInt8], zeros: Bool = false) throws -> Data {
    var plainText = plainText
    let blockSize = 16
    let iv = [UInt8](repeating: 0, count: blockSize)
    let n = blockSize - plainText.count % blockSize
    if n != 0, !zeros {
        plainText += Array<UInt8>(repeating: UInt8(n), count: n)
    }
    return try AES._CBC.encrypt(plainText, using: .init(data: key), iv: AES._CBC.IV(ivBytes: iv)) // set padding behavior?
}

This is the related code: https://github.com/lovetodream/oracle-nio/blob/main/Sources/OracleNIO/Helper/Crypto.swift

@Lukasa
Copy link
Contributor

Lukasa commented Nov 14, 2023

This code is interesting: your padding is almost a perfect match for the PKCS#7 padding we use in Crypto except in the following cases:

  1. With zeros set to true, you pad with zeros. This is almost never a good thing to do, which makes me curious why Oracle does it.
  2. With zeros set to false, you pad using PKCS#8 except if you have a perfect match to the block size when you don't pad at all.

This is a very strange way to use CBC. No padding at all is a roughly acceptable thing to do, I think, but the zero padding mode is deeply confusing. Is it really used?

@lovetodream
Copy link
Contributor Author

lovetodream commented Nov 14, 2023

Zero padding is only used once (for debugging purposes), although I didn't implement that yet. It seems like there are other options for that too, but I'd have to ask someone at Oracle. It's not really necessary for the driver.

But enabling no padding would be required in order to do the authentication properly

@Lukasa
Copy link
Contributor

Lukasa commented Nov 14, 2023

So we can add an overload here that avoids the padding. It should be pretty easy really, it's much like the identical code with a check that the length is exactly a multiple of the block size and then skipping appending the final padding block. The existing implementation and the new one can funnel into the same shared code.

Would you be open to writing that patch?

@lovetodream
Copy link
Contributor Author

Yeah, sure

@lovetodream
Copy link
Contributor Author

Do you know of any test files I could include for testing the version without padding @Lukasa?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 16, 2023

There isn't any good source for this: unpadded CBC is pretty rare. You may be able to find some in some RFCs.

Lukasa pushed a commit that referenced this issue Dec 5, 2023
Motivation:

As described in #209, I personally need this to migrate an oracle driver from a third party crypto lib to swift-crypto. I think other users might benefit from this addition too.

Modifications:

I've added an overload to the encrypt and decrypt methods of AES._CBC, allowing the user to configure if padding should be added or not. With noPadding set to true, an error will be thrown if the plaintext isn't a multiple of the block size. I've added the corresponding inline documentation.

I've also added tests to ensure both encrypting and decrypting without padding work as expected. Although those tests might not be sufficient enough, because I couldn't find good resources online. I've created a bunch of random hex strings and encrypted/decrypted them using another implementation of paddingless CBC and checked if I receive the expected results. To further validate the feature, I've tested it as part of the authentication in oracle-nio, which worked in all test scenarios I've been running.

Result:

After merging this, it will be possible to use CBC without padding. This closes #209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants