New msgr2 crc and secure modes (msgr2.1) by idryomov · Pull Request #35078 · ceph/ceph

idryomov · 2020-05-14T21:35:55Z

This is based on @theanalyst's upstream-rgw-msgr-fixes branch for now (#34927). I'll rebase once it merges.

The secure mode uses AES-128-GCM with 96-bit nonces consisting of a 32-bit counter followed by a 64-bit salt. The counter is incremented after processing each frame, the salt is fixed for the duration of the session. Both are initialized from the session key generated during session negotiation, so the counter starts with essentially a random value. It is allowed to wrap, and, after 2**32 frames, it repeats, resulting in nonce reuse (the actual sequence numbers that the messenger works with are 64-bit, so the session continues on). Because of how GCM works, this completely breaks both confidentiality and integrity aspects of the secure mode. A single nonce reuse reveals the XOR of two plaintexts and almost completely reveals the subkey used for producing authentication tags. After a few nonces get used twice, all confidentiality and integrity goes out the window and the attacker can potentially encrypt-authenticate plaintext of their choice. We can't easily change the nonce format to extend the counter to 64 bits (and possibly XOR it with a longer salt). Instead, just remember the initial nonce and cut the session before it repeats, forcing renegotiation. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Reviewed-by: Radoslaw Zarzynski <rzarzyns@redhat.com> Reviewed-by: Sage Weil <sage@redhat.com>

As a AES-GCM IV, nonce_t is implicitly shared between server and client. Currently, if their endianness doesn't match, they are unable to communicate in secure mode because each gets its own idea of what the next nonce should be after the counter is incremented. Several RFCs state that the nonce counter should be BE, but since we use LE for everything on-disk and on-wire, make it LE. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Reviewed-by: Radoslaw Zarzynski <rzarzyns@redhat.com> Reviewed-by: Sage Weil <sage@redhat.com>

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> Reviewed-by: Casey Bodley <cbodley@redhat.com> (cherry picked from commit d8dd5e513c0c62bbd7d3044d7e2eddcd897bd400)

As per Robin's comments and S3 spec Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

S3 GetObject permits overriding response header values, but those inputs need to be validated to insure only characters that are valid in an HTTP header value are present. Credit: Initial vulnerability discovery by William Bowling (@wcbowling) Credit: Further vulnerability discovery by Robin H. Johnson <rjohnson@digitalocean.com> Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>

Provide an iterator-like interface as initializer lists cannot be formed dynamically. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Add asserts to avoid bugs like the one fixed in 1a975fb ("msg/async: fix unnecessary 4 kB allocation in secure mode."). Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

It is unused and doesn't make much sense in TxHandler. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

tchaikov · 2020-05-15T03:09:30Z

@cyx1231st just in case.

idryomov · 2020-05-15T08:27:30Z

cc @jdurgin @liewegas

idryomov · 2020-05-15T08:32:48Z

jenkins test make check

src/test/msgr/test_frames_v2.cc

rzarzynski

Comments from 1st part of the review. So far looks good.

src/msg/async/ProtocolV2.cc

src/msg/async/crypto_onwire.cc

src/msg/async/crypto_onwire.h

src/msg/async/frames_v2.cc

src/msg/async/frames_v2.h

src/msg/async/ProtocolV2.cc

src/msg/async/frames_v2.cc

src/msg/async/ProtocolV2.cc

src/msg/async/ProtocolV2.h

src/msg/async/frames_v2.h

rzarzynski

In progress. Generally looks good so far!

src/msg/async/frames_v2.h

src/msg/async/frames_v2.cc

src/msg/async/frames_v2.h

rzarzynski · 2020-05-28T18:57:06Z

src/msg/async/frames_v2.h

-  FrameAssembler(const ceph::crypto::onwire::rxtx_t* crypto)
-      : m_crypto(crypto) {}
+  FrameAssembler(const ceph::crypto::onwire::rxtx_t* crypto, bool is_r1)
+      : m_crypto(crypto), m_is_r1(is_r1) {}


nit: one per line.

src/msg/async/frames_v2.cc

src/msg/async/frames_v2.h

rzarzynski

The changes generally look good! Some nits and questions.
I'm moving forward to a brief performance comparison.

src/msg/async/crypto_onwire.cc

src/test/msgr/test_frames_v2.cc

rzarzynski · 2020-06-01T13:17:40Z

src/test/msgr/test_frames_v2.cc

