* [PATCH] libdw: check that DWARF strings are null-terminated
@ 2023-02-14 20:00 vvvvvv
2023-02-14 20:30 ` [PATCH v2 0/1] " Aleksei Vetrov
0 siblings, 1 reply; 4+ messages in thread
From: vvvvvv @ 2023-02-14 20:00 UTC (permalink / raw)
To: elfutils-devel; +Cc: kernel-team, maennich, vvvvvv
From: Aleksei Vetrov <vvvvvv@google.com>
It is expected from libdw to return strings that are null-terminated to
avoid overflowing ELF data.
* Add calculation of a safe prefix inside string sections, where any
string will be null-terminated.
* Check if offset overflows the safe prefix in dwarf_formstring.
Signed-off-by: Aleksei Vetrov <vvvvvv@google.com>
---
libdw/dwarf_begin_elf.c | 37 +++++++++++++++++++++++++++++++++++++
libdw/dwarf_formstring.c | 5 ++++-
libdw/libdwP.h | 11 +++++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 8fcef335..76b30a3c 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -70,6 +70,30 @@ static const char dwarf_scnnames[IDX_last][19] =
};
#define ndwarf_scnnames (sizeof (dwarf_scnnames) / sizeof (dwarf_scnnames[0]))
+/* Map from section index to string secton index.
+ Non-string sections should have STR_SCN_IDX_last. */
+static const enum string_secton_index scn_to_string_secton_idx[IDX_last] =
+{
+ [IDX_debug_info] = STR_SCN_IDX_last,
+ [IDX_debug_types] = STR_SCN_IDX_last,
+ [IDX_debug_abbrev] = STR_SCN_IDX_last,
+ [IDX_debug_addr] = STR_SCN_IDX_last,
+ [IDX_debug_aranges] = STR_SCN_IDX_last,
+ [IDX_debug_line] = STR_SCN_IDX_last,
+ [IDX_debug_line_str] = STR_SCN_IDX_debug_line_str,
+ [IDX_debug_frame] = STR_SCN_IDX_last,
+ [IDX_debug_loc] = STR_SCN_IDX_last,
+ [IDX_debug_loclists] = STR_SCN_IDX_last,
+ [IDX_debug_pubnames] = STR_SCN_IDX_last,
+ [IDX_debug_str] = STR_SCN_IDX_debug_str,
+ [IDX_debug_str_offsets] = STR_SCN_IDX_last,
+ [IDX_debug_macinfo] = STR_SCN_IDX_last,
+ [IDX_debug_macro] = STR_SCN_IDX_last,
+ [IDX_debug_ranges] = STR_SCN_IDX_last,
+ [IDX_debug_rnglists] = STR_SCN_IDX_last,
+ [IDX_gnu_debugaltlink] = STR_SCN_IDX_last
+};
+
static enum dwarf_type
scn_dwarf_type (Dwarf *result, size_t shstrndx, Elf_Scn *scn)
{
@@ -230,6 +254,19 @@ check_section (Dwarf *result, size_t shstrndx, Elf_Scn *scn, bool inscngrp)
/* We can now read the section data into results. */
result->sectiondata[cnt] = data;
+ /* If the section contains string data, we want to know a size of a prefix
+ where any string will be null-terminated. */
+ enum string_secton_index string_secton_idx = scn_to_string_secton_idx[cnt];
+ if (string_secton_idx < STR_SCN_IDX_last)
+ {
+ size_t size = data->d_size;
+ /* Reduce the size by the number of non-zero bytes at the end of the
+ section. */
+ while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0')
+ --size;
+ result->string_secton_size[string_secton_idx] = size;
+ }
+
return result;
}
diff --git a/libdw/dwarf_formstring.c b/libdw/dwarf_formstring.c
index c3e892a8..a6f511f8 100644
--- a/libdw/dwarf_formstring.c
+++ b/libdw/dwarf_formstring.c
@@ -61,6 +61,9 @@ dwarf_formstring (Dwarf_Attribute *attrp)
Elf_Data *data = ((attrp->form == DW_FORM_line_strp)
? dbg_ret->sectiondata[IDX_debug_line_str]
: dbg_ret->sectiondata[IDX_debug_str]);
+ size_t data_size = ((attrp->form == DW_FORM_line_strp)
+ ? dbg_ret->string_secton_size[STR_SCN_IDX_debug_line_str]
+ : dbg_ret->string_secton_size[STR_SCN_IDX_debug_str]);
if (data == NULL)
{
__libdw_seterrno ((attrp->form == DW_FORM_line_strp)
@@ -171,7 +174,7 @@ dwarf_formstring (Dwarf_Attribute *attrp)
else
off = read_8ubyte_unaligned (dbg, datap);
- if (off > dbg->sectiondata[IDX_debug_str]->d_size)
+ if (off >= data_size)
goto invalid_offset;
}
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index 961fa4e7..55eb45ed 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -86,6 +86,13 @@ enum
IDX_last
};
+/* Valid indices for the string section's information. */
+enum string_secton_index
+ {
+ STR_SCN_IDX_debug_line_str,
+ STR_SCN_IDX_debug_str,
+ STR_SCN_IDX_last
+ };
/* Error values. */
enum
@@ -169,6 +176,10 @@ struct Dwarf
/* The section data. */
Elf_Data *sectiondata[IDX_last];
+ /* Size of a prefix of string sections, where any string will be
+ null-terminated. */
+ size_t string_secton_size[STR_SCN_IDX_last];
+
/* True if the file has a byte order different from the host. */
bool other_byte_order;
--
2.39.1.581.gbfd45094c4-goog
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 0/1] libdw: check that DWARF strings are null-terminated
2023-02-14 20:00 [PATCH] libdw: check that DWARF strings are null-terminated vvvvvv
@ 2023-02-14 20:30 ` Aleksei Vetrov
2023-02-14 20:30 ` [PATCH v2 1/1] " Aleksei Vetrov
0 siblings, 1 reply; 4+ messages in thread
From: Aleksei Vetrov @ 2023-02-14 20:30 UTC (permalink / raw)
To: vvvvvv; +Cc: elfutils-devel, kernel-team, maennich
Hello,
In the first version of the patch was typo "secton" everywhere.
Reuploading fixed version.
Aleksei Vetrov (1):
libdw: check that DWARF strings are null-terminated
libdw/dwarf_begin_elf.c | 37 +++++++++++++++++++++++++++++++++++++
libdw/dwarf_formstring.c | 5 ++++-
libdw/libdwP.h | 11 +++++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
--
2.39.1.581.gbfd45094c4-goog
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/1] libdw: check that DWARF strings are null-terminated
2023-02-14 20:30 ` [PATCH v2 0/1] " Aleksei Vetrov
@ 2023-02-14 20:30 ` Aleksei Vetrov
2023-02-16 23:45 ` Mark Wielaard
0 siblings, 1 reply; 4+ messages in thread
From: Aleksei Vetrov @ 2023-02-14 20:30 UTC (permalink / raw)
To: vvvvvv; +Cc: elfutils-devel, kernel-team, maennich
It is expected from libdw to return strings that are null-terminated to
avoid overflowing ELF data.
* Add calculation of a safe prefix inside string sections, where any
string will be null-terminated.
* Check if offset overflows the safe prefix in dwarf_formstring.
Signed-off-by: Aleksei Vetrov <vvvvvv@google.com>
---
libdw/dwarf_begin_elf.c | 37 +++++++++++++++++++++++++++++++++++++
libdw/dwarf_formstring.c | 5 ++++-
libdw/libdwP.h | 11 +++++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 8fcef335..1d4bb333 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -70,6 +70,30 @@ static const char dwarf_scnnames[IDX_last][19] =
};
#define ndwarf_scnnames (sizeof (dwarf_scnnames) / sizeof (dwarf_scnnames[0]))
+/* Map from section index to string section index.
+ Non-string sections should have STR_SCN_IDX_last. */
+static const enum string_section_index scn_to_string_section_idx[IDX_last] =
+{
+ [IDX_debug_info] = STR_SCN_IDX_last,
+ [IDX_debug_types] = STR_SCN_IDX_last,
+ [IDX_debug_abbrev] = STR_SCN_IDX_last,
+ [IDX_debug_addr] = STR_SCN_IDX_last,
+ [IDX_debug_aranges] = STR_SCN_IDX_last,
+ [IDX_debug_line] = STR_SCN_IDX_last,
+ [IDX_debug_line_str] = STR_SCN_IDX_debug_line_str,
+ [IDX_debug_frame] = STR_SCN_IDX_last,
+ [IDX_debug_loc] = STR_SCN_IDX_last,
+ [IDX_debug_loclists] = STR_SCN_IDX_last,
+ [IDX_debug_pubnames] = STR_SCN_IDX_last,
+ [IDX_debug_str] = STR_SCN_IDX_debug_str,
+ [IDX_debug_str_offsets] = STR_SCN_IDX_last,
+ [IDX_debug_macinfo] = STR_SCN_IDX_last,
+ [IDX_debug_macro] = STR_SCN_IDX_last,
+ [IDX_debug_ranges] = STR_SCN_IDX_last,
+ [IDX_debug_rnglists] = STR_SCN_IDX_last,
+ [IDX_gnu_debugaltlink] = STR_SCN_IDX_last
+};
+
static enum dwarf_type
scn_dwarf_type (Dwarf *result, size_t shstrndx, Elf_Scn *scn)
{
@@ -230,6 +254,19 @@ check_section (Dwarf *result, size_t shstrndx, Elf_Scn *scn, bool inscngrp)
/* We can now read the section data into results. */
result->sectiondata[cnt] = data;
+ /* If the section contains string data, we want to know a size of a prefix
+ where any string will be null-terminated. */
+ enum string_section_index string_section_idx = scn_to_string_section_idx[cnt];
+ if (string_section_idx < STR_SCN_IDX_last)
+ {
+ size_t size = data->d_size;
+ /* Reduce the size by the number of non-zero bytes at the end of the
+ section. */
+ while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0')
+ --size;
+ result->string_section_size[string_section_idx] = size;
+ }
+
return result;
}
diff --git a/libdw/dwarf_formstring.c b/libdw/dwarf_formstring.c
index c3e892a8..0ee42411 100644
--- a/libdw/dwarf_formstring.c
+++ b/libdw/dwarf_formstring.c
@@ -61,6 +61,9 @@ dwarf_formstring (Dwarf_Attribute *attrp)
Elf_Data *data = ((attrp->form == DW_FORM_line_strp)
? dbg_ret->sectiondata[IDX_debug_line_str]
: dbg_ret->sectiondata[IDX_debug_str]);
+ size_t data_size = ((attrp->form == DW_FORM_line_strp)
+ ? dbg_ret->string_section_size[STR_SCN_IDX_debug_line_str]
+ : dbg_ret->string_section_size[STR_SCN_IDX_debug_str]);
if (data == NULL)
{
__libdw_seterrno ((attrp->form == DW_FORM_line_strp)
@@ -171,7 +174,7 @@ dwarf_formstring (Dwarf_Attribute *attrp)
else
off = read_8ubyte_unaligned (dbg, datap);
- if (off > dbg->sectiondata[IDX_debug_str]->d_size)
+ if (off >= data_size)
goto invalid_offset;
}
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index 961fa4e7..5cbdc279 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -86,6 +86,13 @@ enum
IDX_last
};
+/* Valid indices for the string section's information. */
+enum string_section_index
+ {
+ STR_SCN_IDX_debug_line_str,
+ STR_SCN_IDX_debug_str,
+ STR_SCN_IDX_last
+ };
/* Error values. */
enum
@@ -169,6 +176,10 @@ struct Dwarf
/* The section data. */
Elf_Data *sectiondata[IDX_last];
+ /* Size of a prefix of string sections, where any string will be
+ null-terminated. */
+ size_t string_section_size[STR_SCN_IDX_last];
+
/* True if the file has a byte order different from the host. */
bool other_byte_order;
--
2.39.1.581.gbfd45094c4-goog
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] libdw: check that DWARF strings are null-terminated
2023-02-14 20:30 ` [PATCH v2 1/1] " Aleksei Vetrov
@ 2023-02-16 23:45 ` Mark Wielaard
0 siblings, 0 replies; 4+ messages in thread
From: Mark Wielaard @ 2023-02-16 23:45 UTC (permalink / raw)
To: Aleksei Vetrov; +Cc: elfutils-devel, kernel-team, maennich
Hi Aleksei,
On Tue, Feb 14, 2023 at 08:30:02PM +0000, Aleksei Vetrov via Elfutils-devel wrote:
> It is expected from libdw to return strings that are null-terminated to
> avoid overflowing ELF data.
>
> * Add calculation of a safe prefix inside string sections, where any
> string will be null-terminated.
>
> * Check if offset overflows the safe prefix in dwarf_formstring.
This is a very nice sanity/hardening fix.
> + /* If the section contains string data, we want to know a size of a prefix
> + where any string will be null-terminated. */
> + enum string_section_index string_section_idx = scn_to_string_section_idx[cnt];
> + if (string_section_idx < STR_SCN_IDX_last)
> + {
> + size_t size = data->d_size;
> + /* Reduce the size by the number of non-zero bytes at the end of the
> + section. */
> + while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0')
> + --size;
> + result->string_section_size[string_section_idx] = size;
> + }
Checking from the end is smart. In the normal case the debug string
section will end with a zero terminator (or zero padding), so this
should be really quick.
> @@ -171,7 +174,7 @@ dwarf_formstring (Dwarf_Attribute *attrp)
> else
> off = read_8ubyte_unaligned (dbg, datap);
>
> - if (off > dbg->sectiondata[IDX_debug_str]->d_size)
> + if (off >= data_size)
> goto invalid_offset;
> }
O, the original check was actually one off.
Looks good. Pushed as is.
Thanks,
Mark
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-16 23:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 20:00 [PATCH] libdw: check that DWARF strings are null-terminated vvvvvv
2023-02-14 20:30 ` [PATCH v2 0/1] " Aleksei Vetrov
2023-02-14 20:30 ` [PATCH v2 1/1] " Aleksei Vetrov
2023-02-16 23:45 ` Mark Wielaard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).