fdad35ef | 22-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: CVE-2017-15119 Reject options larger than 32M
The NBD spec gives us permission to abruptly disconnect on clients that send outrageously large option requests, rather than having to spend
nbd/server: CVE-2017-15119 Reject options larger than 32M
The NBD spec gives us permission to abruptly disconnect on clients that send outrageously large option requests, rather than having to spend the time reading to the end of the option. No real option request requires that much data anyways; and meanwhile, we already have the practice of abruptly dropping the connection on any client that sends NBD_CMD_WRITE with a payload larger than 32M.
For comparison, nbdkit drops the connection on any request with more than 4096 bytes; however, that limit is probably too low (as the NBD spec states an export name can theoretically be up to 4096 bytes, which means a valid NBD_OPT_INFO could be even longer) - even if qemu doesn't permit exports longer than 256 bytes.
It could be argued that a malicious client trying to get us to read nearly 4G of data on a bad request is a form of denial of service. In particular, if the server requires TLS, but a client that does not know the TLS credentials sends any option (other than NBD_OPT_STARTTLS or NBD_OPT_EXPORT_NAME) with a stated payload of nearly 4G, then the server was keeping the connection alive trying to read all the payload, tying up resources that it would rather be spending on a client that can get past the TLS handshake. Hence, this warranted a CVE.
Present since at least 2.5 when handling known options, and made worse in 2.6 when fixing support for NBD_FLAG_C_FIXED_NEWSTYLE to handle unknown options.
CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
fed5f8f8 | 15-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Fix error reporting for bad requests
The NBD spec says an attempt to NBD_CMD_TRIM on a read-only export should fail with EPERM, as a trim has the potential to change disk contents, but w
nbd/server: Fix error reporting for bad requests
The NBD spec says an attempt to NBD_CMD_TRIM on a read-only export should fail with EPERM, as a trim has the potential to change disk contents, but we were relying on the block layer to catch that for us, which might not always give the right error (and even if it does, it does not let us pass back a sane message for structured replies).
The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of bounds should fail with ENOSPC, not EINVAL.
Our check for u64 offset + u32 length wraparound up front is pointless; nothing uses offset until after the second round of sanity checks, and we can just as easily ensure there is no wraparound by checking whether offset is in bounds (since a disk size cannot exceed off_t which is 63 bits, adding a 32-bit number for a valid offset can't overflow). Bonus: dropping the up-front check lets us keep the connection alive after NBD_CMD_WRITE, whereas before we would drop the connection (of course, any client sending a packet that would trigger the failure is already buggy, so it's also okay to drop the connection, but better quality-of-implementation never hurts).
Solve all of these issues by some code motion and improved request validation.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171115213557.3548-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
01b05c66 | 13-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/client: Don't hard-disconnect on ESHUTDOWN from server
The NBD spec says that a server may fail any transmission request with ESHUTDOWN when it is apparent that no further request from the clien
nbd/client: Don't hard-disconnect on ESHUTDOWN from server
The NBD spec says that a server may fail any transmission request with ESHUTDOWN when it is apparent that no further request from the client can be successfully honored. The client is supposed to then initiate a soft shutdown (wait for all remaining in-flight requests to be answered, then send NBD_CMD_DISC). However, since qemu's server never uses ESHUTDOWN errors, this code was mostly untested since its introduction in commit b6f5d3b5.
More recently, I learned that nbdkit as the NBD server is able to send ESHUTDOWN errors, so I finally tested this code, and noticed that our client was special-casing ESHUTDOWN to cause a hard shutdown (immediate disconnect, with no NBD_CMD_DISC), but only if the server sends this error as a simple reply. Further investigation found that commit d2febedb introduced a regression where structured replies behave differently than simple replies - but that the structured reply behavior is more in line with the spec (even if we still lack code in nbd-client.c to properly quit sending further requests). So this patch reverts the portion of b6f5d3b5 that introduced an improper hard-disconnect special-case at the lower level, and leaves the future enhancement of a nicer soft-disconnect at the higher level for another day.
CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171113194857.13933-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
cb6b1a3f | 13-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/client: Use error_prepend() correctly
When using error prepend(), it is necessary to end with a space in the format string; otherwise, messages come out incorrectly, such as when connecting to a
nbd/client: Use error_prepend() correctly
When using error prepend(), it is necessary to end with a space in the format string; otherwise, messages come out incorrectly, such as when connecting to a socket that hangs up immediately:
can't open device nbd://localhost:10809/: Failed to read dataUnexpected end-of-file before all bytes were read
Originally botched in commit e44ed99d, then several more instances added in the meantime.
Pre-existing and not fixed here: we are inconsistent on capitalization; some of our messages start with lower case, and others start with upper, although the use of error_prepend() is much nicer to read when all fragments consistently start with lower.
CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171113152424.25381-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
ef8c887e | 08-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Fix structured read of length 0
The NBD spec was recently clarified to state that a read of length 0 should not be attempted by a compliant client; but that a server must still handle it
nbd/server: Fix structured read of length 0
The NBD spec was recently clarified to state that a read of length 0 should not be attempted by a compliant client; but that a server must still handle it correctly in an unspecified manner (that is, either a successful no-op or an error reply, but not a crash) [1]. However, it also implies that NBD_REPLY_TYPE_OFFSET_DATA must have a non-zero payload length, but our existing code was replying with a chunk that a picky client could reject as invalid because it was missing a payload (our own client implementation was recently patched to be that picky, after first fixing it to not send 0-length requests).
We are already doing successful no-ops for 0-length writes and for non-structured reads; so for consistency, we want structured reply reads to also be a no-op. The easiest way to do this is to return a NBD_REPLY_TYPE_NONE chunk; this is best done via a new helper function (especially since future patches for other structured replies may benefit from using the same helper).
[1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-8-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
efdc0c10 | 08-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd: Fix struct name for structured reads
A closer read of the NBD spec shows that a structured reply chunk for a hole is not quite identical to the prefix of a data chunk, because the hole has to a
nbd: Fix struct name for structured reads
A closer read of the NBD spec shows that a structured reply chunk for a hole is not quite identical to the prefix of a data chunk, because the hole has to also send a 32-bit size field. Although we do not yet send holes, we should fix the misleading information in our header and make it easier for a future patch to support sparse reads. Messed up in commit bae245d1.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-5-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
079d3266 | 08-Nov-2017 |
Eric Blake <eblake@redhat.com> |
nbd/client: Nicer trace of structured reply
It's useful to know which structured reply chunk is being processed. Missed in commit d2febedb.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id:
nbd/client: Nicer trace of structured reply
It's useful to know which structured reply chunk is being processed. Missed in commit d2febedb.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-4-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
46321d6b | 01-Nov-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/server: fix nbd_negotiate_handle_info
namelen should be here, length is unrelated, and always 0 at this point. Broken in introduction in commit f37708f6, but mostly harmless (replying with '' a
nbd/server: fix nbd_negotiate_handle_info
namelen should be here, length is unrelated, and always 0 at this point. Broken in introduction in commit f37708f6, but mostly harmless (replying with '' as the name does not violate protocol, and does not confuse qemu as the nbd client since our implementation does not ask for the name; but might confuse some other client that does ask for the name especially if the default export is different than the export name being queried).
Adding an assert makes it obvious that we are not skipping any bytes in the client's message, as well as making it obvious that we were using the wrong variable.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> CC: qemu-stable@nongnu.org Message-Id: <20171101154204.27146-1-vsementsov@virtuozzo.com> [eblake: improve commit message, squash in assert addition] Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
f140e300 | 27-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd: Minimal structured read for client
Minimal implementation: for structured error only error_report error message.
Note that test 83 is now more verbose, because the implementation prints more w
nbd: Minimal structured read for client
Minimal implementation: for structured error only error_report error message.
Note that test 83 is now more verbose, because the implementation prints more warnings about unexpected communication errors; perhaps future patches should tone things down by using trace messages instead of traces, but the common case of successful communication is no noisier than before.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-13-eblake@redhat.com>
show more ...
|
56dc682b | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd: Move nbd_read() to common header
An upcoming change to block/nbd-client.c will want to read the tail of a structured reply chunk directly from the wire. Move this function to make it easier.
nbd: Move nbd_read() to common header
An upcoming change to block/nbd-client.c will want to read the tail of a structured reply chunk directly from the wire. Move this function to make it easier.
Based on a patch from Vladimir Sementsov-Ogievskiy.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-12-eblake@redhat.com>
show more ...
|
d2febedb | 27-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/client: prepare nbd_receive_reply for structured reply
In following patch nbd_receive_reply will be used both for simple and structured reply header receiving. NBDReply is altered into union of
nbd/client: prepare nbd_receive_reply for structured reply
In following patch nbd_receive_reply will be used both for simple and structured reply header receiving. NBDReply is altered into union of simple reply header and structured reply chunk header, simple error translation moved to block/nbd-client to be consistent with further structured reply error translation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-11-eblake@redhat.com>
show more ...
|
d795299b | 27-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/client: refactor nbd_receive_starttls
Split out nbd_request_simple_option to be reused for structured reply option.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed
nbd/client: refactor nbd_receive_starttls
Split out nbd_request_simple_option to be reused for structured reply option.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-10-eblake@redhat.com>
show more ...
|
a57f6dea | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Include human-readable message in structured errors
The NBD spec permits including a human-readable error string if structured replies are in force, so we might as well send the client t
nbd/server: Include human-readable message in structured errors
The NBD spec permits including a human-readable error string if structured replies are in force, so we might as well send the client the message that we logged on any error.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-9-eblake@redhat.com>
show more ...
|
5c54e7fa | 27-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd: Minimal structured read for server
Minimal implementation of structured read: one structured reply chunk, no segmentation. Minimal structured error implementation: no text message. Support DF f
nbd: Minimal structured read for server
Minimal implementation of structured read: one structured reply chunk, no segmentation. Minimal structured error implementation: no text message. Support DF flag, but just ignore it, as there is no segmentation any way.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-8-eblake@redhat.com>
show more ...
|
e68c35cf | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Refactor zero-length option check
Consolidate the response for a non-zero-length option payload into a new function, nbd_reject_length(). This check will also be used when introducing s
nbd/server: Refactor zero-length option check
Consolidate the response for a non-zero-length option payload into a new function, nbd_reject_length(). This check will also be used when introducing support for structured replies.
Note that STARTTLS response differs based on time: if the connection is still unencrypted, we set fatal to true (a client that can't request TLS correctly may still think that we are ready to start the TLS handshake, so we must disconnect); while if the connection is already encrypted, the client is sending a bogus request but is no longer at risk of being confused by continuing the connection.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-7-eblake@redhat.com> [eblake: correct return value on STARTTLS] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
8cbee49e | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Simplify nbd_negotiate_options loop
Instead of making each caller check whether a transmission error occurred, we can sink a common error check to the end of the loop.
Signed-off-by: Er
nbd/server: Simplify nbd_negotiate_options loop
Instead of making each caller check whether a transmission error occurred, we can sink a common error check to the end of the loop.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-6-eblake@redhat.com> [eblake: squash in compiler warning fix] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
show more ...
|
8fb48b8b | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd/server: Report error for write to read-only export
When the server is read-only, we were already reporting an error message for NBD_CMD_WRITE_ZEROES, but failed to set errp for a similar NBD_CMD
nbd/server: Report error for write to read-only export
When the server is read-only, we were already reporting an error message for NBD_CMD_WRITE_ZEROES, but failed to set errp for a similar NBD_CMD_WRITE. This will matter more once structured replies allow the server to propagate the errp information back to the client. While at it, use an error message that makes a bit more sense if viewed on the client side.
Note that when using qemu-io to test qemu-nbd behavior, it is rather difficult to convince qemu-io to send protocol violations (such as a read beyond bounds), because we have a lot of active checking on the client side that a qemu-io request makes sense before it ever goes over the wire to the server. The case of a client attempting a write when the server is started as 'qemu-nbd -r' is one of the few places where we can easily test error path handling, without having to resort to hacking in known temporary bugs to either the server or client. [Maybe we want a future patch to the client to do up-front checking on writes to a read-only export, the way it does up-front bounds checking; but I don't see anything in the NBD spec that points to a protocol violation in our current behavior.]
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-5-eblake@redhat.com>
show more ...
|
bae245d1 | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd: Expose constants and structs for structured read
Upcoming patches will implement the NBD structured reply extension [1] for both client and server roles. Declare the constants, structs, and lo
nbd: Expose constants and structs for structured read
Upcoming patches will implement the NBD structured reply extension [1] for both client and server roles. Declare the constants, structs, and lookup routines that will be valuable whether the server or client code is backported in isolation.
This includes moving one constant from an internal header to the public header, as part of the structured read processing will be done in block/nbd-client.c rather than nbd/client.c.
[1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md
Based on patches from Vladimir Sementsov-Ogievskiy.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-4-eblake@redhat.com>
show more ...
|
dd689440 | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd: Move nbd_errno_to_system_errno() to public header
This is needed in preparation for structured reply handling, as we will be performing the translation from NBD error to system errno value high
nbd: Move nbd_errno_to_system_errno() to public header
This is needed in preparation for structured reply handling, as we will be performing the translation from NBD error to system errno value higher in the stack at block/nbd-client.c.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-3-eblake@redhat.com>
show more ...
|
e7a78d0e | 27-Oct-2017 |
Eric Blake <eblake@redhat.com> |
nbd: Include error names in trace messages
NBD errors were originally sent over the wire based on Linux errno values; but not all the world is Linux, and not all platforms share the same values. Si
nbd: Include error names in trace messages
NBD errors were originally sent over the wire based on Linux errno values; but not all the world is Linux, and not all platforms share the same values. Since a number isn't very easy to decipher on all platforms, update the trace messages to include the name of NBD errors being sent/received over the wire. Tweak the trace messages to be at the point where we are using the NBD error, not the translation to the host errno values.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-2-eblake@redhat.com>
show more ...
|
92652b12 | 12-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd: header constants indenting
Prepare indenting for the following commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Messag
nbd: header constants indenting
Prepare indenting for the following commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20171012095319.136610-9-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
de79bfc3 | 12-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/server: simplify reply transmission
Send qiov via qio_channel_writev_all instead of calling nbd_write twice with a cork.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> M
nbd/server: simplify reply transmission
Send qiov via qio_channel_writev_all instead of calling nbd_write twice with a cork.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-8-vsementsov@virtuozzo.com> [eblake: rebase to tweaks earlier in series] Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
978df1b6 | 12-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/server: refactor nbd_co_send_simple_reply parameters
Pass client and buffer (*data) parameters directly, to make the function consistent with further structured reply sending functions.
Signed-
nbd/server: refactor nbd_co_send_simple_reply parameters
Pass client and buffer (*data) parameters directly, to make the function consistent with further structured reply sending functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
14cea41d | 12-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/server: do not use NBDReply structure
NBDReply structure will be upgraded in future patches to handle both simple and structured replies and will be used only in the client
Signed-off-by: Vladi
nbd/server: do not use NBDReply structure
NBDReply structure will be upgraded in future patches to handle both simple and structured replies and will be used only in the client
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-6-vsementsov@virtuozzo.com> [eblake: rebase to tweaks earlier in series] Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|
caad5384 | 12-Oct-2017 |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
nbd/server: structurize simple reply header sending
Use packed structure instead of pointer arithmetics.
Also, merge two redundant traces into one.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vse
nbd/server: structurize simple reply header sending
Use packed structure instead of pointer arithmetics.
Also, merge two redundant traces into one.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-5-vsementsov@virtuozzo.com> [eblake: tweak and mention impact on traces, fix errp usage] Signed-off-by: Eric Blake <eblake@redhat.com>
show more ...
|