+  return bl;
+}
+
+bool disassemble_frame(FrameAssembler* frame_asm, bufferlist* frame_bl,


nit: one param per line.

src/test/msgr/test_frames_v2.cc

src/msg/async/ProtocolV2.cc

src/include/msgr.h

rzarzynski · 2020-06-03T10:40:58Z

I made a really brief performance evaluation. For small chunk we don't need to worry; for bigger ones there is a nice improvement in latency.

I think we can move forward.

OpenSSL supports in-place decryption so we can avoid allocating potentially multi-megabyte and strictly aligned buffer for each decryption operation. ProtocolV2 actually gets the alignment wrong: after read_frame_segment() allocates with cur_rx_desc.alignment, handle_read_frame_segment() effectively replaces that with segment_t::DEFAULT_ALIGNMENT. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Use it in ProtocolV2.h and later in unit tests. While at it, drop the unused len struct. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov · 2020-06-12T16:33:12Z

@rzarzynski Implemented reserving before appending and addressed most of the style nits (including using mutable references, so the diff is quite large but mostly mechanical). reserve calls is pretty much the only functional change, any testing you have done should still be valid.

idryomov · 2020-06-12T16:48:08Z

I think we can move forward.

Since I haven't received any other comments on the wire format itself, I'm taking off the RFC.

Start separating frame assembly and disassembly code from frame sending, receiving and handling code, so that assembly and disassembly pieces can be unit tested and hopefully also shared between different messengers (e.g. crimson). This commit factors out the assembly code from Frame. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Factor out the disassembly code from ProtocolV2 and switch ProtocolV2 to FrameAssembler. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

l_msgr_recv_bytes calculation was never updated from msgr1. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

In preparation for msgr2,1, rename epilogue structs: epilogue_plain_block_t to epilogue_crc_rev0_block_t and epilogue_secure_block_t to epilogue_secure_rev0_block_t (rev0 stands for revision 0). Also, get rid of size constants that just disguise the struct type. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Clarify that the frame can be aborted at any point after the preamble and the first segment are put on the wire. When that happens, the remaining segments (including the data segment) may be filled with zeros. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov · 2020-06-14T12:54:43Z

@rzarzynski One more fixup: moved the assert into get_buffer().

Implement msgr2.1-crc and msgr2.1-secure modes. Issues with existing msgr2.0-crc and msgr2.0-secure modes and their resolution will be described in doc/dev/msgr2.rst. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Move to a 64-bit counter to avoid wrapping and having to reset the session before the counter repeats. This is in line with NIST Recommendation for GCM [1]: "... this Recommendation suggests, but does not require, that the leading (i.e., leftmost) 32 bits of the IV hold the fixed field; and that the trailing (i.e., rightmost) 64 bits hold the invocation field." See commit bb61e6a ("msg/async/ProtocolV2: avoid AES-GCM nonce reuse vulnerabilities"). [1] https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

In both msgr2.0 and msgr2.1, segments can be empty. In msgr2.1, epilogue can be empty as well. Handle both by calling the respective handler function directly instead of allocating a buffer::ptr_node for an empty buffer and passing that through READ[_RXBUF]. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

We aren't interested in peer_required_features anywhere outside _handle_peer_banner_payload() -- once we know there is no mismatch, it's all about peer_supported_features. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Use msgr2.1 if the peer supports it and fall back to msgr2.0 otherwise. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

src/msg/async/ProtocolV2.cc

src/msg/async/frames_v2.cc

rzarzynski · 2020-06-17T23:47:17Z

src/msg/async/frames_v2.cc

+  }
+
+  preamble_block_t preamble;
+  fill_preamble(tag, preamble);


It looks this is the sole caller of fill_preamble(), so returning preamble_block_t seems pretty straightforward. Anyway, this is really a minor as the method is internal to FrameAssembler.

src/msg/async/frames_v2.cc

src/msg/async/frames_v2.h

rzarzynski

Looks good!

src/msg/async/frames_v2.cc

rzarzynski · 2020-06-18T14:41:05Z

src/msg/async/frames_v2.h

+  // more than one segment (i.e. at least one of second to fourth
+  // segments is not empty).  In crc mode, it stores crcs for
+  // second to fourh segments; the preamble and the first segment
+  // are covered by their own crcs.  In secure mode, the epilogue


Yeah, the stuff got more complex. It might be worth adding here a single sentence about the krbd-driven rationale for having the first segment available (we have something like that in msgr2.rst but the comment would be much closer).
Still, not a blocker.

While it is true that this change is driven by the kernel client, we may want to resurrect the option of receiving into preallocated buffers in userspace at some point too. Having the header crc on the header itself is what msgr1 did (i.e. this goes back to the dawn of ceph), so I don't think the rationale needs stressing here. It's really just a regression fix.

tchaikov · 2020-06-20T11:52:37Z

cephadm tests failed due to accessing docker.io instead of via a mirror registry.

http://pulpito.ceph.com/kchai-2020-06-19_07:01:33-upgrade-wip-kefu-testing-2020-06-19-1102-distro-basic-smithi/

idryomov · 2020-06-30T09:30:59Z

A critical follow-on fix for the issue exposed by msgr2.1: #35816.

idryomov and others added 8 commits May 6, 2020 09:54

rgw: reject unauthenticated response-header actions

7dbc3f7

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> Reviewed-by: Casey Bodley <cbodley@redhat.com> (cherry picked from commit d8dd5e513c0c62bbd7d3044d7e2eddcd897bd400)

