b.liu | e958203 | 2025-04-17 19:18:16 +0800 | [diff] [blame^] | 1 | From 98b47424cf548f58c4d295fa8235210406ea85ca 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 11/11] 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 | lib/ipmi_fru.c | 2 +- |
| 23 | lib/ipmi_sdr.c | 40 ++++++++++++++++++++++++---------------- |
| 24 | 2 files changed, 25 insertions(+), 17 deletions(-) |
| 25 | |
| 26 | --- a/lib/ipmi_fru.c |
| 27 | +++ b/lib/ipmi_fru.c |
| 28 | @@ -3062,7 +3062,7 @@ ipmi_fru_print(struct ipmi_intf * intf, |
| 29 | return 0; |
| 30 | |
| 31 | memset(desc, 0, sizeof(desc)); |
| 32 | - memcpy(desc, fru->id_string, fru->id_code & 0x01f); |
| 33 | + memcpy(desc, fru->id_string, __min(fru->id_code & 0x01f, sizeof(desc))); |
| 34 | desc[fru->id_code & 0x01f] = 0; |
| 35 | printf("FRU Device Description : %s (ID %d)\n", desc, fru->device_id); |
| 36 | |
| 37 | --- a/lib/ipmi_sdr.c |
| 38 | +++ b/lib/ipmi_sdr.c |
| 39 | @@ -2084,7 +2084,7 @@ ipmi_sdr_print_sensor_eventonly(struct i |
| 40 | return -1; |
| 41 | |
| 42 | memset(desc, 0, sizeof (desc)); |
| 43 | - snprintf(desc, (sensor->id_code & 0x1f) + 1, "%s", sensor->id_string); |
| 44 | + snprintf(desc, sizeof(desc), "%.*s", (sensor->id_code & 0x1f) + 1, sensor->id_string); |
| 45 | |
| 46 | if (verbose) { |
| 47 | printf("Sensor ID : %s (0x%x)\n", |
| 48 | @@ -2135,7 +2135,7 @@ ipmi_sdr_print_sensor_mc_locator(struct |
| 49 | return -1; |
| 50 | |
| 51 | memset(desc, 0, sizeof (desc)); |
| 52 | - snprintf(desc, (mc->id_code & 0x1f) + 1, "%s", mc->id_string); |
| 53 | + snprintf(desc, sizeof(desc), "%.*s", (mc->id_code & 0x1f) + 1, mc->id_string); |
| 54 | |
| 55 | if (verbose == 0) { |
| 56 | if (csv_output) |
| 57 | @@ -2228,7 +2228,7 @@ ipmi_sdr_print_sensor_generic_locator(st |
| 58 | char desc[17]; |
| 59 | |
| 60 | memset(desc, 0, sizeof (desc)); |
| 61 | - snprintf(desc, (dev->id_code & 0x1f) + 1, "%s", dev->id_string); |
| 62 | + snprintf(desc, sizeof(desc), "%.*s", (dev->id_code & 0x1f) + 1, dev->id_string); |
| 63 | |
| 64 | if (!verbose) { |
| 65 | if (csv_output) |
| 66 | @@ -2285,7 +2285,7 @@ ipmi_sdr_print_sensor_fru_locator(struct |
| 67 | char desc[17]; |
| 68 | |
| 69 | memset(desc, 0, sizeof (desc)); |
| 70 | - snprintf(desc, (fru->id_code & 0x1f) + 1, "%s", fru->id_string); |
| 71 | + snprintf(desc, sizeof(desc), "%.*s", (fru->id_code & 0x1f) + 1, fru->id_string); |
| 72 | |
| 73 | if (!verbose) { |
| 74 | if (csv_output) |
| 75 | @@ -2489,35 +2489,43 @@ ipmi_sdr_print_name_from_rawentry(struct |
| 76 | |
| 77 | int rc =0; |
| 78 | char desc[17]; |
| 79 | + const char *id_string; |
| 80 | + uint8_t id_code; |
| 81 | memset(desc, ' ', sizeof (desc)); |
| 82 | |
| 83 | switch ( type) { |
| 84 | case SDR_RECORD_TYPE_FULL_SENSOR: |
| 85 | record.full = (struct sdr_record_full_sensor *) raw; |
| 86 | - snprintf(desc, (record.full->id_code & 0x1f) +1, "%s", |
| 87 | - (const char *)record.full->id_string); |
| 88 | + id_code = record.full->id_code; |
| 89 | + id_string = record.full->id_string; |
| 90 | break; |
| 91 | + |
| 92 | case SDR_RECORD_TYPE_COMPACT_SENSOR: |
| 93 | record.compact = (struct sdr_record_compact_sensor *) raw ; |
| 94 | - snprintf(desc, (record.compact->id_code & 0x1f) +1, "%s", |
| 95 | - (const char *)record.compact->id_string); |
| 96 | + id_code = record.compact->id_code; |
| 97 | + id_string = record.compact->id_string; |
| 98 | break; |
| 99 | + |
| 100 | case SDR_RECORD_TYPE_EVENTONLY_SENSOR: |
| 101 | record.eventonly = (struct sdr_record_eventonly_sensor *) raw ; |
| 102 | - snprintf(desc, (record.eventonly->id_code & 0x1f) +1, "%s", |
| 103 | - (const char *)record.eventonly->id_string); |
| 104 | - break; |
| 105 | + id_code = record.eventonly->id_code; |
| 106 | + id_string = record.eventonly->id_string; |
| 107 | + break; |
| 108 | + |
| 109 | case SDR_RECORD_TYPE_MC_DEVICE_LOCATOR: |
| 110 | record.mcloc = (struct sdr_record_mc_locator *) raw ; |
| 111 | - snprintf(desc, (record.mcloc->id_code & 0x1f) +1, "%s", |
| 112 | - (const char *)record.mcloc->id_string); |
| 113 | + id_code = record.mcloc->id_code; |
| 114 | + id_string = record.mcloc->id_string; |
| 115 | break; |
| 116 | + |
| 117 | default: |
| 118 | rc = -1; |
| 119 | - break; |
| 120 | - } |
| 121 | + } |
| 122 | + if (!rc) { |
| 123 | + snprintf(desc, sizeof(desc), "%.*s", (id_code & 0x1f) + 1, id_string); |
| 124 | + } |
| 125 | |
| 126 | - lprintf(LOG_INFO, "ID: 0x%04x , NAME: %-16s", id, desc); |
| 127 | + lprintf(LOG_INFO, "ID: 0x%04x , NAME: %-16s", id, desc); |
| 128 | return rc; |
| 129 | } |
| 130 | |