| Message ID | 20210706095924.764117-12-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: misc cleanup and introduce gnutls backend driver | expand |
Daniel P. Berrangé <berrange@redhat.com> writes: > Currently the crypto layer exposes support for a 'des-rfb' > algorithm which is just normal single-DES, with the bits > in each key byte reversed. This special key munging is > required by the RFB protocol password authentication > mechanism. > > Since the crypto layer is generic shared code, it makes > more sense to do the key byte munging in the VNC server > code, and expose normal single-DES support. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- [...] > diff --git a/qapi/crypto.json b/qapi/crypto.json > index 7116ae9a46..6b3fadabac 100644 > --- a/qapi/crypto.json > +++ b/qapi/crypto.json > @@ -66,7 +66,7 @@ > # @aes-128: AES with 128 bit / 16 byte keys > # @aes-192: AES with 192 bit / 24 byte keys > # @aes-256: AES with 256 bit / 32 byte keys > -# @des-rfb: RFB specific variant of single DES. Do not use except in VNC. > +# @des: DES with 56 bit / 8 byte keys. Do not use except in VNC. > # @3des: 3DES(EDE) with 192 bit / 24 byte keys (since 2.9) > # @cast5-128: Cast5 with 128 bit / 16 byte keys > # @serpent-128: Serpent with 128 bit / 16 byte keys > @@ -80,7 +80,7 @@ > { 'enum': 'QCryptoCipherAlgorithm', > 'prefix': 'QCRYPTO_CIPHER_ALG', > 'data': ['aes-128', 'aes-192', 'aes-256', > - 'des-rfb', '3des', > + 'des', '3des', > 'cast5-128', > 'serpent-128', 'serpent-192', 'serpent-256', > 'twofish-128', 'twofish-192', 'twofish-256']} Is enum value "des-rfb" part of any external interface? [...]
On Wed, Jul 07, 2021 at 02:47:15PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > Currently the crypto layer exposes support for a 'des-rfb' > > algorithm which is just normal single-DES, with the bits > > in each key byte reversed. This special key munging is > > required by the RFB protocol password authentication > > mechanism. > > > > Since the crypto layer is generic shared code, it makes > > more sense to do the key byte munging in the VNC server > > code, and expose normal single-DES support. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > [...] > > > diff --git a/qapi/crypto.json b/qapi/crypto.json > > index 7116ae9a46..6b3fadabac 100644 > > --- a/qapi/crypto.json > > +++ b/qapi/crypto.json > > @@ -66,7 +66,7 @@ > > # @aes-128: AES with 128 bit / 16 byte keys > > # @aes-192: AES with 192 bit / 24 byte keys > > # @aes-256: AES with 256 bit / 32 byte keys > > -# @des-rfb: RFB specific variant of single DES. Do not use except in VNC. > > +# @des: DES with 56 bit / 8 byte keys. Do not use except in VNC. > > # @3des: 3DES(EDE) with 192 bit / 24 byte keys (since 2.9) > > # @cast5-128: Cast5 with 128 bit / 16 byte keys > > # @serpent-128: Serpent with 128 bit / 16 byte keys > > @@ -80,7 +80,7 @@ > > { 'enum': 'QCryptoCipherAlgorithm', > > 'prefix': 'QCRYPTO_CIPHER_ALG', > > 'data': ['aes-128', 'aes-192', 'aes-256', > > - 'des-rfb', '3des', > > + 'des', '3des', > > 'cast5-128', > > 'serpent-128', 'serpent-192', 'serpent-256', > > 'twofish-128', 'twofish-192', 'twofish-256']} > > Is enum value "des-rfb" part of any external interface? Strictly speaking, yes, but in reality it doesn't matter. The only place in QEMU that actually uses DES-RFB is the VNC server code. That is an indirect usage when the user sets the "password" option flag in QemuOpts. The fact that it uses DES-RFB is an internal impl detail. The one place that does publically expose ability to set a field using the QCryptoCipherAlgorithm enum type is the LUKS support in the block layer: { 'struct': 'QCryptoBlockCreateOptionsLUKS', 'base': 'QCryptoBlockOptionsLUKS', 'data': { '*cipher-alg': 'QCryptoCipherAlgorithm', '*cipher-mode': 'QCryptoCipherMode', '*ivgen-alg': 'QCryptoIVGenAlgorithm', '*ivgen-hash-alg': 'QCryptoHashAlgorithm', '*hash-alg': 'QCryptoHashAlgorithm', '*iter-time': 'int'}} eg exposed on CLI as: $ qemu-img create -f luks -o cipher-alg=NNN foo.luks 1G or equivalant with QMP blockdev-create While the QMP schema allows any valid QCryptoCipherAlgorithm string to be set, the actual implementation does not. The crypto/block-luks.c code has a map between cipher algs and LUKS format algoritm names: static const QCryptoBlockLUKSCipherNameMap qcrypto_block_luks_cipher_name_map[] = { { "aes", qcrypto_block_luks_cipher_size_map_aes }, { "cast5", qcrypto_block_luks_cipher_size_map_cast5 }, { "serpent", qcrypto_block_luks_cipher_size_map_serpent }, { "twofish", qcrypto_block_luks_cipher_size_map_twofish }, }; If it isn't in that table, it can't be used. IOW, the only scenario we're affecting in this rename is one which would already result in an error condition Original behaviour: $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-rfb qemu-img: demo.luks: Algorithm 'des-rfb' not supported New behaviour: $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-fish qemu-img: demo.luks: Invalid parameter 'des-rfb' I considered this incompatibility to be acceptable, and thus not worth going through a deprecation dance. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Jul 07, 2021 at 02:47:15PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > Currently the crypto layer exposes support for a 'des-rfb' >> > algorithm which is just normal single-DES, with the bits >> > in each key byte reversed. This special key munging is >> > required by the RFB protocol password authentication >> > mechanism. >> > >> > Since the crypto layer is generic shared code, it makes >> > more sense to do the key byte munging in the VNC server >> > code, and expose normal single-DES support. >> > >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >> > --- >> >> [...] >> >> > diff --git a/qapi/crypto.json b/qapi/crypto.json >> > index 7116ae9a46..6b3fadabac 100644 >> > --- a/qapi/crypto.json >> > +++ b/qapi/crypto.json >> > @@ -66,7 +66,7 @@ >> > # @aes-128: AES with 128 bit / 16 byte keys >> > # @aes-192: AES with 192 bit / 24 byte keys >> > # @aes-256: AES with 256 bit / 32 byte keys >> > -# @des-rfb: RFB specific variant of single DES. Do not use except in VNC. >> > +# @des: DES with 56 bit / 8 byte keys. Do not use except in VNC. >> > # @3des: 3DES(EDE) with 192 bit / 24 byte keys (since 2.9) >> > # @cast5-128: Cast5 with 128 bit / 16 byte keys >> > # @serpent-128: Serpent with 128 bit / 16 byte keys >> > @@ -80,7 +80,7 @@ >> > { 'enum': 'QCryptoCipherAlgorithm', >> > 'prefix': 'QCRYPTO_CIPHER_ALG', >> > 'data': ['aes-128', 'aes-192', 'aes-256', >> > - 'des-rfb', '3des', >> > + 'des', '3des', >> > 'cast5-128', >> > 'serpent-128', 'serpent-192', 'serpent-256', >> > 'twofish-128', 'twofish-192', 'twofish-256']} >> >> Is enum value "des-rfb" part of any external interface? > > Strictly speaking, yes, but in reality it doesn't matter. > > > The only place in QEMU that actually uses DES-RFB is the > VNC server code. That is an indirect usage when the user > sets the "password" option flag in QemuOpts. The fact that > it uses DES-RFB is an internal impl detail. > > The one place that does publically expose ability to set a > field using the QCryptoCipherAlgorithm enum type is the > LUKS support in the block layer: > > { 'struct': 'QCryptoBlockCreateOptionsLUKS', > 'base': 'QCryptoBlockOptionsLUKS', > 'data': { '*cipher-alg': 'QCryptoCipherAlgorithm', > '*cipher-mode': 'QCryptoCipherMode', > '*ivgen-alg': 'QCryptoIVGenAlgorithm', > '*ivgen-hash-alg': 'QCryptoHashAlgorithm', > '*hash-alg': 'QCryptoHashAlgorithm', > '*iter-time': 'int'}} > > eg exposed on CLI as: > > $ qemu-img create -f luks -o cipher-alg=NNN foo.luks 1G > > or equivalant with QMP blockdev-create > > While the QMP schema allows any valid QCryptoCipherAlgorithm > string to be set, the actual implementation does not. > > The crypto/block-luks.c code has a map between cipher algs > and LUKS format algoritm names: > > > static const QCryptoBlockLUKSCipherNameMap > qcrypto_block_luks_cipher_name_map[] = { > { "aes", qcrypto_block_luks_cipher_size_map_aes }, > { "cast5", qcrypto_block_luks_cipher_size_map_cast5 }, > { "serpent", qcrypto_block_luks_cipher_size_map_serpent }, > { "twofish", qcrypto_block_luks_cipher_size_map_twofish }, > }; > > If it isn't in that table, it can't be used. IOW, the only > scenario we're affecting in this rename is one which would > already result in an error condition > > Original behaviour: > > $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G > Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-rfb > qemu-img: demo.luks: Algorithm 'des-rfb' not supported > > New behaviour: > > $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G > Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-fish > qemu-img: demo.luks: Invalid parameter 'des-rfb' > > I considered this incompatibility to be acceptable, and thus > not worth going through a deprecation dance. Thanks for the explanation. I agree the deprecation dance is not necessary here. Please consider explaining this in your commit message. Suggest to append a variation of the tail of your explanation: Replacing cipher 'des-rfb' by 'des' looks like an incompatible interface change, but it doesn't matter. While the QMP schema allows ... qemu-img: demo.luks: Invalid parameter 'des-rfb' Also consider tweaking the title to crypto: Replace 'des-rfb' cipher by 'des' because it's not actually just a rename. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Tue, Jul 06, 2021 at 10:59:17AM +0100, Daniel P. Berrangé wrote: > Currently the crypto layer exposes support for a 'des-rfb' > algorithm which is just normal single-DES, with the bits > in each key byte reversed. This special key munging is > required by the RFB protocol password authentication > mechanism. > > Since the crypto layer is generic shared code, it makes > more sense to do the key byte munging in the VNC server > code, and expose normal single-DES support. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> I agree with Markus' suggestion to enhance the commit message. > +++ b/tests/unit/test-crypto-cipher.c > @@ -155,28 +155,28 @@ static QCryptoCipherTestData test_data[] = { > * in single AES block, and gives identical > * ciphertext in ECB and CBC modes > */ > - .path = "/crypto/cipher/des-rfb-ecb-56-one-block", > - .alg = QCRYPTO_CIPHER_ALG_DES_RFB, > + .path = "/crypto/cipher/des-ecb-56-one-block", > + .alg = QCRYPTO_CIPHER_ALG_DES, > .mode = QCRYPTO_CIPHER_MODE_ECB, > - .key = "0123456789abcdef", > + .key = "80c4a2e691d5b3f7", > .plaintext = "70617373776f7264", > .ciphertext = "73fa80b66134e403", > }, This is a rather cute way to avoid recomputing the canonical .ciphertext due to the change in bit ordering. Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Jul 08, 2021 at 04:41:28PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Jul 07, 2021 at 02:47:15PM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > Currently the crypto layer exposes support for a 'des-rfb' > >> > algorithm which is just normal single-DES, with the bits > >> > in each key byte reversed. This special key munging is > >> > required by the RFB protocol password authentication > >> > mechanism. > >> > > >> > Since the crypto layer is generic shared code, it makes > >> > more sense to do the key byte munging in the VNC server > >> > code, and expose normal single-DES support. > >> > > >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > >> > --- > >> > >> [...] > >> > >> > diff --git a/qapi/crypto.json b/qapi/crypto.json > >> > index 7116ae9a46..6b3fadabac 100644 > >> > --- a/qapi/crypto.json > >> > +++ b/qapi/crypto.json > >> > @@ -66,7 +66,7 @@ > >> > # @aes-128: AES with 128 bit / 16 byte keys > >> > # @aes-192: AES with 192 bit / 24 byte keys > >> > # @aes-256: AES with 256 bit / 32 byte keys > >> > -# @des-rfb: RFB specific variant of single DES. Do not use except in VNC. > >> > +# @des: DES with 56 bit / 8 byte keys. Do not use except in VNC. > >> > # @3des: 3DES(EDE) with 192 bit / 24 byte keys (since 2.9) > >> > # @cast5-128: Cast5 with 128 bit / 16 byte keys > >> > # @serpent-128: Serpent with 128 bit / 16 byte keys > >> > @@ -80,7 +80,7 @@ > >> > { 'enum': 'QCryptoCipherAlgorithm', > >> > 'prefix': 'QCRYPTO_CIPHER_ALG', > >> > 'data': ['aes-128', 'aes-192', 'aes-256', > >> > - 'des-rfb', '3des', > >> > + 'des', '3des', > >> > 'cast5-128', > >> > 'serpent-128', 'serpent-192', 'serpent-256', > >> > 'twofish-128', 'twofish-192', 'twofish-256']} > >> > >> Is enum value "des-rfb" part of any external interface? > > > > Strictly speaking, yes, but in reality it doesn't matter. > > > > > > The only place in QEMU that actually uses DES-RFB is the > > VNC server code. That is an indirect usage when the user > > sets the "password" option flag in QemuOpts. The fact that > > it uses DES-RFB is an internal impl detail. > > > > The one place that does publically expose ability to set a > > field using the QCryptoCipherAlgorithm enum type is the > > LUKS support in the block layer: > > > > { 'struct': 'QCryptoBlockCreateOptionsLUKS', > > 'base': 'QCryptoBlockOptionsLUKS', > > 'data': { '*cipher-alg': 'QCryptoCipherAlgorithm', > > '*cipher-mode': 'QCryptoCipherMode', > > '*ivgen-alg': 'QCryptoIVGenAlgorithm', > > '*ivgen-hash-alg': 'QCryptoHashAlgorithm', > > '*hash-alg': 'QCryptoHashAlgorithm', > > '*iter-time': 'int'}} > > > > eg exposed on CLI as: > > > > $ qemu-img create -f luks -o cipher-alg=NNN foo.luks 1G > > > > or equivalant with QMP blockdev-create > > > > While the QMP schema allows any valid QCryptoCipherAlgorithm > > string to be set, the actual implementation does not. > > > > The crypto/block-luks.c code has a map between cipher algs > > and LUKS format algoritm names: > > > > > > static const QCryptoBlockLUKSCipherNameMap > > qcrypto_block_luks_cipher_name_map[] = { > > { "aes", qcrypto_block_luks_cipher_size_map_aes }, > > { "cast5", qcrypto_block_luks_cipher_size_map_cast5 }, > > { "serpent", qcrypto_block_luks_cipher_size_map_serpent }, > > { "twofish", qcrypto_block_luks_cipher_size_map_twofish }, > > }; > > > > If it isn't in that table, it can't be used. IOW, the only > > scenario we're affecting in this rename is one which would > > already result in an error condition > > > > Original behaviour: > > > > $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G > > Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-rfb > > qemu-img: demo.luks: Algorithm 'des-rfb' not supported > > > > New behaviour: > > > > $ qemu-img create -f luks --object secret,id=sec0,data=123 -o cipher-alg=des-rfb,key-secret=sec0 demo.luks 1G > > Formatting 'demo.luks', fmt=luks size=1073741824 key-secret=sec0 cipher-alg=des-fish > > qemu-img: demo.luks: Invalid parameter 'des-rfb' > > > > I considered this incompatibility to be acceptable, and thus > > not worth going through a deprecation dance. > > Thanks for the explanation. I agree the deprecation dance is not > necessary here. > > Please consider explaining this in your commit message. Suggest to > append a variation of the tail of your explanation: > > Replacing cipher 'des-rfb' by 'des' looks like an incompatible > interface change, but it doesn't matter. While the QMP schema allows > ... > qemu-img: demo.luks: Invalid parameter 'des-rfb' > > Also consider tweaking the title to > > crypto: Replace 'des-rfb' cipher by 'des' > > because it's not actually just a rename. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks, I've updated the commit msg as suggested Regards, Daniel
diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc index 3aab08a1a9..a6a0117717 100644 --- a/crypto/cipher-gcrypt.c.inc +++ b/crypto/cipher-gcrypt.c.inc @@ -24,7 +24,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, QCryptoCipherMode mode) { switch (alg) { - case QCRYPTO_CIPHER_ALG_DES_RFB: + case QCRYPTO_CIPHER_ALG_DES: case QCRYPTO_CIPHER_ALG_3DES: case QCRYPTO_CIPHER_ALG_AES_128: case QCRYPTO_CIPHER_ALG_AES_192: @@ -186,7 +186,7 @@ static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg, } switch (alg) { - case QCRYPTO_CIPHER_ALG_DES_RFB: + case QCRYPTO_CIPHER_ALG_DES: gcryalg = GCRY_CIPHER_DES; break; case QCRYPTO_CIPHER_ALG_3DES: @@ -257,17 +257,7 @@ static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg, } ctx->blocksize = gcry_cipher_get_algo_blklen(gcryalg); - if (alg == QCRYPTO_CIPHER_ALG_DES_RFB) { - /* We're using standard DES cipher from gcrypt, so we need - * to munge the key so that the results are the same as the - * bizarre RFB variant of DES :-) - */ - uint8_t *rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey); - err = gcry_cipher_setkey(ctx->handle, rfbkey, nkey); - g_free(rfbkey); - } else { - err = gcry_cipher_setkey(ctx->handle, key, nkey); - } + err = gcry_cipher_setkey(ctx->handle, key, nkey); if (err != 0) { error_setg(errp, "Cannot set key: %s", gcry_strerror(err)); goto error; diff --git a/crypto/cipher-nettle.c.inc b/crypto/cipher-nettle.c.inc index fc6f40c026..24cc61f87b 100644 --- a/crypto/cipher-nettle.c.inc +++ b/crypto/cipher-nettle.c.inc @@ -235,11 +235,11 @@ static const struct QCryptoCipherDriver NAME##_driver_xts = { \ DEFINE_XTS(NAME, TYPE, BLEN, ENCRYPT, DECRYPT) -typedef struct QCryptoNettleDESRFB { +typedef struct QCryptoNettleDES { QCryptoCipher base; struct des_ctx key; uint8_t iv[DES_BLOCK_SIZE]; -} QCryptoNettleDESRFB; +} QCryptoNettleDES; static void des_encrypt_native(const void *ctx, size_t length, uint8_t *dst, const uint8_t *src) @@ -253,7 +253,7 @@ static void des_decrypt_native(const void *ctx, size_t length, des_decrypt(ctx, length, dst, src); } -DEFINE_ECB_CBC_CTR(qcrypto_nettle_des_rfb, QCryptoNettleDESRFB, +DEFINE_ECB_CBC_CTR(qcrypto_nettle_des, QCryptoNettleDES, DES_BLOCK_SIZE, des_encrypt_native, des_decrypt_native) @@ -431,7 +431,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, QCryptoCipherMode mode) { switch (alg) { - case QCRYPTO_CIPHER_ALG_DES_RFB: + case QCRYPTO_CIPHER_ALG_DES: case QCRYPTO_CIPHER_ALG_3DES: case QCRYPTO_CIPHER_ALG_AES_128: case QCRYPTO_CIPHER_ALG_AES_192: @@ -480,32 +480,28 @@ static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg, } switch (alg) { - case QCRYPTO_CIPHER_ALG_DES_RFB: + case QCRYPTO_CIPHER_ALG_DES: { - QCryptoNettleDESRFB *ctx; + QCryptoNettleDES *ctx; const QCryptoCipherDriver *drv; - uint8_t *rfbkey; switch (mode) { case QCRYPTO_CIPHER_MODE_ECB: - drv = &qcrypto_nettle_des_rfb_driver_ecb; + drv = &qcrypto_nettle_des_driver_ecb; break; case QCRYPTO_CIPHER_MODE_CBC: - drv = &qcrypto_nettle_des_rfb_driver_cbc; + drv = &qcrypto_nettle_des_driver_cbc; break; case QCRYPTO_CIPHER_MODE_CTR: - drv = &qcrypto_nettle_des_rfb_driver_ctr; + drv = &qcrypto_nettle_des_driver_ctr; break; default: goto bad_cipher_mode; } - ctx = g_new0(QCryptoNettleDESRFB, 1); + ctx = g_new0(QCryptoNettleDES, 1); ctx->base.driver = drv; - - rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey); - des_set_key(&ctx->key, rfbkey); - g_free(rfbkey); + des_set_key(&ctx->key, key); return &ctx->base; } diff --git a/crypto/cipher.c b/crypto/cipher.c index 068b2fb867..1f5528be49 100644 --- a/crypto/cipher.c +++ b/crypto/cipher.c @@ -29,7 +29,7 @@ static const size_t alg_key_len[QCRYPTO_CIPHER_ALG__MAX] = { [QCRYPTO_CIPHER_ALG_AES_128] = 16, [QCRYPTO_CIPHER_ALG_AES_192] = 24, [QCRYPTO_CIPHER_ALG_AES_256] = 32, - [QCRYPTO_CIPHER_ALG_DES_RFB] = 8, + [QCRYPTO_CIPHER_ALG_DES] = 8, [QCRYPTO_CIPHER_ALG_3DES] = 24, [QCRYPTO_CIPHER_ALG_CAST5_128] = 16, [QCRYPTO_CIPHER_ALG_SERPENT_128] = 16, @@ -44,7 +44,7 @@ static const size_t alg_block_len[QCRYPTO_CIPHER_ALG__MAX] = { [QCRYPTO_CIPHER_ALG_AES_128] = 16, [QCRYPTO_CIPHER_ALG_AES_192] = 16, [QCRYPTO_CIPHER_ALG_AES_256] = 16, - [QCRYPTO_CIPHER_ALG_DES_RFB] = 8, + [QCRYPTO_CIPHER_ALG_DES] = 8, [QCRYPTO_CIPHER_ALG_3DES] = 8, [QCRYPTO_CIPHER_ALG_CAST5_128] = 8, [QCRYPTO_CIPHER_ALG_SERPENT_128] = 16, @@ -107,9 +107,9 @@ qcrypto_cipher_validate_key_length(QCryptoCipherAlgorithm alg, } if (mode == QCRYPTO_CIPHER_MODE_XTS) { - if (alg == QCRYPTO_CIPHER_ALG_DES_RFB - || alg == QCRYPTO_CIPHER_ALG_3DES) { - error_setg(errp, "XTS mode not compatible with DES-RFB/3DES"); + if (alg == QCRYPTO_CIPHER_ALG_DES || + alg == QCRYPTO_CIPHER_ALG_3DES) { + error_setg(errp, "XTS mode not compatible with DES/3DES"); return false; } if (nkey % 2) { @@ -132,24 +132,6 @@ qcrypto_cipher_validate_key_length(QCryptoCipherAlgorithm alg, return true; } -#if defined(CONFIG_GCRYPT) || defined(CONFIG_NETTLE) -static uint8_t * -qcrypto_cipher_munge_des_rfb_key(const uint8_t *key, - size_t nkey) -{ - uint8_t *ret = g_new0(uint8_t, nkey); - size_t i; - for (i = 0; i < nkey; i++) { - uint8_t r = key[i]; - r = (r & 0xf0) >> 4 | (r & 0x0f) << 4; - r = (r & 0xcc) >> 2 | (r & 0x33) << 2; - r = (r & 0xaa) >> 1 | (r & 0x55) << 1; - ret[i] = r; - } - return ret; -} -#endif /* CONFIG_GCRYPT || CONFIG_NETTLE */ - #ifdef CONFIG_GCRYPT #include "cipher-gcrypt.c.inc" #elif defined CONFIG_NETTLE diff --git a/qapi/crypto.json b/qapi/crypto.json index 7116ae9a46..6b3fadabac 100644 --- a/qapi/crypto.json +++ b/qapi/crypto.json @@ -66,7 +66,7 @@ # @aes-128: AES with 128 bit / 16 byte keys # @aes-192: AES with 192 bit / 24 byte keys # @aes-256: AES with 256 bit / 32 byte keys -# @des-rfb: RFB specific variant of single DES. Do not use except in VNC. +# @des: DES with 56 bit / 8 byte keys. Do not use except in VNC. # @3des: 3DES(EDE) with 192 bit / 24 byte keys (since 2.9) # @cast5-128: Cast5 with 128 bit / 16 byte keys # @serpent-128: Serpent with 128 bit / 16 byte keys @@ -80,7 +80,7 @@ { 'enum': 'QCryptoCipherAlgorithm', 'prefix': 'QCRYPTO_CIPHER_ALG', 'data': ['aes-128', 'aes-192', 'aes-256', - 'des-rfb', '3des', + 'des', '3des', 'cast5-128', 'serpent-128', 'serpent-192', 'serpent-256', 'twofish-128', 'twofish-192', 'twofish-256']} diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c index 7dca7b26e4..d9d9d078ff 100644 --- a/tests/unit/test-crypto-cipher.c +++ b/tests/unit/test-crypto-cipher.c @@ -155,28 +155,28 @@ static QCryptoCipherTestData test_data[] = { * in single AES block, and gives identical * ciphertext in ECB and CBC modes */ - .path = "/crypto/cipher/des-rfb-ecb-56-one-block", - .alg = QCRYPTO_CIPHER_ALG_DES_RFB, + .path = "/crypto/cipher/des-ecb-56-one-block", + .alg = QCRYPTO_CIPHER_ALG_DES, .mode = QCRYPTO_CIPHER_MODE_ECB, - .key = "0123456789abcdef", + .key = "80c4a2e691d5b3f7", .plaintext = "70617373776f7264", .ciphertext = "73fa80b66134e403", }, { /* See previous comment */ - .path = "/crypto/cipher/des-rfb-cbc-56-one-block", - .alg = QCRYPTO_CIPHER_ALG_DES_RFB, + .path = "/crypto/cipher/des-cbc-56-one-block", + .alg = QCRYPTO_CIPHER_ALG_DES, .mode = QCRYPTO_CIPHER_MODE_CBC, - .key = "0123456789abcdef", + .key = "80c4a2e691d5b3f7", .iv = "0000000000000000", .plaintext = "70617373776f7264", .ciphertext = "73fa80b66134e403", }, { - .path = "/crypto/cipher/des-rfb-ecb-56", - .alg = QCRYPTO_CIPHER_ALG_DES_RFB, + .path = "/crypto/cipher/des-ecb-56", + .alg = QCRYPTO_CIPHER_ALG_DES, .mode = QCRYPTO_CIPHER_MODE_ECB, - .key = "0123456789abcdef", + .key = "80c4a2e691d5b3f7", .plaintext = "6bc1bee22e409f96e93d7e117393172a" "ae2d8a571e03ac9c9eb76fac45af8e51" diff --git a/ui/vnc.c b/ui/vnc.c index 0e5fcb278f..af02522e84 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -2733,6 +2733,19 @@ static void authentication_failed(VncState *vs) vnc_client_error(vs); } +static void +vnc_munge_des_rfb_key(unsigned char *key, size_t nkey) +{ + size_t i; + for (i = 0; i < nkey; i++) { + uint8_t r = key[i]; + r = (r & 0xf0) >> 4 | (r & 0x0f) << 4; + r = (r & 0xcc) >> 2 | (r & 0x33) << 2; + r = (r & 0xaa) >> 1 | (r & 0x55) << 1; + key[i] = r; + } +} + static int protocol_client_auth_vnc(VncState *vs, uint8_t *data, size_t len) { unsigned char response[VNC_AUTH_CHALLENGE_SIZE]; @@ -2757,9 +2770,10 @@ static int protocol_client_auth_vnc(VncState *vs, uint8_t *data, size_t len) pwlen = strlen(vs->vd->password); for (i=0; i<sizeof(key); i++) key[i] = i<pwlen ? vs->vd->password[i] : 0; + vnc_munge_des_rfb_key(key, sizeof(key)); cipher = qcrypto_cipher_new( - QCRYPTO_CIPHER_ALG_DES_RFB, + QCRYPTO_CIPHER_ALG_DES, QCRYPTO_CIPHER_MODE_ECB, key, G_N_ELEMENTS(key), &err); @@ -4045,9 +4059,9 @@ void vnc_display_open(const char *id, Error **errp) goto fail; } if (!qcrypto_cipher_supports( - QCRYPTO_CIPHER_ALG_DES_RFB, QCRYPTO_CIPHER_MODE_ECB)) { + QCRYPTO_CIPHER_ALG_DES, QCRYPTO_CIPHER_MODE_ECB)) { error_setg(errp, - "Cipher backend does not support DES RFB algorithm"); + "Cipher backend does not support DES algorithm"); goto fail; } }
Currently the crypto layer exposes support for a 'des-rfb' algorithm which is just normal single-DES, with the bits in each key byte reversed. This special key munging is required by the RFB protocol password authentication mechanism. Since the crypto layer is generic shared code, it makes more sense to do the key byte munging in the VNC server code, and expose normal single-DES support. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- crypto/cipher-gcrypt.c.inc | 16 +++------------- crypto/cipher-nettle.c.inc | 26 +++++++++++--------------- crypto/cipher.c | 28 +++++----------------------- qapi/crypto.json | 4 ++-- tests/unit/test-crypto-cipher.c | 18 +++++++++--------- ui/vnc.c | 20 +++++++++++++++++--- 6 files changed, 47 insertions(+), 65 deletions(-)