Bug 1979100
| Summary: | FIPS mode AES CBC Decrypter produces incorrect result | |||
|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 8 | Reporter: | Di Wu <di.wu> | |
| Component: | golang | Assignee: | Derek Parker <deparker> | |
| Status: | CLOSED ERRATA | QA Contact: | Edjunior Barbosa Machado <emachado> | |
| Severity: | high | Docs Contact: | Eva-Lotte Gebhardt <egebhard> | |
| Priority: | unspecified | |||
| Version: | 8.4 | CC: | asm, cww, dbelyavs, dbenoit, deparker, emachado, sahana, tschelle, tstellar | |
| Target Milestone: | beta | Keywords: | Bugfix, Reopened, Triaged, ZStream | |
| Target Release: | --- | Flags: | pm-rhel:
mirror+
|
|
| Hardware: | All | |||
| OS: | All | |||
| Whiteboard: | ||||
| Fixed In Version: | go-toolset-rhel8-8050020210716084948.8aa62369 | Doc Type: | Bug Fix | |
| Doc Text: |
.Incorrect file decryption using OpenSSL `aes-cbc` mode
The OpenSSL EVP `aes-cbc` mode did not decrypt files correctly, because it expects to handle padding while the Go CryptoBlocks interface expects full blocks. This issue has been fixed by disabling padding before executing EVP operations in OpenSSL.
|
Story Points: | --- | |
| Clone Of: | ||||
| : | 1983976 (view as bug list) | Environment: | ||
| Last Closed: | 2021-11-09 17:41:36 UTC | Type: | Bug | |
| Regression: | --- | Mount Type: | --- | |
| Documentation: | --- | CRM: | ||
| Verified Versions: | Category: | --- | ||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | ||
| Cloudforms Team: | --- | Target Upstream Version: | ||
| Embargoed: | ||||
| Bug Depends On: | ||||
| Bug Blocks: | 1983976 | |||
I believe this is essentially a duplicate of rhbz#1972825 just in the opposite direction (decrypt vs encrypt). Can you verify that the patch proposed in the linked bug fixes this issue as well? The patch fixes an issue where the IV was getting incorrectly re-initialized across CryptBlocks calls. *** This bug has been marked as a duplicate of bug 1972825 *** No, this is not a duplicate because I pre-applied the other fix for the encryption, and then tested this and found the failure. Try this test case on top of the build with the other fix and you'll see the failure. It is independent of the encryption bug. Thank you for the clarification. I will reopen this bug and investigate. I have a proposed fix for this, which seem to be due to OpenSSL EVP enabling padding by default in its cipher context and automatically trying to unpad results. The proposed fix is: in goopenssl.h, we need to expose the EVP_CIPHER_CTX_set_padding c function ``` 650a651 > DEFINEFUNC(int, EVP_CIPHER_CTX_set_padding, (EVP_CIPHER_CTX *ctx, int padding), (ctx, padding)) ``` and in aes.go, as part of NewCBCDecrypter, disable padding after CipherInit ``` 237a238,240 > if C.int(1) != C._goboringcrypto_EVP_CIPHER_CTX_set_padding(x.ctx, 0) { > panic("cipher: unable to disable EVP cipher ctx padding") > } ``` This will get the unit test to pass correctly. (In reply to Di Wu from comment #14) > I have a proposed fix for this, which seem to be due to OpenSSL EVP enabling > padding by default in its cipher context and automatically trying to unpad > results. > > The proposed fix is: > in goopenssl.h, we need to expose the EVP_CIPHER_CTX_set_padding c function > ``` > 650a651 > > DEFINEFUNC(int, EVP_CIPHER_CTX_set_padding, (EVP_CIPHER_CTX *ctx, int padding), (ctx, padding)) > ``` > and in aes.go, as part of NewCBCDecrypter, disable padding after CipherInit > ``` > 237a238,240 > > if C.int(1) != C._goboringcrypto_EVP_CIPHER_CTX_set_padding(x.ctx, 0) { > > panic("cipher: unable to disable EVP cipher ctx padding") > > } > ``` > > This will get the unit test to pass correctly. Thank you, sorry there is a lot of discussion on this bug that has been Red Hat internal, but we came to that same solution and have already submitted this fix: https://pagure.io/go/c/49e7e3014ebb01378d3093bbb6dc75761ae1284a?branch=go1.16-openssl-fips. When will we get a new released RPM with both the encryption and decryption bug fixes? Di, we're planning to fix this bug in the next release. Hi guys,
I'm pretty certain the proposed fix also need to be applied identically to the AES ECB decryption code path.
I have a test case locally that uses AES ECB and it's seeing the exact same issue (unnecessary unpadding).
We need something like this please:
```
if c.dec_ctx == nil {
c.dec_ctx = C._goboringcrypto_EVP_CIPHER_CTX_new()
if c.dec_ctx == nil {
panic("cipher: unable to create EVP cipher ctx")
}
k := (*C.uchar)(unsafe.Pointer(&c.key[0]))
if C.int(1) != C._goboringcrypto_EVP_CipherInit_ex(c.dec_ctx, c.cipher, nil, k, nil, C.GO_AES_DECRYPT) {
panic("cipher: unable to initialize EVP cipher ctx")
}
if C.int(1) != C._goboringcrypto_EVP_CIPHER_CTX_set_padding(c.dec_ctx, 0) {
panic("cipher: unable to disable EVP cipher ctx padding")
}
}
```
Take the `c.dec_ctx` and also disable the padding, or else ECB mode decryption is also broken.
Thanks, and please bundle this into the same release if possible so all of these decryption issues are fixed together.
Hi guys, I see that there's a new RPM released, can we have the new fixed Golang rpm installed into a new version of the UBI8 Golang image? https://catalog.redhat.com/software/containers/rhel8/go-toolset/5b9c810add19c70b45cbd666 This is the RHEL8 UBI that we use. Thanks, Di Hi Derek, this is the first draft for this RN. Please let me know, if and what needs to be changed: .Incorrect file decryption using OpenSSL `EVP_aes-cbc` Currently, the `EVP_aes-cbc` interface of the OpenSSL library does not deprypt files correctly. To work around this issue, disable padding for the `aes-cbc` decryption. As a result, files are decripted correctly. (In reply to Eva-Lotte Gebhardt from comment #31) > Hi Derek, > > this is the first draft for this RN. Please let me know, if and what needs > to be changed: > > > .Incorrect file decryption using OpenSSL `EVP_aes-cbc` > > Currently, the `EVP_aes-cbc` interface of the OpenSSL library does not > deprypt files correctly. To work around this issue, disable padding for the > `aes-cbc` decryption. As a result, files are decripted correctly. I would just say: "OpenSSL aes-cbc mode decrypt expects to handle padding, but Go CryptoBlocks interface expects full blocks. This issue has been fixed by disabling padding before executing EVP operations in OpenSSL". Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory (Moderate: go-toolset:rhel8 security, bug fix, and enhancement update), and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHSA-2021:4156 The needinfo request[s] on this closed bug have been removed as they have been unresolved for 120 days |
Description of problem: When in FIPS mode GOLANG_FIPS=1, AES CBC block decrypter produces incorrect result on all but the first call to CryptBlocks(). See test example. ``` func TestDecryptSimple(t *testing.T) { key := []byte{ 0x24, 0xcd, 0x8b, 0x13, 0x37, 0xc5, 0xc1, 0xb1, 0x0, 0xbb, 0x27, 0x40, 0x4f, 0xab, 0x5f, 0x7b, 0x2d, 0x0, 0x20, 0xf5, 0x1, 0x84, 0x4, 0xbf, 0xe3, 0xbd, 0xa1, 0xc4, 0xbf, 0x61, 0x2f, 0xc5, } block, err := aes.NewCipher(key) if err != nil { panic(err) } iv := []byte{ 0x91, 0xc7, 0xa7, 0x54, 0x52, 0xef, 0x10, 0xdb, 0x91, 0xa8, 0x6c, 0xf9, 0x79, 0xd5, 0xac, 0x74, } plainText := []byte{ 0x54, 0x68, 0x65, 0x72, 0x65, 0x20, 0x69, 0x73, 0x20, 0x6f, 0x6e, 0x6c, 0x79, 0x20, 0x6f, 0x6e, 0x65, 0x20, 0x4c, 0x6f, 0x72, 0x64, 0x20, 0x6f, 0x66, 0x20, 0x74, 0x68, 0x65, 0x20, 0x52, 0x69, 0x6e, 0x67, 0x2c, 0x20, 0x6f, 0x6e, 0x6c, 0x79, 0x20, 0x6f, 0x6e, 0x65, 0x20, 0x77, 0x68, 0x6f, 0x20, 0x63, 0x61, 0x6e, 0x20, 0x62, 0x65, 0x6e, 0x64, 0x20, 0x69, 0x74, 0x20, 0x74, 0x6f, 0x20, 0x68, 0x69, 0x73, 0x20, 0x77, 0x69, 0x6c, 0x6c, 0x2e, 0x20, 0x41, 0x6e, 0x64, 0x20, 0x68, 0x65, 0x20, 0x64, 0x6f, 0x65, 0x73, 0x20, 0x6e, 0x6f, 0x74, 0x20, 0x73, 0x68, 0x61, 0x72, 0x65, 0x20, 0x70, 0x6f, 0x77, 0x65, 0x72, 0x2e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, } cipherText := make([]byte, len(plainText)) encrypter := cipher.NewCBCEncrypter(block, iv) encrypter.CryptBlocks(cipherText, plainText) fmt.Println(cipherText) fmt.Println("plaintext head", plainText[:16]) fmt.Println("plaintext tail", plainText[96:]) fmt.Println("ciphertext head", cipherText[:16]) fmt.Println("ciphertext tail", cipherText[96:]) expectedCipherText := []byte{ 23, 51, 192, 210, 170, 124, 30, 218, 176, 54, 70, 132, 141, 124, 3, 152, 47, 3, 37, 81, 187, 101, 197, 94, 11, 38, 128, 60, 112, 20, 235, 130, 111, 236, 176, 99, 121, 6, 221, 181, 190, 228, 150, 177, 218, 3, 196, 0, 5, 141, 169, 151, 3, 161, 64, 244, 231, 237, 252, 143, 111, 37, 68, 70, 11, 137, 220, 243, 195, 90, 182, 83, 96, 80, 122, 14, 93, 178, 62, 159, 25, 162, 200, 155, 21, 150, 6, 101, 21, 234, 12, 74, 190, 213, 159, 220, 111, 184, 94, 169, 188, 93, 38, 150, 3, 208, 185, 201, 212, 246, 238, 181, } if bytes.Compare(expectedCipherText, cipherText) != 0 { t.Fail() } else { fmt.Println("ciphertext matches expected") } firstHalf := make([]byte, 64) secondHalf := make([]byte, 48) decrypter := cipher.NewCBCDecrypter(block, iv) // first call decrypts correctly decrypter.CryptBlocks(firstHalf, cipherText[:64]) // subsequent call decrypts incorrectly decrypter.CryptBlocks(secondHalf, cipherText[64:]) decrypted := make([]byte, 0) decrypted = append(decrypted, firstHalf...) decrypted = append(decrypted, secondHalf...) if len(decrypted) != 112 { t.Fail() } fmt.Println("decrypted head", decrypted[:16]) fmt.Println("decrypted tail", decrypted[96:]) if bytes.Compare(plainText[:64], firstHalf) != 0 { fmt.Printf("first half incorrect\nexpected %v, got %v\n", plainText[:64], firstHalf) t.Fail() } else { fmt.Println("first half decrypted correctly") } if bytes.Compare(plainText[64:], secondHalf) != 0 { fmt.Printf("second half incorrect\nexpected %v, got %v\n", plainText[64:], secondHalf) t.Fail() } else { fmt.Println("second half decrypted correctly") } } ``` Version-Release number of selected component (if applicable): go version go1.15.13 linux/amd64 How reproducible: Run the unit test with FIPS mode on vs off, test passes when FIPS is off but fails when FIPS is on. ``` GOLANG_FIPS=0 go test -v -run TestDecryptSimple # pass GOLANG_FIPS=1 go test -v -run TestDecryptSimple # fail ``` Steps to Reproduce: 1. Turn on FIPS mode with env variable GOLANG_FIPS=1 2. Run the decryption test 3. Verify that test passes Actual results: ``` === RUN TestDecryptSimple [23 51 192 210 170 124 30 218 176 54 70 132 141 124 3 152 47 3 37 81 187 101 197 94 11 38 128 60 112 20 235 130 111 236 176 99 121 6 221 181 190 228 150 177 218 3 196 0 5 141 169 151 3 161 64 244 231 237 252 143 111 37 68 70 11 137 220 243 195 90 182 83 96 80 122 14 93 178 62 159 25 162 200 155 21 150 6 101 21 234 12 74 190 213 159 220 111 184 94 169 188 93 38 150 3 208 185 201 212 246 238 181] plaintext head [84 104 101 114 101 32 105 115 32 111 110 108 121 32 111 110] plaintext tail [112 111 119 101 114 46 0 0 0 0 0 0 0 0 0 0] ciphertext head [23 51 192 210 170 124 30 218 176 54 70 132 141 124 3 152] ciphertext tail [111 184 94 169 188 93 38 150 3 208 185 201 212 246 238 181] ciphertext matches expected decrypted head [84 104 101 114 101 32 105 115 32 111 110 108 121 32 111 110] decrypted tail [32 100 111 101 115 32 110 111 116 32 115 104 97 114 101 32] first half decrypted correctly second half incorrect expected [104 105 115 32 119 105 108 108 46 32 65 110 100 32 104 101 32 100 111 101 115 32 110 111 116 32 115 104 97 114 101 32 112 111 119 101 114 46 0 0 0 0 0 0 0 0 0 0], got [32 99 97 110 32 98 101 110 100 32 105 116 32 116 111 32 104 105 115 32 119 105 108 108 46 32 65 110 100 32 104 101 32 100 111 101 115 32 110 111 116 32 115 104 97 114 101 32] --- FAIL: TestDecryptSimple (0.00s) FAIL ``` Expected results: ``` === RUN TestDecryptSimple [23 51 192 210 170 124 30 218 176 54 70 132 141 124 3 152 47 3 37 81 187 101 197 94 11 38 128 60 112 20 235 130 111 236 176 99 121 6 221 181 190 228 150 177 218 3 196 0 5 141 169 151 3 161 64 244 231 237 252 143 111 37 68 70 11 137 220 243 195 90 182 83 96 80 122 14 93 178 62 159 25 162 200 155 21 150 6 101 21 234 12 74 190 213 159 220 111 184 94 169 188 93 38 150 3 208 185 201 212 246 238 181] plaintext head [84 104 101 114 101 32 105 115 32 111 110 108 121 32 111 110] plaintext tail [112 111 119 101 114 46 0 0 0 0 0 0 0 0 0 0] ciphertext head [23 51 192 210 170 124 30 218 176 54 70 132 141 124 3 152] ciphertext tail [111 184 94 169 188 93 38 150 3 208 185 201 212 246 238 181] ciphertext matches expected decrypted head [84 104 101 114 101 32 105 115 32 111 110 108 121 32 111 110] decrypted tail [112 111 119 101 114 46 0 0 0 0 0 0 0 0 0 0] first half decrypted correctly second half decrypted correctly --- PASS: TestDecryptSimple (0.00s) PASS ``` Additional info: I did some debugging and it feels like there's a bug with the cipher context EVP_CIPHER_CTX not carrying correct chaining across each _goboringcrypto_EVP_CipherUpdate call