[RFC] gdb, configure: Add disable-readers option for configure

  This is a proof-of-concept of a feature we've discussed internally for a
bit of time, where users would be able to select debug information
formats to not be readable by GDB. This comes from a security concern,
since some readers are pretty old and haven't been touched in a while,
but if they have vulnerabilities, they would make every GDB vulnerable.
This makes it so downstreams don't have to scramble to fix every
security issue, even for things that are extremely unlikely to be used.

I'm not sure I like the way this selection works yet. The ideal way this
features works for me is if we could say --disable-readers=foo,bar, however
autoconf will always treat --disable-option as being an alias for
--enable-option=no, so its impossible to do it that way. The choices then
become either using --enable-readers (the approach of this patch) or adding
a switch for every reader that can be disabled, which could pollute the
help text. If anyone has opinions on which is best - or knows of an even
better way to do this - I'm all ears. This is my first patch dealing
with autotools after all...

I'm also sending this as an RFC to see if people think this is a useful
feature, before I go on to disentangle the many readers and allow for
separate compilation, just in case this turns out to be a doomed project
:-)

---

GDB has support for many debug information formats, some which might be
very unlikely to be found in some situations (such as the COFF format in
linux). This commit introduces the option for a user to choose which
formats GDB will support at build configuration time.

This is especially useful to avoid possible security concerns with
readers that aren't expected to be used at all, as debug info is one of
the simplest vectors for an attacker to try and hit GDB with.  This
change also can reduce the size of the final binary, if that is a
concern.

In this patch, most readers are still considered mandatory as there is
interdependence between what should be independent formats, but future
patches should remove this interdependence and allow all the listed
debug format to be independently enabled or disabled.

The only reader that can be safely disabled at this point is xcoff,
which shows no regressions in my fedora 40 machine.
---
 gdb/Makefile.in      | 76 ++++++++++++++++++++++++--------------------
 gdb/README           |  3 ++
 gdb/configure        | 50 +++++++++++++++++++++++++++--
 gdb/configure.ac     | 37 +++++++++++++++++++++
 gdb/configure.reader | 51 +++++++++++++++++++++++++++++
 5 files changed, 181 insertions(+), 36 deletions(-)
 create mode 100644 gdb/configure.reader
  

On 2024-08-26 13:45, Guinevere Larsen wrote:
> This is a proof-of-concept of a feature we've discussed internally for a
> bit of time, where users would be able to select debug information
> formats to not be readable by GDB. This comes from a security concern,
> since some readers are pretty old and haven't been touched in a while,
> but if they have vulnerabilities, they would make every GDB vulnerable.
> This makes it so downstreams don't have to scramble to fix every
> security issue, even for things that are extremely unlikely to be used.
> 
> I'm not sure I like the way this selection works yet. The ideal way this
> features works for me is if we could say --disable-readers=foo,bar, however
> autoconf will always treat --disable-option as being an alias for
> --enable-option=no, so its impossible to do it that way. The choices then
> become either using --enable-readers (the approach of this patch) or adding
> a switch for every reader that can be disabled, which could pollute the
> help text. If anyone has opinions on which is best - or knows of an even
> better way to do this - I'm all ears. This is my first patch dealing
> with autotools after all...
> 
> I'm also sending this as an RFC to see if people think this is a useful
> feature, before I go on to disentangle the many readers and allow for
> separate compilation, just in case this turns out to be a doomed project
> :-)
> 
> ---

I think you should not put your meta-comment (or whathever that's
called, the comment that you want to send to the list but not want to be
part of the commit message) before `---`, because git-am strips what
follows `---`, so we effectively lose your commit message when applying
your patch.  I think the idea is that you're supposed to put your
meta-comment after this `---`, so that it does get removed by git-am.  I
think it's fine for the "new in v2" kind of messages, but in your case
I'd rather see the context before reading the commit message, so I don't
know what's the best way.  We could perhaps find and settle on a
convention for this particular case.

