Andrew Geissler | 82c905d | 2020-04-13 13:39:40 -0500 | [diff] [blame] | 1 | From 26e64ca78ae844c5ceedde89531e2924d7d4594c Mon Sep 17 00:00:00 2001 |
| 2 | From: Chrostoper Ertl <chertl@microsoft.com> |
| 3 | Date: Thu, 28 Nov 2019 17:13:45 +0000 |
| 4 | Subject: [PATCH 5/5] fru, sdr: Fix id_string buffer overflows |
| 5 | |
| 6 | Final part of the fixes for CVE-2020-5208, see |
| 7 | https://github.com/ipmitool/ipmitool/security/advisories/GHSA-g659-9qxw-p7cp |
| 8 | |
| 9 | 9 variants of stack buffer overflow when parsing `id_string` field of |
| 10 | SDR records returned from `CMD_GET_SDR` command. |
| 11 | |
| 12 | SDR record structs have an `id_code` field, and an `id_string` `char` |
| 13 | array. |
| 14 | |
| 15 | The length of `id_string` is calculated as `(id_code & 0x1f) + 1`, |
| 16 | which can be larger than expected 16 characters (if `id_code = 0xff`, |
| 17 | then length will be `(0xff & 0x1f) + 1 = 32`). |
| 18 | |
| 19 | In numerous places, this can cause stack buffer overflow when copying |
| 20 | into fixed buffer of size `17` bytes from this calculated length. |
| 21 | |
| 22 | Upstream-Status: Backport[https://github.com/ipmitool/ipmitool/commit/7ccea283dd62a05a320c1921e3d8d71a87772637] |
| 23 | CVE: CVE-2020-5208 |
| 24 | |
| 25 | Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com> |
| 26 | --- |
| 27 | lib/ipmi_fru.c | 2 +- |
| 28 | lib/ipmi_sdr.c | 40 ++++++++++++++++++++++++---------------- |
| 29 | 2 files changed, 25 insertions(+), 17 deletions(-) |
| 30 | |
| 31 | diff --git a/lib/ipmi_fru.c b/lib/ipmi_fru.c |
| 32 | index b71ea23..1decea2 100644 |
| 33 | --- a/lib/ipmi_fru.c |
| 34 | +++ b/lib/ipmi_fru.c |
| 35 | @@ -3038,7 +3038,7 @@ ipmi_fru_print(struct ipmi_intf * intf, struct sdr_record_fru_locator * fru) |
| 36 | return 0; |
| 37 | |
| 38 | memset(desc, 0, sizeof(desc)); |
| 39 | - memcpy(desc, fru->id_string, fru->id_code & 0x01f); |
| 40 | + memcpy(desc, fru->id_string, __min(fru->id_code & 0x01f, sizeof(desc))); |
| 41 | desc[fru->id_code & 0x01f] = 0; |
| 42 | printf("FRU Device Description : %s (ID %d)\n", desc, fru->device_id); |
| 43 | |
| 44 | diff --git a/lib/ipmi_sdr.c b/lib/ipmi_sdr.c |
| 45 | index fa7b082..175a86f 100644 |
| 46 | --- a/lib/ipmi_sdr.c |
| 47 | +++ b/lib/ipmi_sdr.c |
| 48 | @@ -2113,7 +2113,7 @@ ipmi_sdr_print_sensor_eventonly(struct ipmi_intf *intf, |
| 49 | return -1; |
| 50 | |
| 51 | memset(desc, 0, sizeof (desc)); |
| 52 | - snprintf(desc, (sensor->id_code & 0x1f) + 1, "%s", sensor->id_string); |
| 53 | + snprintf(desc, sizeof(desc), "%.*s", (sensor->id_code & 0x1f) + 1, sensor->id_string); |
| 54 | |
| 55 | if (verbose) { |
| 56 | printf("Sensor ID : %s (0x%x)\n", |
| 57 | @@ -2164,7 +2164,7 @@ ipmi_sdr_print_sensor_mc_locator(struct ipmi_intf *intf, |
| 58 | return -1; |
| 59 | |
| 60 | memset(desc, 0, sizeof (desc)); |
| 61 | - snprintf(desc, (mc->id_code & 0x1f) + 1, "%s", mc->id_string); |
| 62 | + snprintf(desc, sizeof(desc), "%.*s", (mc->id_code & 0x1f) + 1, mc->id_string); |
| 63 | |
| 64 | if (verbose == 0) { |
| 65 | if (csv_output) |
| 66 | @@ -2257,7 +2257,7 @@ ipmi_sdr_print_sensor_generic_locator(struct ipmi_intf *intf, |
| 67 | char desc[17]; |
| 68 | |
| 69 | memset(desc, 0, sizeof (desc)); |
| 70 | - snprintf(desc, (dev->id_code & 0x1f) + 1, "%s", dev->id_string); |
| 71 | + snprintf(desc, sizeof(desc), "%.*s", (dev->id_code & 0x1f) + 1, dev->id_string); |
| 72 | |
| 73 | if (!verbose) { |
| 74 | if (csv_output) |
| 75 | @@ -2314,7 +2314,7 @@ ipmi_sdr_print_sensor_fru_locator(struct ipmi_intf *intf, |
| 76 | char desc[17]; |
| 77 | |
| 78 | memset(desc, 0, sizeof (desc)); |
| 79 | - snprintf(desc, (fru->id_code & 0x1f) + 1, "%s", fru->id_string); |
| 80 | + snprintf(desc, sizeof(desc), "%.*s", (fru->id_code & 0x1f) + 1, fru->id_string); |
| 81 | |
| 82 | if (!verbose) { |
| 83 | if (csv_output) |
| 84 | @@ -2518,35 +2518,43 @@ ipmi_sdr_print_name_from_rawentry(struct ipmi_intf *intf,uint16_t id, |
| 85 | |
| 86 | int rc =0; |
| 87 | char desc[17]; |
| 88 | + const char *id_string; |
| 89 | + uint8_t id_code; |
| 90 | memset(desc, ' ', sizeof (desc)); |
| 91 | |
| 92 | switch ( type) { |
| 93 | case SDR_RECORD_TYPE_FULL_SENSOR: |
| 94 | record.full = (struct sdr_record_full_sensor *) raw; |
| 95 | - snprintf(desc, (record.full->id_code & 0x1f) +1, "%s", |
| 96 | - (const char *)record.full->id_string); |
| 97 | + id_code = record.full->id_code; |
| 98 | + id_string = record.full->id_string; |
| 99 | break; |
| 100 | + |
| 101 | case SDR_RECORD_TYPE_COMPACT_SENSOR: |
| 102 | record.compact = (struct sdr_record_compact_sensor *) raw ; |
| 103 | - snprintf(desc, (record.compact->id_code & 0x1f) +1, "%s", |
| 104 | - (const char *)record.compact->id_string); |
| 105 | + id_code = record.compact->id_code; |
| 106 | + id_string = record.compact->id_string; |
| 107 | break; |
| 108 | + |
| 109 | case SDR_RECORD_TYPE_EVENTONLY_SENSOR: |
| 110 | record.eventonly = (struct sdr_record_eventonly_sensor *) raw ; |
| 111 | - snprintf(desc, (record.eventonly->id_code & 0x1f) +1, "%s", |
| 112 | - (const char *)record.eventonly->id_string); |
| 113 | - break; |
| 114 | + id_code = record.eventonly->id_code; |
| 115 | + id_string = record.eventonly->id_string; |
| 116 | + break; |
| 117 | + |
| 118 | case SDR_RECORD_TYPE_MC_DEVICE_LOCATOR: |
| 119 | record.mcloc = (struct sdr_record_mc_locator *) raw ; |
| 120 | - snprintf(desc, (record.mcloc->id_code & 0x1f) +1, "%s", |
| 121 | - (const char *)record.mcloc->id_string); |
| 122 | + id_code = record.mcloc->id_code; |
| 123 | + id_string = record.mcloc->id_string; |
| 124 | break; |
| 125 | + |
| 126 | default: |
| 127 | rc = -1; |
| 128 | - break; |
| 129 | - } |
| 130 | + } |
| 131 | + if (!rc) { |
| 132 | + snprintf(desc, sizeof(desc), "%.*s", (id_code & 0x1f) + 1, id_string); |
| 133 | + } |
| 134 | |
| 135 | - lprintf(LOG_INFO, "ID: 0x%04x , NAME: %-16s", id, desc); |
| 136 | + lprintf(LOG_INFO, "ID: 0x%04x , NAME: %-16s", id, desc); |
| 137 | return rc; |
| 138 | } |
| 139 | |
| 140 | -- |
| 141 | 1.9.1 |
| 142 | |