From c0cdb87a8081619352f419adb22bab74054f7758 Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Fri, 26 Jul 2024 15:10:52 +0100 Subject: [PATCH] Print full NBD error from server resolves: RHEL-50667 --- ...int-full-error-in-handle_reply_error.patch | 191 ++++++++++++++++++ ...-overwrite-error-in-nbd_opt_-go-info.patch | 38 ++++ ...ator-Restore-assignment-to-local-err.patch | 43 ++++ ...-newstyle.c-Quote-untrusted-string-f.patch | 175 ++++++++++++++++ ...-newstyle.c-Don-t-sign-extend-escape.patch | 27 +++ copy-patches.sh | 2 +- libnbd.spec | 15 +- 7 files changed, 488 insertions(+), 3 deletions(-) create mode 100644 0001-generator-Print-full-error-in-handle_reply_error.patch create mode 100644 0002-lib-Don-t-overwrite-error-in-nbd_opt_-go-info.patch create mode 100644 0003-generator-Restore-assignment-to-local-err.patch create mode 100644 0004-generator-states-newstyle.c-Quote-untrusted-string-f.patch create mode 100644 0005-generator-states-newstyle.c-Don-t-sign-extend-escape.patch diff --git a/0001-generator-Print-full-error-in-handle_reply_error.patch b/0001-generator-Print-full-error-in-handle_reply_error.patch new file mode 100644 index 0000000..460a9e3 --- /dev/null +++ b/0001-generator-Print-full-error-in-handle_reply_error.patch @@ -0,0 +1,191 @@ +From 9e51ca3dc11b4abe2e5145837ae80863fc300646 Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Tue, 23 Jul 2024 17:22:12 +0100 +Subject: [PATCH] generator: Print full error in handle_reply_error + +Print the full error from the server during handshaking. This +modifies the contract of handle_reply_error so it calls set_error, +which can be overridden by callers or ignored completely. + +(cherry picked from commit cf49a49adc8abc8c917437db7461ed9956583877) +--- + generator/states-newstyle-opt-go.c | 32 +-------- + generator/states-newstyle-opt-list.c | 5 +- + generator/states-newstyle-opt-meta-context.c | 8 +-- + generator/states-newstyle.c | 68 ++++++++++++++++++-- + 4 files changed, 69 insertions(+), 44 deletions(-) + +diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c +index 5bc9a9ae..f6eb8afc 100644 +--- a/generator/states-newstyle-opt-go.c ++++ b/generator/states-newstyle-opt-go.c +@@ -247,37 +247,9 @@ STATE_MACHINE { + SET_NEXT_STATE (%.DEAD); + return 0; + } +- /* Decode expected known errors into a nicer string */ +- switch (reply) { +- case NBD_REP_ERR_UNSUP: ++ if (reply == NBD_REP_ERR_UNSUP) + assert (h->opt_current == NBD_OPT_INFO); +- set_error (ENOTSUP, "handshake: server lacks NBD_OPT_INFO support"); +- break; +- case NBD_REP_ERR_POLICY: +- case NBD_REP_ERR_PLATFORM: +- set_error (0, "handshake: server policy prevents NBD_OPT_GO"); +- break; +- case NBD_REP_ERR_INVALID: +- case NBD_REP_ERR_TOO_BIG: +- set_error (EINVAL, "handshake: server rejected NBD_OPT_GO as invalid"); +- break; +- case NBD_REP_ERR_TLS_REQD: +- set_error (ENOTSUP, "handshake: server requires TLS encryption first"); +- break; +- case NBD_REP_ERR_UNKNOWN: +- set_error (ENOENT, "handshake: server has no export named '%s'", +- h->export_name); +- break; +- case NBD_REP_ERR_SHUTDOWN: +- set_error (ESHUTDOWN, "handshake: server is shutting down"); +- break; +- case NBD_REP_ERR_BLOCK_SIZE_REQD: +- set_error (EINVAL, "handshake: server requires specific block sizes"); +- break; +- default: +- set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%" PRIx32, +- reply); +- } ++ + nbd_internal_reset_size_and_flags (h); + h->meta_valid = false; + err = nbd_get_errno () ? : ENOTSUP; +diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c +index cdd4676e..6605ee0a 100644 +--- a/generator/states-newstyle-opt-list.c ++++ b/generator/states-newstyle-opt-list.c +@@ -127,9 +127,8 @@ STATE_MACHINE { + SET_NEXT_STATE (%.DEAD); + return 0; + } +- err = ENOTSUP; +- set_error (err, "unexpected response, possibly the server does not " +- "support listing exports"); ++ debug (h, "unexpected response, possibly the server does not " ++ "support listing exports"); + break; + } + +diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c +index 6f016e66..3945411e 100644 +--- a/generator/states-newstyle-opt-meta-context.c ++++ b/generator/states-newstyle-opt-meta-context.c +@@ -270,12 +270,8 @@ STATE_MACHINE { + } + + if (opt == h->opt_current) { +- /* XXX Should we decode specific expected errors, like +- * REP_ERR_UNKNOWN to ENOENT or REP_ERR_TOO_BIG to ERANGE? +- */ +- err = ENOTSUP; +- set_error (err, "unexpected response, possibly the server does not " +- "support meta contexts"); ++ debug (h, "unexpected response, possibly the server does not " ++ "support meta contexts"); + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); + SET_NEXT_STATE (%.NEGOTIATING); +diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c +index 45893a8b..6c7cc45c 100644 +--- a/generator/states-newstyle.c ++++ b/generator/states-newstyle.c +@@ -79,14 +79,18 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt) + return 0; + } + +-/* Check an unexpected server reply. If it is an error, log any +- * message from the server and return 0; otherwise, return -1. ++/* Check an unexpected server reply error. ++ * ++ * This calls set_error with a descriptive error message and returns ++ * 0. Unless there is a further unexpected error while processing ++ * this error, in which case it calls set_error and returns -1. + */ + static int + handle_reply_error (struct nbd_handle *h) + { + uint32_t len; + uint32_t reply; ++ char *msg = NULL; + + len = be32toh (h->sbuf.or.option_reply.replylen); + reply = be32toh (h->sbuf.or.option_reply.reply); +@@ -101,9 +105,63 @@ handle_reply_error (struct nbd_handle *h) + return -1; + } + +- if (len > 0) +- debug (h, "handshake: server error message: %.*s", (int)len, +- h->sbuf.or.payload.err_msg); ++ /* Decode expected errors into a nicer string. ++ * ++ * XXX Note this string comes directly from the server, and most ++ * libnbd users simply print the error using 'fprintf'. We really ++ * ought to quote this string somehow, but we don't have a useful ++ * function for that. ++ */ ++ if (len > 0) { ++ if (asprintf (&msg, ": %.*s", ++ (int)len, h->sbuf.or.payload.err_msg) == -1) { ++ set_error (errno, "asprintf"); ++ return -1; ++ } ++ } ++ ++ switch (reply) { ++ case NBD_REP_ERR_UNSUP: ++ set_error (ENOTSUP, "the operation is not supported by the server%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_POLICY: ++ set_error (0, "server policy prevents the operation%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_PLATFORM: ++ set_error (0, "the operation is not supported by the server platform%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_INVALID: ++ set_error (EINVAL, "the server rejected this operation as invalid%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_TOO_BIG: ++ set_error (EINVAL, "the operation is too large to process%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_TLS_REQD: ++ set_error (ENOTSUP, "the server requires TLS encryption first%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_UNKNOWN: ++ set_error (ENOENT, "the server has no export named '%s'%s", ++ h->export_name, msg ? : ""); ++ break; ++ case NBD_REP_ERR_SHUTDOWN: ++ set_error (ESHUTDOWN, "the server is shutting down%s", ++ msg ? : ""); ++ break; ++ case NBD_REP_ERR_BLOCK_SIZE_REQD: ++ set_error (EINVAL, "the server requires specific block sizes%s", ++ msg ? : ""); ++ break; ++ default: ++ set_error (0, "handshake: unknown reply from the server: 0x%" PRIx32 "%s", ++ reply, msg ? : ""); ++ } ++ free (msg); + + return 0; + } +-- +2.43.0 + diff --git a/0002-lib-Don-t-overwrite-error-in-nbd_opt_-go-info.patch b/0002-lib-Don-t-overwrite-error-in-nbd_opt_-go-info.patch new file mode 100644 index 0000000..8b6dd8b --- /dev/null +++ b/0002-lib-Don-t-overwrite-error-in-nbd_opt_-go-info.patch @@ -0,0 +1,38 @@ +From 6ef03cf2a1fe9f88c07c6f2d97afe9f82bfe03a8 Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Tue, 23 Jul 2024 17:26:39 +0100 +Subject: [PATCH] lib: Don't overwrite error in nbd_opt_{go,info} + +We already set the error in handle_reply_error, so don't overwrite +that here. + +(cherry picked from commit 474a4ae6c8d11212a4a8c06ea3e8b3fd97a7e97d) +--- + lib/opt.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/lib/opt.c b/lib/opt.c +index 600265a0..5872dd54 100644 +--- a/lib/opt.c ++++ b/lib/opt.c +@@ -99,7 +99,7 @@ nbd_unlocked_opt_go (struct nbd_handle *h) + if (r == 0 && err) { + assert (nbd_internal_is_state_negotiating (get_next_state (h)) || + nbd_internal_is_state_dead (get_next_state (h))); +- set_error (err, "server replied with error to opt_go request"); ++ /* handle_reply_error already called set_error */ + return -1; + } + if (r == 0) +@@ -122,7 +122,7 @@ nbd_unlocked_opt_info (struct nbd_handle *h) + if (r == 0 && err) { + assert (nbd_internal_is_state_negotiating (get_next_state (h)) || + nbd_internal_is_state_dead (get_next_state (h))); +- set_error (err, "server replied with error to opt_info request"); ++ /* handle_reply_error already called set_error */ + return -1; + } + return r; +-- +2.43.0 + diff --git a/0003-generator-Restore-assignment-to-local-err.patch b/0003-generator-Restore-assignment-to-local-err.patch new file mode 100644 index 0000000..290b8c1 --- /dev/null +++ b/0003-generator-Restore-assignment-to-local-err.patch @@ -0,0 +1,43 @@ +From 1357302046b5eaae09a8817ff050790b0285183d Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Thu, 25 Jul 2024 13:39:28 +0100 +Subject: [PATCH] generator: Restore assignment to local 'err' + +I accidentally removed the assignment of local variable 'err' along +these paths in commit cf49a49adc ("generator: Print full error in +handle_reply_error"). + +Fixes: commit cf49a49adc8abc8c917437db7461ed9956583877 +(cherry picked from commit e75d20b9e19143b1bd0d232fc49cb2e0287f824a) +--- + generator/states-newstyle-opt-list.c | 1 + + generator/states-newstyle-opt-meta-context.c | 1 + + 2 files changed, 2 insertions(+) + +diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c +index 6605ee0a..48559574 100644 +--- a/generator/states-newstyle-opt-list.c ++++ b/generator/states-newstyle-opt-list.c +@@ -129,6 +129,7 @@ STATE_MACHINE { + } + debug (h, "unexpected response, possibly the server does not " + "support listing exports"); ++ err = ENOTSUP; + break; + } + +diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c +index 3945411e..699e24aa 100644 +--- a/generator/states-newstyle-opt-meta-context.c ++++ b/generator/states-newstyle-opt-meta-context.c +@@ -272,6 +272,7 @@ STATE_MACHINE { + if (opt == h->opt_current) { + debug (h, "unexpected response, possibly the server does not " + "support meta contexts"); ++ err = ENOTSUP; + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); + SET_NEXT_STATE (%.NEGOTIATING); +-- +2.43.0 + diff --git a/0004-generator-states-newstyle.c-Quote-untrusted-string-f.patch b/0004-generator-states-newstyle.c-Quote-untrusted-string-f.patch new file mode 100644 index 0000000..bdc9946 --- /dev/null +++ b/0004-generator-states-newstyle.c-Quote-untrusted-string-f.patch @@ -0,0 +1,175 @@ +From 1de017428047f1a8991285766b69b767ab895c24 Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Thu, 25 Jul 2024 13:25:34 +0100 +Subject: [PATCH] generator/states-newstyle.c: Quote untrusted string from the + server + +Updates: commit cf49a49adc8abc8c917437db7461ed9956583877 +(cherry picked from commit 5dbfc418cb6176102634acea2256b2335520159c) +--- + generator/states-newstyle.c | 124 ++++++++++++++++++++---------------- + 1 file changed, 68 insertions(+), 56 deletions(-) + +diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c +index 6c7cc45c..8c483bd2 100644 +--- a/generator/states-newstyle.c ++++ b/generator/states-newstyle.c +@@ -18,6 +18,7 @@ + + #include + ++#include "ascii-ctype.h" + #include "internal.h" + + /* Common code for parsing a reply to NBD_OPT_*. */ +@@ -88,80 +89,91 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt) + static int + handle_reply_error (struct nbd_handle *h) + { +- uint32_t len; + uint32_t reply; +- char *msg = NULL; ++ uint32_t replylen; ++ FILE *fp; ++ char *s = NULL; ++ size_t len = 0; ++ int err = 0; + +- len = be32toh (h->sbuf.or.option_reply.replylen); + reply = be32toh (h->sbuf.or.option_reply.reply); + if (!NBD_REP_IS_ERR (reply)) { + set_error (0, "handshake: unexpected option reply type %d", reply); + return -1; + } + ++ replylen = be32toh (h->sbuf.or.option_reply.replylen); + assert (NBD_MAX_STRING < sizeof h->sbuf.or.payload); +- if (len > NBD_MAX_STRING) { ++ if (replylen > NBD_MAX_STRING) { + set_error (0, "handshake: option error string too long"); + return -1; + } + +- /* Decode expected errors into a nicer string. +- * +- * XXX Note this string comes directly from the server, and most +- * libnbd users simply print the error using 'fprintf'. We really +- * ought to quote this string somehow, but we don't have a useful +- * function for that. +- */ +- if (len > 0) { +- if (asprintf (&msg, ": %.*s", +- (int)len, h->sbuf.or.payload.err_msg) == -1) { +- set_error (errno, "asprintf"); +- return -1; +- } ++ /* Decode expected errors into a nicer string. */ ++ fp = open_memstream (&s, &len); ++ if (fp == NULL) { ++ set_error (errno, "open_memstream"); ++ return -1; + } + + switch (reply) { + case NBD_REP_ERR_UNSUP: +- set_error (ENOTSUP, "the operation is not supported by the server%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_POLICY: +- set_error (0, "server policy prevents the operation%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_PLATFORM: +- set_error (0, "the operation is not supported by the server platform%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_INVALID: +- set_error (EINVAL, "the server rejected this operation as invalid%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_TOO_BIG: +- set_error (EINVAL, "the operation is too large to process%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_TLS_REQD: +- set_error (ENOTSUP, "the server requires TLS encryption first%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_UNKNOWN: +- set_error (ENOENT, "the server has no export named '%s'%s", +- h->export_name, msg ? : ""); +- break; +- case NBD_REP_ERR_SHUTDOWN: +- set_error (ESHUTDOWN, "the server is shutting down%s", +- msg ? : ""); +- break; +- case NBD_REP_ERR_BLOCK_SIZE_REQD: +- set_error (EINVAL, "the server requires specific block sizes%s", +- msg ? : ""); +- break; +- default: +- set_error (0, "handshake: unknown reply from the server: 0x%" PRIx32 "%s", +- reply, msg ? : ""); ++ err = ENOTSUP; ++ fprintf (fp, "the operation is not supported by the server"); ++ break; ++ case NBD_REP_ERR_POLICY: ++ fprintf (fp, "server policy prevents the operation"); ++ break; ++ case NBD_REP_ERR_PLATFORM: ++ fprintf (fp, "the operation is not supported by the server platform"); ++ break; ++ case NBD_REP_ERR_INVALID: ++ err = EINVAL; ++ fprintf (fp, "the server rejected this operation as invalid"); ++ break; ++ case NBD_REP_ERR_TOO_BIG: ++ err = EINVAL; ++ fprintf (fp, "the operation is too large to process"); ++ break; ++ case NBD_REP_ERR_TLS_REQD: ++ err = ENOTSUP; ++ fprintf (fp, "the server requires TLS encryption first"); ++ break; ++ case NBD_REP_ERR_UNKNOWN: ++ err = ENOENT; ++ fprintf (fp, "the server has no export named '%s'", h->export_name); ++ break; ++ case NBD_REP_ERR_SHUTDOWN: ++ err = ESHUTDOWN; ++ fprintf (fp, "the server is shutting down"); ++ break; ++ case NBD_REP_ERR_BLOCK_SIZE_REQD: ++ err = EINVAL; ++ fprintf (fp, "the server requires specific block sizes"); ++ break; ++ default: ++ fprintf (fp, "handshake: unknown reply from the server: 0x%" PRIx32, ++ reply); ++ } ++ ++ if (replylen > 0) { ++ /* Since this message comes from the server, take steps to quote it. */ ++ uint32_t i; ++ const char *msg = h->sbuf.or.payload.err_msg; ++ ++ fprintf (fp, ": "); ++ for (i = 0; i < replylen; ++i) { ++ if (ascii_isprint (msg[i])) ++ fputc (msg[i], fp); ++ else ++ fprintf (fp, "\\x%02x", msg[i]); + } +- free (msg); ++ } ++ ++ fclose (fp); ++ ++ set_error (err, "%s", s); ++ free (s); + + return 0; + } +-- +2.43.0 + diff --git a/0005-generator-states-newstyle.c-Don-t-sign-extend-escape.patch b/0005-generator-states-newstyle.c-Don-t-sign-extend-escape.patch new file mode 100644 index 0000000..f4c8f90 --- /dev/null +++ b/0005-generator-states-newstyle.c-Don-t-sign-extend-escape.patch @@ -0,0 +1,27 @@ +From f24c7801aef0e2f8936d74ac5237c3391fb39d26 Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Thu, 25 Jul 2024 15:48:46 +0100 +Subject: [PATCH] generator/states-newstyle.c: Don't sign extend escaped chars + +Fixes: commit 5dbfc418cb6176102634acea2256b2335520159c +(cherry picked from commit 0d6c6bbb3386de3b60ab6c4831045f2b1896051b) +--- + generator/states-newstyle.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c +index 8c483bd2..1e026a8a 100644 +--- a/generator/states-newstyle.c ++++ b/generator/states-newstyle.c +@@ -159,7 +159,7 @@ handle_reply_error (struct nbd_handle *h) + if (replylen > 0) { + /* Since this message comes from the server, take steps to quote it. */ + uint32_t i; +- const char *msg = h->sbuf.or.payload.err_msg; ++ const unsigned char *msg = (unsigned char *) h->sbuf.or.payload.err_msg; + + fprintf (fp, ": "); + for (i = 0; i < replylen; ++i) { +-- +2.43.0 + diff --git a/copy-patches.sh b/copy-patches.sh index 00041ac..bab7b72 100755 --- a/copy-patches.sh +++ b/copy-patches.sh @@ -6,7 +6,7 @@ set -e # directory. Use it like this: # ./copy-patches.sh -rhel_version=8.3 +rhel_version=10.0 # Check we're in the right directory. if [ ! -f libnbd.spec ]; then diff --git a/libnbd.spec b/libnbd.spec index f82ef8f..a349036 100644 --- a/libnbd.spec +++ b/libnbd.spec @@ -21,7 +21,7 @@ Name: libnbd Version: 1.20.2 -Release: 1%{?dist} +Release: 2%{?dist} Summary: NBD client library in userspace License: LGPL-2.0-or-later AND BSD-3-Clause @@ -37,6 +37,15 @@ Source2: libguestfs.keyring # Maintainer script which helps with handling patches. Source3: copy-patches.sh +# Patches are stored in the upstream repository: +# https://gitlab.com/nbdkit/libnbd/-/commits/rhel-10.0/ + +Patch0001: 0001-generator-Print-full-error-in-handle_reply_error.patch +Patch0002: 0002-lib-Don-t-overwrite-error-in-nbd_opt_-go-info.patch +Patch0003: 0003-generator-Restore-assignment-to-local-err.patch +Patch0004: 0004-generator-states-newstyle.c-Quote-untrusted-string-f.patch +Patch0005: 0005-generator-states-newstyle.c-Don-t-sign-extend-escape.patch + %if 0%{verify_tarball_signature} BuildRequires: gnupg2 %endif @@ -376,10 +385,12 @@ make %{?_smp_mflags} check || { %changelog -* Fri Jul 19 2024 Richard W.M. Jones - 1.20.2-1 +* Fri Jul 26 2024 Richard W.M. Jones - 1.20.2-2 - Rebase to libnbd 1.20.2 - Fix multiple flaws in TLS server certificate checking resolves: RHEL-49802 +- Print full NBD error from server + resolves: RHEL-50667 * Tue Jun 25 2024 Troy Dawson - 1.20.1-5 - Bump release for June 2024 mass rebuild