> GDB has support for many debug information formats, some which might be
> very unlikely to be found in some situations (such as the COFF format in
> linux). This commit introduces the option for a user to choose which
> formats GDB will support at build configuration time.
> 
> This is especially useful to avoid possible security concerns with
> readers that aren't expected to be used at all, as debug info is one of
> the simplest vectors for an attacker to try and hit GDB with.  This
> change also can reduce the size of the final binary, if that is a
> concern.
> 
> In this patch, most readers are still considered mandatory as there is
> interdependence between what should be independent formats, but future
> patches should remove this interdependence and allow all the listed
> debug format to be independently enabled or disabled.
> 
> The only reader that can be safely disabled at this point is xcoff,
> which shows no regressions in my fedora 40 machine.

I think the idea and the justification are good.  If I understand
correctly from the description above, the default is to include all
debug info readers, but as soon as you use the --enable-readers=...
option, then only the specified readers are enabled (an "additive"
option)?  This seems fine to me for the "security conscious" folks,
since it makes it so that you only enabled the debug info you asked for.
If a new one gets added, it won't get enable without an action from your
part.

So, I'd be fine without a "subtractive" option (start with all enabled
and remove the specified ones).  But if you really wanted to make one,
one way would be to go with a variable (AC_ARG_VAR), although that would
be inconsistent with how we do things today.

Once it is agreed that it is a direction we'd like to take, I would
prefer if we did the work to make all debug info readers optional prior
to merging, so that we won't have this weird state of have some debug
info formats that are mandatory.  I know that we use some DWARF stuff in
some core structures, that will probably need to be sorted out, but it's
probably for good.  It will help avoid this kind of abstraction leak.

Simon
  

On 8/26/24 11:58 PM, Simon Marchi wrote:
>
> On 2024-08-26 13:45, Guinevere Larsen wrote:
>> This is a proof-of-concept of a feature we've discussed internally for a
>> bit of time, where users would be able to select debug information
>> formats to not be readable by GDB. This comes from a security concern,
>> since some readers are pretty old and haven't been touched in a while,
>> but if they have vulnerabilities, they would make every GDB vulnerable.
>> This makes it so downstreams don't have to scramble to fix every
>> security issue, even for things that are extremely unlikely to be used.
>>
>> I'm not sure I like the way this selection works yet. The ideal way this
>> features works for me is if we could say --disable-readers=foo,bar, however
>> autoconf will always treat --disable-option as being an alias for
>> --enable-option=no, so its impossible to do it that way. The choices then
>> become either using --enable-readers (the approach of this patch) or adding
>> a switch for every reader that can be disabled, which could pollute the
>> help text. If anyone has opinions on which is best - or knows of an even
>> better way to do this - I'm all ears. This is my first patch dealing
>> with autotools after all...
>>
>> I'm also sending this as an RFC to see if people think this is a useful
>> feature, before I go on to disentangle the many readers and allow for
>> separate compilation, just in case this turns out to be a doomed project
>> :-)
>>
>> ---
> I think you should not put your meta-comment (or whathever that's
> called, the comment that you want to send to the list but not want to be
> part of the commit message) before `---`, because git-am strips what
> follows `---`, so we effectively lose your commit message when applying
> your patch.  I think the idea is that you're supposed to put your
> meta-comment after this `---`, so that it does get removed by git-am.  I
> think it's fine for the "new in v2" kind of messages, but in your case
> I'd rather see the context before reading the commit message, so I don't
> know what's the best way.  We could perhaps find and settle on a
> convention for this particular case.
Ah, sorry. I thought that this time it made sense to have the meta 
explanation before the more technical information, and I thought I had 
seen in the past git am not stripping meta comments, so I thought this 
was fine. I'll keep my meta comments to after the commit messsages from 
now on.
>
>> GDB has support for many debug information formats, some which might be
>> very unlikely to be found in some situations (such as the COFF format in
>> linux). This commit introduces the option for a user to choose which
>> formats GDB will support at build configuration time.
>>
>> This is especially useful to avoid possible security concerns with
>> readers that aren't expected to be used at all, as debug info is one of
>> the simplest vectors for an attacker to try and hit GDB with.  This
>> change also can reduce the size of the final binary, if that is a
>> concern.
>>
>> In this patch, most readers are still considered mandatory as there is
>> interdependence between what should be independent formats, but future
>> patches should remove this interdependence and allow all the listed
>> debug format to be independently enabled or disabled.
>>
>> The only reader that can be safely disabled at this point is xcoff,
>> which shows no regressions in my fedora 40 machine.
> I think the idea and the justification are good.  If I understand
> correctly from the description above, the default is to include all
> debug info readers, but as soon as you use the --enable-readers=...
> option, then only the specified readers are enabled (an "additive"
> option)?  This seems fine to me for the "security conscious" folks,
> since it makes it so that you only enabled the debug info you asked for.
> If a new one gets added, it won't get enable without an action from your
> part.
You did understand it correctly. When you put it that way, that nothing 
will silently be enabled by GDB updates, using --enable does sound like 
a better choice.
>
> So, I'd be fine without a "subtractive" option (start with all enabled
> and remove the specified ones).  But if you really wanted to make one,
> one way would be to go with a variable (AC_ARG_VAR), although that would
> be inconsistent with how we do things today.
>
> Once it is agreed that it is a direction we'd like to take, I would
> prefer if we did the work to make all debug info readers optional prior
> to merging, so that we won't have this weird state of have some debug
> info formats that are mandatory.  I know that we use some DWARF stuff in
> some core structures, that will probably need to be sorted out, but it's
> probably for good.  It will help avoid this kind of abstraction leak.

