Re: [PATCH] PR28204, debuginfod IMA
Hi Frank, On Fri, 2023-10-27 at 15:15 -0400, Frank Ch. Eigler wrote: > > > I would not expect the emailed patch to apply, esp. with all the other > > > work done in the intermediate months, which is why the code is also in > > > the git branch. The binary files do not seem effectively reviewable > > > anyway. > > > > It would be really convenient though. And modern git format-patch does > > includes base tree information which allows tools to stich commits at > > the right place. > > (I would be surprised if many-month-old patches could just be > automatically "stitched".) > > > It would also help with patchwork and pre-commit CI. > > https://git-scm.com/docs/git-format-patch#_base_tree_information > > Considering how easily the trybots can process the actual code - and > have done so before posting the patch for review - we can consider > some CI well done already. After approval but before merge, it would > undergo another round of trybotting. With such workflow, patchwork > does not need to concern itself with additional pre-commit CI/CD. My point is really that posting with git format-patch or send-email makes it possible for someone to simply use git am, b4 or git pw to try out a patch. If the patch doesn't apply then that will be the first review issue. > > > > [...] > > > > > The default is ima:permissive mode, which allows signatures to > > > > > function like a checksum to detect accidental corruption, but > > > > > accepts > > > > > operation in a mix of signed and unsigned packages & servers. > > > > > > > > I still think "permissive" is confusing. Since it is a term also used > > > > by e.g. selinux, but doesn't work that way. And it doesn't seem > > > > connected with the threat-model that enforcing protects against. > > > > > > The connection is the following: > > > "enforcing" mode protects against accidental or deliberate (MITM) > > > corruption. > > > "permissive" mode protects against accidental corruption. > > > > > > > Since it is a different concept maybe it shouldn't be part of this > > > > patch. It is a form of integrity checking, but doesn't protect (or > > > > warns) about integrity failures. > > > > > > It does protect and warn against integrity failures of the form of > > > incorrect signatures. > > > My issue is that I don't really understand "permissive". Originally I > > assumed it was like selinux permissive mode, it does do the checks, > > but if they fail we just warn and continue. That seems a clear concept. > > The proposed documentation explains it thusly: > > ima:enforcing Every downloaded file requires a valid signature. > > ima:permissive Every downloaded file accompanied by a signature > must be valid, but downloads without signatures are accepted. > > ima:ignore Skips verification altogether. > > You're right that it is not an exact match for the selinux concept. > But if one's not hunting around for a precise analogy, and just reads > the single sentence description, it tries to be clear. > > > [...] if there is a signature, but we don't have the corresponding > > certificate to check it against, should it still fail, or is it > > more like a none-signed file and we can be "permissive" and accept > > it? Maybe I don't have enough imagination. > > I see your point. One could make an argument either way, coming from > fuzziness with the concept of an "invalid signature". One could > clarify with a rewording to "known-invalid". Then "permissive" > becomes permit everything except known-invalid files. Missing > certificates would not qualify as known-invalid, merely unknown. > Would you like me to draft up a sentence or two description of that > concept for the man page? > > The intended benefit of permissive mode as a default is to give > elfutils users as much reassurance possible, without requiring manual > configuration changes or manual downloads. See also the certificate > distribution topic below - it's really toward the same goal. I think my issue is really that it is unclear what "reassurances" we are making. What is the threat-model? Permissive says to me that although checks are done, they don't block receiving files. Maybe calling it ima:known-valid ? ima:checking (if you reject unknown- valid)? I don't have an issue with ima:enforcing, that seems to have clear semantics. The threat-model is clear, you only want to get files that are signed by a specific set of keys/certificates you trust. No middle- person acting as an intermediary between the distributor and user can circumvent that. > > [...] > > > Yes, it is odd. Unfortunately, it is not possible to enforce crypto > > > signatures from distros upon unsigned slices. A couple of possible > > > solutions: > > > [...] > > > - disable section queries from enforcing-mode servers (which could > > > then nuke gdbindex capability for e.g. future fedora/gdb users) > > [...] > > > > I think only option 2 makes sense given the enforcing threat-model
Re: [PATCH] PR28204, debuginfod IMA
Hi, Mark - > > Considering how easily the trybots can process the actual code - and > > have done so before posting the patch for review - we can consider > > some CI well done already. After approval but before merge, it would > > undergo another round of trybotting. With such workflow, patchwork > > does not need to concern itself with additional pre-commit CI/CD. > > My point is really that posting with git format-patch or send-email > makes it possible for someone to simply use git am, b4 or git pw to try > out a patch. If the patch doesn't apply then that will be the first > review issue. I see what you mean, but maybe that puts the cart ahead of the horse. What's desirable is easy access to a runnable version of the patch, not the choice of command line tooling to do that. A published git branch containing the same patch is even simpler than using git am/b4/pw. git is the most convenient transport protocol for patches. > I think my issue is really that it is unclear what "reassurances" we > are making. What is the threat-model? Permissive says to me that > although checks are done, they don't block receiving files. [...] In the current version of the users/fche/try-bz28204 branch, this has been renamed "optimistic", and the man page blurb now says: \fIima:optimistic\fP Every downloaded file with a known-invalid signature is rejected, protecting against some types of corruption. > > They already make the decision whom they download debuginfo from. > > That's literally what setting $DEBUGINFOD_URLS is. The scenario > > you're describing would be trusting a server enough to supply content, > > trusting our code to fetch & check that content, but not trusting us > > to redistribute public certificates for the servers. > > The user shouldn't trust a middle-person. Unless we are signing those > files we shouldn't distribute the certificates are being trustable > imho. We sign our elfutils releases, and packagers often sign their builds of our releases, which users can verify. > So you propose we setup a curating process to decide which certificates > to include? Sure. We already curate a set of debuginfod servers. > If we do then I would suggest we create a separate package for that > (or just a separate tar ball or repository). [...] Another compromise possibility is to keep shipping these certificates, but not include them in the default $DEBUGINFOD_IMA_CERT_PATH. A user wishing to trust our curation can add /etc/debuginfod/ima-certs. > > > I do understand hex2dec, but I don't understand what extract_skid > > > does. Maybe add an explanation what a certificates subject key id is > > > and why we need it. It's a brief fingerprint/hash of the key, convenient for identification in diagnostics. > > > [...] > > > > Not sure, but this is how libimaevm.c similar code does it. I assume > > > > the first byte of the signature is something else. > > > > (https://git.code.sf.net/p/linux-ima/ima-evm-utils) > > > > > > ewww. Does this pass ubsan (--enable-sanitize-undefined)? > > > > Haven't tried but it passes valgrind. > > valgrind doesn't check for undefined behavior or type alignment > requirements. An elfutils build with --enable-sanitize-undefined had no complaints. > > OK, will take a look at that. What other global-state conflicting > > things did you notice? > > A quick look at the code shows that various functions can read/write a > static public_keys variable linked list, including (indirectly) > ima_verify_signature. So that can cause data-races. [...] OK, added a mutex around the entire function. > One other issue I noticed is that it seems to be distributed under > GPLv2-only. Which seems to mean that anything based on it should also > be distributed under GPLv2-only, which would include libdebuginfod. Is > there code we can rely on that is GPLv2+ and LGPLv3+ compatible? Ugh. I don't know of an alternative. There isn't an equivalent command line wrapping of the library either (with respect to certificate searching) that we could fork out to. (OTOH, GPLv2 is compatible with GPLv2+.) > > openssl's initialization is fine & thread-safe in practice, despite > > the documentation's warnings. > > OK. But even if it is thread-safe, you also need to make sure it inits > the same. [...] Interesting issue. One openssl initialization call is deep inside libcurl, another one in libimaevm's solib ctor. Neither is parametrized such that we could influence them. However, things are working(tm). - FChE
[Bug libdw/30967] Discriminator in Dwarf_Line_s may exceed 24 bits
https://sourceware.org/bugzilla/show_bug.cgi?id=30967 --- Comment #12 from Aleksei Vetrov --- (In reply to Mark Wielaard from comment #6) > So my preferred workaround: > https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=really-large-disc Do you have plan to merge it to master? -- You are receiving this mail because: You are on the CC list for the bug.
[PATCH] readelf: Support .gdb_index version 9
Hi,
I'd like to merge this patch before the next release. Unless anyone objects
I'll merge it by Friday Nov 3.
Commit message:
Version 9 adds a "shortcut table" to the index. The shortcut table contains
the name and language of the main function, if it exists.
A testcase added in this patch uses an executable written with Fortran.
This is because gdb does not currently populate the shortcut table of
C/C++ programs (see sourceware PR30996).
Signed-off-by: Aaron Merey
---
src/readelf.c | 66 +++-
tests/Makefile.am | 3 +-
tests/run-readelf-gdb_index.sh | 95 +++-
tests/testfilegdbindex9-no-maininfo.bz2 | Bin 0 -> 3502 bytes
tests/testfilegdbindex9.bz2 | Bin 0 -> 4266 bytes
5 files changed, 159 insertions(+), 5 deletions(-)
create mode 100755 tests/testfilegdbindex9-no-maininfo.bz2
create mode 100755 tests/testfilegdbindex9.bz2
diff --git a/src/readelf.c b/src/readelf.c
index db31ad09..a28f6236 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -11539,8 +11539,9 @@ print_gdb_index_section (Dwfl_Module *dwflmod, Ebl *ebl,
// hash used for generating the table. Version 6 contains symbols
// for inlined functions, older versions didn't. Version 7 adds
// symbol kinds. Version 8 just indicates that it correctly includes
- // TUs for symbols.
- if (vers < 4 || vers > 8)
+ // TUs for symbols. Version 9 adds shortcut table for information
+ // regarding the main function.
+ if (vers < 4 || vers > 9)
{
printf (_(" unknown version, cannot parse section\n"));
return;
@@ -11578,6 +11579,17 @@ print_gdb_index_section (Dwfl_Module *dwflmod, Ebl
*ebl,
if (unlikely (readp + 4 > dataend))
goto invalid_data;
+ uint32_t shortcut_off = 0;
+ if (vers >= 9)
+{
+ shortcut_off = read_4ubyte_unaligned (dbg, readp);
+ printf (_(" shortcut offset: %#" PRIx32 "\n"), shortcut_off);
+
+ readp += 4;
+ if (unlikely (readp + 4 > dataend))
+ goto invalid_data;
+}
+
uint32_t const_off = read_4ubyte_unaligned (dbg, readp);
printf (_(" constant offset: %#" PRIx32 "\n"), const_off);
@@ -11675,8 +11687,19 @@ print_gdb_index_section (Dwfl_Module *dwflmod, Ebl
*ebl,
if (const_off >= data->d_size)
goto invalid_data;
+ const unsigned char *shortcut_start = NULL;
+ if (vers >= 9)
+{
+ if (shortcut_off >= data->d_size)
+ goto invalid_data;
+
+ shortcut_start = data->d_buf + shortcut_off;
+ nextp = shortcut_start;
+}
+ else
+nextp = const_start;
+
readp = data->d_buf + sym_off;
- nextp = const_start;
size_t sym_nr = (nextp - readp) / 8;
printf (_("\n Symbol table at offset %#" PRIx32
@@ -11750,6 +11773,43 @@ print_gdb_index_section (Dwfl_Module *dwflmod, Ebl
*ebl,
}
n++;
}
+
+ if (vers < 9)
+return;
+
+ if (unlikely (shortcut_start == NULL))
+goto invalid_data;
+
+ readp = shortcut_start;
+ nextp = const_start;
+ size_t shortcut_nr = (nextp - readp) / 4;
+
+ if (unlikely (shortcut_nr != 2))
+goto invalid_data;
+
+ printf (_("\nShortcut table at offset %#" PRIx32 " contains %zu slots:\n"),
+ shortcut_off, shortcut_nr);
+
+ uint32_t lang = read_4ubyte_unaligned (dbg, readp);
+ readp += 4;
+
+ printf (_("Language of main: %s\n"), dwarf_lang_name (lang));
+ printf (_("Name of main: "));
+
+ if (lang != 0)
+{
+ uint32_t name = read_4ubyte_unaligned (dbg, readp);
+ readp += 4;
+ const unsigned char *sym = const_start + name;
+
+ if (unlikely ((size_t) (dataend - const_start) < name
+ || memchr (sym, '\0', dataend - sym) == NULL))
+ goto invalid_data;
+
+ printf ("%s\n", sym);
+}
+ else
+printf ("\n");
}
/* Returns true and sets split DWARF CU id if there is a split compile
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ef5b6bb5..e8bc9058 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -421,7 +421,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
run-readelf-Dd.sh \
testfile-s390x-hash-both.bz2 \
run-readelf-gdb_index.sh testfilegdbindex5.bz2 \
-testfilegdbindex7.bz2 \
+testfilegdbindex7.bz2 testfilegdbindex9.bz2 \
+testfilegdbindex9-no-maininfo.bz2 \
run-readelf-s.sh testfilebazdbg.bz2 testfilebazdyn.bz2 \
testfilebazmin.bz2 testfilebazdbg.debug.bz2 testfilebazmdb.bz2 \
testfilebaztab.bz2 testfilebasmin.bz2 testfilebaxmin.bz2 \
diff --git a/tests/run-readelf-gdb_index.sh b/tests/run-readelf-gdb_index.sh
index fcbc3c57..95367ef8 100755
--- a/tests/run-readelf-gdb_index.sh
+++ b/tests/run-readelf-gdb_index.sh
@@ -63,7 +63,7 @@
# (gdb) save gdb-index .
# objcopy --add-section .gdb_index=testfilegdbindex7.gdb-index
--set-section-flags .gdb_index=readonly testfilegdbindex7 testfilegdbindex7
-testfiles testfilegdbindex5 testfilegdbin
[Bug libdw/30967] Discriminator in Dwarf_Line_s may exceed 24 bits
https://sourceware.org/bugzilla/show_bug.cgi?id=30967 --- Comment #13 from Mark Wielaard --- (In reply to Aleksei Vetrov from comment #12) > (In reply to Mark Wielaard from comment #6) > > So my preferred workaround: > > https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=really-large-disc > > > Do you have plan to merge it to master? I pushed it, so we have at least a workaround in for the release. But I'll keep this bug open so we can do a proper fix. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/30967] Discriminator in Dwarf_Line_s may exceed 24 bits
https://sourceware.org/bugzilla/show_bug.cgi?id=30967 Mark Wielaard changed: What|Removed |Added Ever confirmed|0 |1 Last reconfirmed||2023-10-31 Status|UNCONFIRMED |NEW -- You are receiving this mail because: You are on the CC list for the bug.