rgw: EPERM to ERR_INVALID_REQUEST

c9b043b

As per Robin's comments and S3 spec Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

msg/async/crypto_onwire: allow dynamic reset_tx_handler() sequences

1fc5cc2

Provide an iterator-like interface as initializer lists cannot be formed dynamically. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/crypto_onwire: add asserts for reset_tx_handler() reservation

4894e59

Add asserts to avoid bugs like the one fixed in 1a975fb ("msg/async: fix unnecessary 4 kB allocation in secure mode."). Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/crypto_onwire: remove TxHandler::calculate_segment_size()

b3e39b1

It is unused and doesn't make much sense in TxHandler. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov requested a review from a team as a code owner May 14, 2020 21:35

idryomov force-pushed the wip-msgr21 branch from 6163405 to 9b093fe Compare May 15, 2020 00:09

idryomov added feature messenger Issues involving one of the Ceph messenger implementations labels May 15, 2020

tchaikov requested a review from cyx1231st May 15, 2020 03:08

idryomov requested a review from rzarzynski May 15, 2020 08:26

rzarzynski reviewed May 25, 2020

View reviewed changes

src/test/msgr/test_frames_v2.cc Outdated Show resolved Hide resolved

rzarzynski mentioned this pull request May 25, 2020

msgr, tests: bring unit testing for the crypto stream handlers #35238

Closed

3 tasks

rzarzynski reviewed May 26, 2020

View reviewed changes

rzarzynski reviewed May 27, 2020

View reviewed changes

rzarzynski reviewed May 28, 2020

View reviewed changes

rzarzynski reviewed Jun 1, 2020

View reviewed changes

idryomov added 4 commits June 12, 2020 14:37

msg/async/ProtocolV2: adjust some douts

e1d1f61

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/ProtocolV2: session_stream_handlers doesn't need to be public

6b6d405

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/frames_v2: make rx_segments_t global

c081f3c

Use it in ProtocolV2.h and later in unit tests. While at it, drop the unused len struct. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov force-pushed the wip-msgr21 branch from 9b093fe to 04f2f34 Compare June 12, 2020 14:25

idryomov changed the title ~~[RFC] New msgr2 crc and secure modes (msgr2.1)~~ New msgr2 crc and secure modes (msgr2.1) Jun 12, 2020

idryomov added 5 commits June 14, 2020 11:56

msg/async/ProtocolV2: switch to FrameAssembler

b9e0cfe

Factor out the disassembly code from ProtocolV2 and switch ProtocolV2 to FrameAssembler. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/ProtocolV2: fix l_msgr_recv_bytes calculation

dcf30f5

l_msgr_recv_bytes calculation was never updated from msgr1. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov force-pushed the wip-msgr21 branch from 04f2f34 to 84663bd Compare June 14, 2020 12:53

tchaikov added the needs-qa label Jun 16, 2020

idryomov added 7 commits June 17, 2020 21:56

msg/async/frames_v2: implement msgr2.1 wire format

2966b2a

Implement msgr2.1-crc and msgr2.1-secure modes. Issues with existing msgr2.0-crc and msgr2.0-secure modes and their resolution will be described in doc/dev/msgr2.rst. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/frames_v2: add initial unit tests

0b3f4c6

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

msg/async/ProtocolV2: add msgr2.1 feature bit

afc2886

Use msgr2.1 if the peer supports it and fall back to msgr2.0 otherwise. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

doc/dev/msgr2: fix inconsistencies and update for msgr2.1

5eea038

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov force-pushed the wip-msgr21 branch from 84663bd to 5eea038 Compare June 17, 2020 20:20

tchaikov added the wip-kefu-testing label Jun 18, 2020

rzarzynski reviewed Jun 18, 2020

View reviewed changes

rzarzynski approved these changes Jun 18, 2020

View reviewed changes

tchaikov merged commit 4536a09 into ceph:master Jun 20, 2020

rzarzynski mentioned this pull request Jun 22, 2020

common, msgr/async: bufferlist::claim_append() doesn't require intermediaries anymore #35716

Merged

3 tasks

This was referenced Jun 23, 2020

octopus: New msgr2 crc and secure modes (msgr2.1) #35720

Merged

nautilus: New msgr2 crc and secure modes (msgr2.1) #35733

Merged

idryomov deleted the wip-msgr21 branch July 8, 2020 11:52

Conversation

idryomov commented May 14, 2020

Uh oh!

tchaikov commented May 15, 2020

Uh oh!

idryomov commented May 15, 2020

Uh oh!

idryomov commented May 15, 2020

Uh oh!

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzarzynski May 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzarzynski Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzarzynski commented Jun 3, 2020

Uh oh!

idryomov commented Jun 12, 2020

Uh oh!

idryomov commented Jun 12, 2020

Uh oh!

idryomov commented Jun 14, 2020

Uh oh!

Uh oh!

Uh oh!

rzarzynski Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!