Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Tue, Oct 21, 2014 at 10:35:06AM +0100, Daniel P. Berrange wrote: > On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote: > > On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote: > > > I cannot comment on whether the proposed STARTTLS command is at the > > > correct > > > stage of the NBD protocol. If there is a protocol description for NBD, I > > > can have a look. > > > > To make this discussion in that regard a bit easier, I've committed the > > proposed spec to a separate branch: > > > > https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt > > So, if I'm understanding correctly the new style "fixed" handshake > currently works like this: > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x1 > > And the client acknowledges with a 32 bits of and first bit set > > - C: 0x1 > > > Now the client has to request one or more options, of which the > NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the > first thing the client sends next. No, on the contrary. NBD_OPT_EXPORT_NAME _must_ be the very last thing the client sends if the client wants to actually select an export to be used. This is because for historical reasons, NBD_OPT_EXPORT_NAME can't get an NBD_REP_ACK, but must immediately introduce the next phase of the protocol (where data is flowing across the channel). It should maybe be renamed to something like "NBD_OPT_SELECT_AND_GO" or some such, but I'm not sure that makes much sense. > So it would send > > - C: 0x49484156454F5054 > - C: 0x1 > - C: 0xa (length of name) > - C: "volumename" > > > > You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to > work we would need to update the language to say that the NBD_OPT_STARTTLS > must be the first option that the client sends to the server, because we > want the option name to transmit in ciphertext. Yes, my proposed protocol allows that. > So the new protocol startup would be > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x1 > > And the client acknowledges with a 32 bits of zero > > - C: 0x1 > > Client requests TLS > > - C: 0x49484156454F5054 > - C: 0x5 > > Server acknowledges TLS: > > - S: 0x3e889045565a9 > - S: 0x5 > - S: 0x1 (REP_ACK) > - S: 0x0 > > Now the TLS handshake is performed by client + server > > > > The client can now set other options using the secure > channel, of which the NBD_OPT_EXPORT_NAME (0x1) option > is mandatory, so it will be the first thing the client (last thing) > sends next. So they would send > > - C: "0x49484156454F5054" > - C: 0x1 > - C: 0xa (length of name) > - C: "volumename" > > etc... > > > This is secure when both client and server want to use TLS. > > This is secure when the client wants TLS and the server does > not want TLS, because the server will reject TLS and the client > will then close the connection. > > My concern is the case where the client does not want TLS but > the server does want TLS. In this case the client will immediately > send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving > the server any chance to reject the use of a clear text channel. In that case (if the client doesn't want TLS and also doesn't want to negotiate anything else apart from a name), the server should disconnect (anything else will fail to communicate with older clients). > This problem is inherant to the approach of using the NBD protocol > options to negotiate TLS. True. > One way I see to solve that insecurity, would be for the server > to make use of one of the extra reserved bits in the very first > message it sends. ie we need to negotiate TLS immediately after the > version number / magic acknowledgment, before we actually start > the main body of the protocol > > ie, with the new style fixed handshake the server should send > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x2 That would be 0x4, because 0x2 is already used for something else (yes, yes, details). > And the client acknowledges with a 32bits and first two bits set > > - C: 0x2 > > The questionmark here is what happens when the client sees the > second bit of reserved field set ? > > The protocol docs merely say > > "S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to >signal fixed newstyle (see below))" > > But don't mention what clients should do upon seeing unknown bits > set in the server's message ? If the client doesn't send the bit you proposed, then the server would know it doesn't understand the TLS variant of the protocol and would then either fall back to unencrypted or drop the connection (depending on policy). I'm not sure using a separate bit for this is necessary, though. The server is supposed to send NBD_REP_ERR_UNSUP for messages it doesn't understand, so a client that talk
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote: > On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote: > > I cannot comment on whether the proposed STARTTLS command is at the correct > > stage of the NBD protocol. If there is a protocol description for NBD, I > > can have a look. > > To make this discussion in that regard a bit easier, I've committed the > proposed spec to a separate branch: > > https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt So, if I'm understanding correctly the new style "fixed" handshake currently works like this: Server starts transmission with version info - S: "NBDMAGIC" - S: 0x49484156454F5054 - S: 0x1 And the client acknowledges with a 32 bits of and first bit set - C: 0x1 Now the client has to request one or more options, of which the NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the first thing the client sends next . So it would send - C: 0x49484156454F5054 - C: 0x1 - C: 0xa (length of name) - C: "volumename" You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to work we would need to update the language to say that the NBD_OPT_STARTTLS must be the first option that the client sends to the server, because we want the option name to transmit in ciphertext. So the new protocol startup would be Server starts transmission with version info - S: "NBDMAGIC" - S: 0x49484156454F5054 - S: 0x1 And the client acknowledges with a 32 bits of zero - C: 0x1 Client requests TLS - C: 0x49484156454F5054 - C: 0x5 Server acknowledges TLS: - S: 0x3e889045565a9 - S: 0x5 - S: 0x1 (REP_ACK) - S: 0x0 Now the TLS handshake is performed by client + server The client can now set other options using the secure channel, of which the NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the first thing the client sends next . So they would send - C: "0x49484156454F5054" - C: 0x1 - C: 0xa (length of name) - C: "volumename" ...etc... This is secure when both client and server want to use TLS. This is secure when the client wants TLS and the server does not want TLS, because the server will reject TLS and the client will then close the connection. My concern is the case where the client does not want TLS but the server does want TLS. In this case the client will immediately send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving the server any chance to reject the use of a clear text channel. This problem is inherant to the approach of using the NBD protocol options to negotiate TLS. One way I see to solve that insecurity, would be for the server to make use of one of the extra reserved bits in the very first message it sends. ie we need to negotiate TLS immediately after the version number / magic acknowledgment, before we actually start the main body of the protocol ie, with the new style fixed handshake the server should send Server starts transmission with version info - S: "NBDMAGIC" - S: 0x49484156454F5054 - S: 0x2 And the client acknowledges with a 32bits and first two bits set - C: 0x2 The questionmark here is what happens when the client sees the second bit of reserved field set ? The protocol docs merely say "S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to signal fixed newstyle (see below))" But don't mention what clients should do upon seeing unknown bits set in the server's message ? If clients ignore unknown bits, then we have the same problem where a client can ignore the TLS bit and start sending option name in the clear before the server rejects the session. If clients abort (close connection) on seeing unknown bits, then we are good. A 3rd alternative is to actually use a separate magic number, which should guarantee the client would immediately close upon seeing a magic number which indicated TLS is required. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote: > I cannot comment on whether the proposed STARTTLS command is at the correct > stage of the NBD protocol. If there is a protocol description for NBD, I > can have a look. To make this discussion in that regard a bit easier, I've committed the proposed spec to a separate branch: https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt Thanks, -- It is easy to love a country that is famous for chocolate and beer -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote: > On 10/20/2014 01:51 PM, Markus Armbruster wrote: > >Furthermore, STARTTLS is vulnerable to active attacks: if you can get > >between the peers, you can make them fall back to unencrypted silently. > >How do you plan to guard against that? > > The usual way to deal with this is to use different syntax for > TLS-enabled and non-TLS addresses (e.g., https:// and http://). > With a TLS address, the client must enforce that only TLS-enabled > connections are possible. STARTTLS isn't the problem here, it's > just an accident of history that many STARTTLS client > implementations do not require a TLS handshake before proceeding. > > I cannot comment on whether the proposed STARTTLS command is at the > correct stage of the NBD protocol. If there is a protocol > description for NBD, I can have a look. Two actually :-) Both are covered here: http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt I believe that the proposed changes only cover the new style protocol. There's no common syntax for nbd URLs that I'm aware of. At least, both qemu & guestfish have nbd:... strings that they can parse, but both have a completely different syntax. But we could still have a client-side indication (flag or nbds:..) to say that we want to force TLS. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On 10/20/2014 01:51 PM, Markus Armbruster wrote: Furthermore, STARTTLS is vulnerable to active attacks: if you can get between the peers, you can make them fall back to unencrypted silently. How do you plan to guard against that? The usual way to deal with this is to use different syntax for TLS-enabled and non-TLS addresses (e.g., https:// and http://). With a TLS address, the client must enforce that only TLS-enabled connections are possible. STARTTLS isn't the problem here, it's just an accident of history that many STARTTLS client implementations do not require a TLS handshake before proceeding. I cannot comment on whether the proposed STARTTLS command is at the correct stage of the NBD protocol. If there is a protocol description for NBD, I can have a look. -- Florian Weimer / Red Hat Product Security
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Mon, Oct 20, 2014 at 01:51:43PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi writes: > > > On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote: > >> On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote: > >> > On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote: > >> > > Hi all, > >> > > > >> > > (added rjones from nbdkit fame -- hi there) > >> > > >> > [I'm happy to implement whatever you come up with, but I've added > >> > Florian Weimer to CC who is part of Red Hat's product security group] > >> > > >> > > So I think the following would make sense to allow TLS in NBD. > >> > > > >> > > This would extend the newstyle negotiation by adding two options (i.e., > >> > > client requests), one server reply, and one server error as well as > >> > > extend one existing reply, in the following manner: > >> > > > >> > > - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. > >> > > The > >> > > former would be used to verify if the server will do TLS for a given > >> > > export: > >> > > > >> > > C: NBD_OPT_PEEK_EXPORT > >> > > S: NBD_REP_SERVER, with an extra field after the export name > >> > > containing flags that describe the export (R/O vs R/W state, > >> > > whether TLS is allowed and/or required). > >> > >> IMHO the server should never provide *any* information about the exported > >> volume(s) until the TLS layer has been fully setup. ie we shouldn't only > >> think about the actual block data transfers, we should protect the entire > >> NBD protocol even metadata related operations. > > > > This makes sense. > > Seconded. > > > TLS is about the transport, not about a particular NBD export. The only > > thing that should be communicated is STARTTLS. > > Furthermore, STARTTLS is vulnerable to active attacks: if you can get > between the peers, you can make them fall back to unencrypted silently. > How do you plan to guard against that? Well the use of a STARTTLS message at a protocol level isn't vulnerable per-se, rather it is the handling of it that matters. The key is what happens if the server wants TLS and the client does not send a STARTTLS message. If the server happily carries on with plain text that's bad. If the server closes any connection that attempts to skip STARTTLS, that's fine. Likewise if the client wants TLS and the server claims to not do TLS, then the client should close the connection and not carry on. This avoids the MITM downgrade problem. So from the POV of QEMU / QEMU-NBD I'd expect us to have a CLI option tls=on|off and if the client / server are configured differently then it would be a hard failure, never any negotiated fallback to plain text if one requests TLS and the other doesn't. If QEMU relies on the CLI option, then technically we do not need any NBD protocol level changes at all. A standard TLS handshake could be started the moment the TCP connection is established. Only once the TLS handshake completes would the NBD protocol start running. The real / main benefit of having a STARTTLS message would be to give better error reporting for clients not attempting TLS. eg so they could report a clear "This server requires TLS" error instead of just seeing unintelligible data from the NBD server and no clue that it is a TLS handshake. This is how the VNC integration works at least. The VNC server advertizes that it requires the TLS auth protocol extension. If the VNC client does not support this, the server will drop the connection and the VNC client can at least report to the user that the server requested use of TLS. The key is that no data or metadata that is in any way related to remote desktop (or NBD volume) is exchanged between server/client until after the TLS auth protocol completes. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] spec, RFC: TLS support for NBD
Stefan Hajnoczi writes: > On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote: >> On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote: >> > On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote: >> > > Hi all, >> > > >> > > (added rjones from nbdkit fame -- hi there) >> > >> > [I'm happy to implement whatever you come up with, but I've added >> > Florian Weimer to CC who is part of Red Hat's product security group] >> > >> > > So I think the following would make sense to allow TLS in NBD. >> > > >> > > This would extend the newstyle negotiation by adding two options (i.e., >> > > client requests), one server reply, and one server error as well as >> > > extend one existing reply, in the following manner: >> > > >> > > - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The >> > > former would be used to verify if the server will do TLS for a given >> > > export: >> > > >> > > C: NBD_OPT_PEEK_EXPORT >> > > S: NBD_REP_SERVER, with an extra field after the export name >> > > containing flags that describe the export (R/O vs R/W state, >> > > whether TLS is allowed and/or required). >> >> IMHO the server should never provide *any* information about the exported >> volume(s) until the TLS layer has been fully setup. ie we shouldn't only >> think about the actual block data transfers, we should protect the entire >> NBD protocol even metadata related operations. > > This makes sense. Seconded. > TLS is about the transport, not about a particular NBD export. The only > thing that should be communicated is STARTTLS. Furthermore, STARTTLS is vulnerable to active attacks: if you can get between the peers, you can make them fall back to unencrypted silently. How do you plan to guard against that? See also https://www.agwa.name/blog/post/starttls_considered_harmful
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote: > On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote: > > On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote: > > > Hi all, > > > > > > (added rjones from nbdkit fame -- hi there) > > > > [I'm happy to implement whatever you come up with, but I've added > > Florian Weimer to CC who is part of Red Hat's product security group] > > > > > So I think the following would make sense to allow TLS in NBD. > > > > > > This would extend the newstyle negotiation by adding two options (i.e., > > > client requests), one server reply, and one server error as well as > > > extend one existing reply, in the following manner: > > > > > > - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The > > > former would be used to verify if the server will do TLS for a given > > > export: > > > > > > C: NBD_OPT_PEEK_EXPORT > > > S: NBD_REP_SERVER, with an extra field after the export name > > > containing flags that describe the export (R/O vs R/W state, > > > whether TLS is allowed and/or required). > > IMHO the server should never provide *any* information about the exported > volume(s) until the TLS layer has been fully setup. ie we shouldn't only > think about the actual block data transfers, we should protect the entire > NBD protocol even metadata related operations. This makes sense. TLS is about the transport, not about a particular NBD export. The only thing that should be communicated is STARTTLS. Stefan pgpXDFIeyNWjU.pgp Description: PGP signature
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote: > On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote: > > Hi all, > > > > (added rjones from nbdkit fame -- hi there) > > [I'm happy to implement whatever you come up with, but I've added > Florian Weimer to CC who is part of Red Hat's product security group] > > > So I think the following would make sense to allow TLS in NBD. > > > > This would extend the newstyle negotiation by adding two options (i.e., > > client requests), one server reply, and one server error as well as > > extend one existing reply, in the following manner: > > > > - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The > > former would be used to verify if the server will do TLS for a given > > export: > > > > C: NBD_OPT_PEEK_EXPORT > > S: NBD_REP_SERVER, with an extra field after the export name > > containing flags that describe the export (R/O vs R/W state, > > whether TLS is allowed and/or required). IMHO the server should never provide *any* information about the exported volume(s) until the TLS layer has been fully setup. ie we shouldn't only think about the actual block data transfers, we should protect the entire NBD protocol even metadata related operations. > > If the server indicates that TLS is allowed, the client may now issue > > NBD_OPT_STARTTLS: > > > > C: NBD_OPT_STARTTLS > > S: NBD_REP_STARTTLS # or NBD_REP_ERR_POLICY, if unwilling > > C: > > > > Once the TLS handshake has completed, negotiation should continue over > > the secure channel. The client should initiate that by sending an > > NBD_OPT_* message. > > > > - The server may reply to any and all negotiation request with > > NBD_REP_ERR_TLS_REQD if it does not want to do anything without TLS. > > However, if at least one export is supported without encryption, the > > server must not in any case use this reply. > > > > There is no command to "exit" TLS again. I don't think that makes sense, > > but I could be persuaded otherwise with sound technical arguments. > > > > Thoughts? > > > > (full spec (with numbers etc) exists as an (uncommitted) diff to > > doc/proto.txt on my laptop, ...) Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] spec, RFC: TLS support for NBD
On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote: > Hi all, > > (added rjones from nbdkit fame -- hi there) [I'm happy to implement whatever you come up with, but I've added Florian Weimer to CC who is part of Red Hat's product security group] > So I think the following would make sense to allow TLS in NBD. > > This would extend the newstyle negotiation by adding two options (i.e., > client requests), one server reply, and one server error as well as > extend one existing reply, in the following manner: > > - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The > former would be used to verify if the server will do TLS for a given > export: > > C: NBD_OPT_PEEK_EXPORT > S: NBD_REP_SERVER, with an extra field after the export name > containing flags that describe the export (R/O vs R/W state, > whether TLS is allowed and/or required). > > If the server indicates that TLS is allowed, the client may now issue > NBD_OPT_STARTTLS: > > C: NBD_OPT_STARTTLS > S: NBD_REP_STARTTLS # or NBD_REP_ERR_POLICY, if unwilling > C: > > Once the TLS handshake has completed, negotiation should continue over > the secure channel. The client should initiate that by sending an > NBD_OPT_* message. > > - The server may reply to any and all negotiation request with > NBD_REP_ERR_TLS_REQD if it does not want to do anything without TLS. > However, if at least one export is supported without encryption, the > server must not in any case use this reply. > > There is no command to "exit" TLS again. I don't think that makes sense, > but I could be persuaded otherwise with sound technical arguments. > > Thoughts? > > (full spec (with numbers etc) exists as an (uncommitted) diff to > doc/proto.txt on my laptop, ...) > > -- > It is easy to love a country that is famous for chocolate and beer > > -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
[Qemu-devel] spec, RFC: TLS support for NBD
Hi all, (added rjones from nbdkit fame -- hi there) So I think the following would make sense to allow TLS in NBD. This would extend the newstyle negotiation by adding two options (i.e., client requests), one server reply, and one server error as well as extend one existing reply, in the following manner: - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The former would be used to verify if the server will do TLS for a given export: C: NBD_OPT_PEEK_EXPORT S: NBD_REP_SERVER, with an extra field after the export name containing flags that describe the export (R/O vs R/W state, whether TLS is allowed and/or required). If the server indicates that TLS is allowed, the client may now issue NBD_OPT_STARTTLS: C: NBD_OPT_STARTTLS S: NBD_REP_STARTTLS # or NBD_REP_ERR_POLICY, if unwilling C: Once the TLS handshake has completed, negotiation should continue over the secure channel. The client should initiate that by sending an NBD_OPT_* message. - The server may reply to any and all negotiation request with NBD_REP_ERR_TLS_REQD if it does not want to do anything without TLS. However, if at least one export is supported without encryption, the server must not in any case use this reply. There is no command to "exit" TLS again. I don't think that makes sense, but I could be persuaded otherwise with sound technical arguments. Thoughts? (full spec (with numbers etc) exists as an (uncommitted) diff to doc/proto.txt on my laptop, ...) -- It is easy to love a country that is famous for chocolate and beer -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26 signature.asc Description: Digital signature