I planned on sending this as the first patch of a series, and subsequent 
patches would introduce code changes required to be able to disable 
other readers.

Would you prefer if I instead sent separate patches/series disentangling 
the code, and once everything is merged I send a final patch with the 
configure changes?
  

On 2024-08-27 06:25, Guinevere Larsen wrote:
> I planned on sending this as the first patch of a series, and
> subsequent patches would introduce code changes required to be able to
> disable other readers.
> 
> Would you prefer if I instead sent separate patches/series
> disentangling the code, and once everything is merged I send a final
> patch with the configure changes?

I would imagine a series that makes any necessary adjustments to the
code first, and the last patch adding the configure machinery to make
all readers optional.  But I don't know the amount of work it
represents, so it's not a hard requirement.  Just that when we leave
things as "future work", we tend not to do it, so it's preferable to do
it right the first time, when possible.

Simon
  

On 8/27/24 11:06 AM, Simon Marchi wrote:
>
> On 2024-08-27 06:25, Guinevere Larsen wrote:
>> I planned on sending this as the first patch of a series, and
>> subsequent patches would introduce code changes required to be able to
>> disable other readers.
>>
>> Would you prefer if I instead sent separate patches/series
>> disentangling the code, and once everything is merged I send a final
>> patch with the configure changes?
> I would imagine a series that makes any necessary adjustments to the
> code first, and the last patch adding the configure machinery to make
> all readers optional.  But I don't know the amount of work it
> represents, so it's not a hard requirement.  Just that when we leave
> things as "future work", we tend not to do it, so it's preferable to do
> it right the first time, when possible.

I've been looking into this some more, and talked to Andrew a bit, and I 
think this patch (and gdb in general) muddles 2 different concepts.

There are file format readers, and there are debug information readers. 
Both are named <something>reader.c (except for the dwarf2 one), but the 
ones that deal with file formats register a symtab_fns struct, while the 
ones that deal with only debug information don't - and their functions 
get called directly by file format readers.

I'm spitting this project into 2, one that disables file formats, and 
the other that disables debug info support even if the file format would 
support it. I'll send them in this order, since file formats are much 
easier - we should have all the infrastructure in place from symtab_fns, 
so it's mostly disentangling debug info from file format reading.