diff --git a/iscsi-scst/usr/isns.c b/iscsi-scst/usr/isns.c index 96ea65594..5e4f68df4 100644 --- a/iscsi-scst/usr/isns.c +++ b/iscsi-scst/usr/isns.c @@ -772,6 +772,23 @@ static int recv_pdu(int fd, struct isns_io *rx, struct isns_hdr *hdr) function, length, flags, transaction, sequence); \ } +/* Validate the complete payload before a handler changes names or ACLs. */ +static bool isns_tlvs_valid(const struct isns_tlv *tlv, size_t length) +{ + while (length) { + uint32_t vlen; + + if (length < sizeof(*tlv)) + return false; + vlen = ntohl(tlv->length); + if (vlen > length - sizeof(*tlv) || vlen % ISNS_ALIGN != 0) + return false; + length -= sizeof(*tlv) + vlen; + tlv = (const struct isns_tlv *)((const char *)tlv->value + vlen); + } + return true; +} + static char *print_scn_pdu(struct isns_hdr *hdr) { struct isns_tlv *tlv = (struct isns_tlv *)hdr->pdu; @@ -783,13 +800,14 @@ static char *print_scn_pdu(struct isns_hdr *hdr) char *name = NULL; get_hdr_param(hdr, function, length, flags, transaction, sequence); + if (!isns_tlvs_valid(tlv, length)) { + log_error("Malformed iSNS SCN attributes"); + return NULL; + } while (length) { uint32_t vlen = ntohl(tlv->length); - if (vlen + sizeof(*tlv) > length) - vlen = length - sizeof(*tlv); - if (vlen < 4) goto next; @@ -826,7 +844,7 @@ static void qry_rsp_handle(struct isns_hdr *hdr) uint16_t flags __attribute__((unused)); uint16_t sequence __attribute__((unused)); uint16_t length, transaction; - uint32_t status = (uint32_t) (*hdr->pdu); + uint32_t status; struct isns_qry_mgmt *mgmt, *n; struct target *target; struct isns_initiator *ini; @@ -847,6 +865,11 @@ static void qry_rsp_handle(struct isns_hdr *hdr) return; found: + if (length < sizeof(status)) { + log_error("Short iSNS query response status"); + goto free_qry_mgmt; + } + status = ntohl(*hdr->pdu); if (status) { log_error("%s %d: error response %u", __func__, __LINE__, status); @@ -854,6 +877,14 @@ found: goto free_qry_mgmt; } + /* Skip the status only after proving that it is present. */ + tlv = (struct isns_tlv *)((char *)hdr->pdu + sizeof(status)); + length -= sizeof(status); + if (!isns_tlvs_valid(tlv, length)) { + log_error("Malformed iSNS query response attributes"); + goto free_qry_mgmt; + } + if (!strlen(mgmt->name)) { log_debug(1, "%s %d: skip %u", __func__, __LINE__, transaction); @@ -869,18 +900,9 @@ found: free_all_acl(target); - /* skip status */ - if (length < 4) - goto free_qry_mgmt; - tlv = (struct isns_tlv *)((char *)hdr->pdu + 4); - length -= 4; - while (length) { uint32_t vlen = ntohl(tlv->length); - if (vlen + sizeof(*tlv) > length) - vlen = length - sizeof(*tlv); - if (vlen < 4) goto next; diff --git a/tests/ginkgo/README.md b/tests/ginkgo/README.md index f1722a3ff..bf934bc4e 100644 --- a/tests/ginkgo/README.md +++ b/tests/ginkgo/README.md @@ -127,6 +127,11 @@ failure cleanup. It runs through `ginkgo-check` and requires `cc`. Interface headers are prepared in a temporary directory; no daemon or storage is used. Run it separately with `go test ./internal/chap` from this directory. +The isolated `internal/isns` suite checks production iSNS handlers against +malformed payloads ending at a guard page, including preservation of existing +ACLs. It also runs through `ginkgo-check`; use `go test ./internal/isns` to +run it separately. It requires `cc` and opens no sockets. + ## SCSI and I/O Install Python 3, util-linux and sg3_utils on the command host: the target for diff --git a/tests/ginkgo/internal/isns/isns_test.go b/tests/ginkgo/internal/isns/isns_test.go new file mode 100644 index 000000000..93c4859a5 --- /dev/null +++ b/tests/ginkgo/internal/isns/isns_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-2.0-only +package isns + +import ( + _ "embed" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "scst-component-tests/internal/admin" + "scst-component-tests/internal/cprobe" +) + +//go:embed testdata/tlv_probe.c +var tlvProbe string + +func TestISNS(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "SCST iSNS payload bounds") +} + +var _ = Describe("iSNS payload validation", Label("unit", "userspace", "isns"), func() { + var probe string + var client *admin.Client + + BeforeEach(func(ctx SpecContext) { + client = &admin.Client{Log: GinkgoWriter} + dir, err := os.MkdirTemp("", "scst-isns-probe-") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { + if client.Blocked() != nil { + AddReportEntry("preserved iSNS probe", dir) + return + } + Expect(os.RemoveAll(dir)).To(Succeed()) + }) + source := os.Getenv("SCST_TEST_SOURCE_DIR") + if source == "" { + _, file, _, ok := runtime.Caller(0) + Expect(ok).To(BeTrue()) + source = filepath.Clean(filepath.Join(filepath.Dir(file), "../../../..")) + } + Expect(cprobe.PrepareISCSIHeaders(source, dir)).To(Succeed()) + input := filepath.Join(dir, "tlv_probe.c") + Expect(os.WriteFile(input, []byte(tlvProbe), 0600)).To(Succeed()) + probe = filepath.Join(dir, "tlv-probe") + // Dead-section removal keeps this unit probe independent of the + // daemon's unrelated authentication and transport dependencies. + r, err := client.RunProgram(ctx, 30*time.Second, + "cc", "-D_GNU_SOURCE", "-O2", "-ffunction-sections", "-fdata-sections", + "-I"+dir, + "-I"+filepath.Join(source, "iscsi-scst/usr"), + "-I"+filepath.Join(source, "iscsi-scst/include"), + "-I"+filepath.Join(source, "scst/include"), + input, "-Wl,--gc-sections", "-o", probe) + Expect(err).NotTo(HaveOccurred(), r.String()) + Expect(r.ExitCode).To(BeZero(), r.String()) + }) + + DescribeTable("checks bounds before reading or replacing ACLs", func(ctx SpecContext, scenario string) { + r, err := client.RunProgram(ctx, 10*time.Second, probe, scenario) + Expect(err).NotTo(HaveOccurred(), r.String()) + Expect(r.ExitCode).To(BeZero(), r.String()) + Expect(r.Stdout).To(Equal("ok\n"), r.String()) + }, + Entry("empty and short SCN payloads stop at the guard page", "scn-short"), + Entry("oversized SCN value length is rejected", "scn-oversized"), + Entry("truncated SCN value is rejected", "scn-truncated"), + Entry("unaligned SCN value is rejected", "scn-unaligned"), + Entry("malformed SCN tail does not modify the earlier name", "scn-tail"), + Entry("valid SCN name and bitmap are accepted", "scn-valid"), + Entry("zero-length delimiter advances to the next attribute", "scn-delimiter"), + Entry("short query status preserves the ACL", "query-short"), + Entry("short query TLV header preserves the ACL", "query-short-tlv"), + Entry("oversized query value preserves the ACL", "query-oversized"), + Entry("malformed query tail preserves the complete ACL", "query-tail"), + Entry("error status preserves the ACL", "query-error"), + Entry("valid query replaces the ACL", "query-valid"), + ) +}) diff --git a/tests/ginkgo/internal/isns/testdata/tlv_probe.c b/tests/ginkgo/internal/isns/testdata/tlv_probe.c new file mode 100644 index 000000000..99b478194 --- /dev/null +++ b/tests/ginkgo/internal/isns/testdata/tlv_probe.c @@ -0,0 +1,194 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include +#include +#include +#include +#include +#include +#include + +/* Keep both production handlers; no socket or daemon entry point is invoked. */ +#include "isns.c" + +static void check(bool condition, int line, const char *expression) +{ + if (!condition) { + fprintf(stderr, "line %d: %s\n", line, expression); + exit(EXIT_FAILURE); + } +} + +#define CHECK(condition) check(!!(condition), __LINE__, #condition) + +static struct target probe_target; + +void __log(const char *func, int line, int prio, int level, const char *fmt, ...) +{ +} + +struct target *target_find_by_name(const char *name) +{ + return strcmp(name, probe_target.name) == 0 ? &probe_target : NULL; +} + +struct guarded_pdu { + void *mapping; + size_t size; + struct isns_hdr *hdr; +}; + +static int make_pdu(struct guarded_pdu *pdu, size_t length) +{ + long page = sysconf(_SC_PAGESIZE); + + CHECK(page > 0 && length + sizeof(*pdu->hdr) < (size_t)page); + pdu->size = 2 * (size_t)page; + pdu->mapping = mmap(NULL, pdu->size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + CHECK(pdu->mapping != MAP_FAILED); + CHECK(mprotect((char *)pdu->mapping + page, page, PROT_NONE) == 0); + /* Any read or write past the declared payload reaches the guard page. */ + pdu->hdr = (struct isns_hdr *)((char *)pdu->mapping + page - + sizeof(*pdu->hdr) - length); + memset(pdu->hdr, 0, sizeof(*pdu->hdr) + length); + pdu->hdr->length = htons(length); + pdu->hdr->transaction = htons(17); + return 0; +} + +static void put_tlv(void *buffer, uint32_t tag, uint32_t length, const void *value) +{ + struct isns_tlv *tlv = buffer; + + tlv->tag = htonl(tag); + tlv->length = htonl(length); + if (value) + memcpy(tlv->value, value, length); +} + +static int check_short_scn(void) +{ + size_t length; + + for (length = 0; length < sizeof(struct isns_tlv); length++) { + struct guarded_pdu pdu; + + CHECK(make_pdu(&pdu, length) == 0); + CHECK(!print_scn_pdu(pdu.hdr)); + CHECK(munmap(pdu.mapping, pdu.size) == 0); + } + return 0; +} + +static int check_scn(const char *scenario) +{ + struct guarded_pdu pdu; + uint32_t bitmap = htonl(1); + bool valid = strcmp(scenario, "scn-valid") == 0; + bool delimiter = strcmp(scenario, "scn-delimiter") == 0; + bool tail = strcmp(scenario, "scn-tail") == 0; + bool unaligned = strcmp(scenario, "scn-unaligned") == 0; + bool oversized = strcmp(scenario, "scn-oversized") == 0; + size_t length = valid ? 24 : delimiter ? 20 : tail ? 16 : unaligned ? 9 : 12; + char *name; + + CHECK(valid || delimiter || tail || unaligned || oversized || + strcmp(scenario, "scn-truncated") == 0); + CHECK(make_pdu(&pdu, length) == 0); + if (valid) { + put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME, 4, "new"); + put_tlv((char *)pdu.hdr->pdu + 12, ISNS_ATTR_ISCSI_SCN_BITMAP, 4, &bitmap); + } else if (delimiter) { + put_tlv(pdu.hdr->pdu, 0, 0, NULL); + put_tlv((char *)pdu.hdr->pdu + 8, ISNS_ATTR_ISCSI_NAME, 4, "new"); + } else if (tail) { + put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME, 4, "oldX"); + } else { + put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME, + oversized ? UINT32_MAX : unaligned ? 1 : 8, NULL); + } + name = print_scn_pdu(pdu.hdr); + if (valid || delimiter) { + CHECK(name && strcmp(name, "new") == 0); + } else { + CHECK(!name); + if (tail) + CHECK(memcmp((char *)pdu.hdr->pdu + 8, "oldX", 4) == 0); + } + CHECK(munmap(pdu.mapping, pdu.size) == 0); + return 0; +} + +static int check_query(size_t length, const char *scenario) +{ + struct guarded_pdu pdu; + struct isns_initiator *original, *current; + struct isns_qry_mgmt *mgmt; + uint32_t node_type = htonl(ISNS_NODE_INITIATOR); + bool valid = strcmp(scenario, "query-valid") == 0; + bool tail = strcmp(scenario, "query-tail") == 0; + + CHECK(make_pdu(&pdu, length) == 0); + INIT_LIST_HEAD(&qry_list); + INIT_LIST_HEAD(&probe_target.isns_head); + snprintf(probe_target.name, sizeof(probe_target.name), "target"); + original = calloc(1, sizeof(*original)); + mgmt = calloc(1, sizeof(*mgmt)); + CHECK(original && mgmt); + snprintf(original->name, sizeof(original->name), "old"); + list_add_tail(&original->ilist, &probe_target.isns_head); + mgmt->transaction = 17; + snprintf(mgmt->name, sizeof(mgmt->name), "%s", probe_target.name); + list_add_tail(&mgmt->qlist, &qry_list); + if (valid || tail) { + put_tlv((char *)pdu.hdr->pdu + 4, ISNS_ATTR_ISCSI_NAME, 4, "new"); + put_tlv((char *)pdu.hdr->pdu + 16, ISNS_ATTR_ISCSI_NODE_TYPE, 4, &node_type); + } else if (strcmp(scenario, "query-oversized") == 0) { + put_tlv((char *)pdu.hdr->pdu + 4, ISNS_ATTR_ISCSI_NAME, UINT32_MAX, NULL); + } else if (strcmp(scenario, "query-error") == 0) { + pdu.hdr->pdu[0] = htonl(3); + } + qry_rsp_handle(pdu.hdr); + CHECK(list_empty(&qry_list)); + CHECK(!list_empty(&probe_target.isns_head)); + current = list_entry(probe_target.isns_head.q_forw, struct isns_initiator, ilist); + CHECK(current->ilist.q_forw == &probe_target.isns_head); + CHECK(strcmp(current->name, valid ? "new" : "old") == 0); + if (!valid) + CHECK(current == original); + free_all_acl(&probe_target); + CHECK(munmap(pdu.mapping, pdu.size) == 0); + return 0; +} + +int main(int argc, char **argv) +{ + const char *scenario; + size_t length; + + CHECK(argc == 2); + scenario = argv[1]; + if (strcmp(scenario, "scn-short") == 0) { + CHECK(check_short_scn() == 0); + } else if (strncmp(scenario, "scn-", 4) == 0) { + CHECK(check_scn(scenario) == 0); + } else if (strcmp(scenario, "query-short") == 0) { + for (length = 0; length < 4; length++) + CHECK(check_query(length, scenario) == 0); + } else if (strcmp(scenario, "query-short-tlv") == 0) { + for (length = 5; length < 12; length++) + CHECK(check_query(length, scenario) == 0); + } else if (strcmp(scenario, "query-valid") == 0) { + CHECK(check_query(28, scenario) == 0); + } else if (strcmp(scenario, "query-tail") == 0) { + CHECK(check_query(32, scenario) == 0); + } else if (strcmp(scenario, "query-oversized") == 0) { + CHECK(check_query(12, scenario) == 0); + } else if (strcmp(scenario, "query-error") == 0) { + CHECK(check_query(4, scenario) == 0); + } else { + CHECK(0); + } + puts("ok"); + return 0; +}