Fix fail on malformed certificate table parsing #417

ideeockus · 2024-07-29T13:42:22Z

Hello!

In some PE files Certificate Table can be malformed / contain invalid data. But if we use ParseOptions::parse_attribute_certificates = true, then whole parsing is failed.

I suggest use default CertificateDirectoryTable in case of error.

m4b

need some clarification on default certificate directory; also formatting things are odd

m4b · 2024-08-12T05:25:41Z

src/pe/mod.rs

 use alloc::borrow::Cow;
 use alloc::string::String;
 use alloc::vec::Vec;
+use core::cmp::max;


I'm surprised by these import changes, did the formatter do this?

m4b · 2024-08-12T05:26:00Z

src/pe/mod.rs

@@ -142,7 +140,7 @@ impl<'a> PE<'a> {
                    return Err(error::Error::Malformed(format!(
                        "Unsupported header magic ({:#x})",
                        magic
-                    )))
+                    )));


ditto here too, this is also surprising to me

m4b · 2024-08-12T05:28:18Z

src/pe/mod.rs

+                    )
+                    .unwrap_or_else(|err| {
+                        warn!("Cannot parse CertificateTable: {:?}", err);
+                        Default::default()


I can't remember, what is a default certificate directory in this case? is it going to cause other problems further down the line when parsing, or if a user accesses parts of it, will it panic? Does it have offsets into other parts of the PE file that are no longer valid, etc.?

default is empty table (no certificates), so no wrong offsets there

m4b · 2024-08-26T07:48:54Z

So while this is an easy merge, I'm on the fence about whether we should; in general the malformed binary is kind of important to know, and in general, we choose to fail in those cases. However, sometimes we don't, and maybe this is one of those times, but it feels like it's just sort of skipping a malformed thing, and putting a default value in its place, which may be ok, but it also might not be.

So I'd like to understand more about:

why binaries have malformed certificates, is it common
is it not a big deal if they're malformed, e.g., it's safe to just supply a dummy/default/empty version of the table when bad ones are encountered, and continue going on?

thanks for your patience!

ideeockus · 2024-10-27T12:06:11Z

in general the malformed binary is kind of important to know, and in general, we choose to fail in those cases. However, sometimes we don't, and maybe this is one of those times, but it feels like it's just sort of skipping a malformed thing

OK, that sounds reasonable.

What if we add something like ParseMode::Strict and ParseMode::Permissive to ParseOptions?

ideeockus · 2024-12-08T13:24:38Z

@m4b can you review please

I think handling malformations may be related to #401

m4b · 2025-01-05T17:23:03Z

src/pe/options.rs

@@ -8,6 +8,16 @@ pub struct ParseOptions {
    /// memory](https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#other-contents-of-the-file).
    /// For on-disk representations, leave as true. Default: true
    pub parse_attribute_certificates: bool,
+    /// Whether or not to end with an error in case of incorrect data or continue parsing if able. Default: ParseMode::Strict


debating whether we should add #[non_exhaustive] to this struct to make it future compatible if we need to add another field like this in the future (and not break people)

m4b

this looks good to me, except as mentioned, but since we're here, i'd like to see the:

non_exhaustive attrib added to ParseOptions
it actually implement Default trait
add fn te() -> Self method for constructing the parse options that TE requires

m4b · 2025-01-05T17:24:47Z

src/pe/options.rs

@@ -16,6 +26,7 @@ impl ParseOptions {
        ParseOptions {


also i don't know who added this but this does not implement Default, but has a method named that instead; this should be fixed to actually implement Default

thanks for review, just pushed fixes

m4b · 2025-01-05T17:25:47Z

src/pe/options.rs

@@ -16,6 +26,7 @@ impl ParseOptions {
        ParseOptions {
            resolve_rva: true,
            parse_attribute_certificates: true,
+            parse_mode: ParseMode::Strict,
        }


can you also add a method that returns what the TE parser uses; we can call it fn te() or something like that, instead of manually constructing it in TE portion.

m4b · 2025-01-05T19:23:13Z

src/pe/options.rs

+
+impl ParseOptions {
+    pub fn te() -> Self {
+        Self {


more idiomatic would be something like:

Self { resolve_rva: false, parse_attribute_certificates: false, .. Self::default() }

this way if new methods are added don't (maybe) need to update this location in source, but it's fine. I'm also wondering if we should make this non pub, just to reduce the api surface for now, i can fix this up though if you don't feel like pushing again.

m4b

this is great, thank you!

fix private

m4b · 2025-01-05T19:31:03Z

NOTE: breaking change #434

ideeockus marked this pull request as draft July 29, 2024 13:43

pe.cert_table: ignore malformed

2c7f7b5

ideeockus force-pushed the fix/fail_on_malformed_cert_table_parsing branch from 2642c9f to 2c7f7b5 Compare July 30, 2024 07:11

ideeockus marked this pull request as ready for review July 30, 2024 07:12

m4b reviewed Aug 12, 2024

View reviewed changes

pe.cert_table: revert formatting

b46e266

ideeockus and others added 2 commits November 18, 2024 15:07

Merge branch 'm4b:master' into fix/fail_on_malformed_cert_table_parsing

872544b

parse_options: added ParseMode

ac97d4c

ideeockus force-pushed the fix/fail_on_malformed_cert_table_parsing branch from b94d9a3 to ac97d4c Compare December 8, 2024 13:20

m4b reviewed Jan 5, 2025

View reviewed changes

m4b requested changes Jan 5, 2025

View reviewed changes

pe.options: added te(), fixes

aa0de1b

m4b reviewed Jan 5, 2025

View reviewed changes

m4b approved these changes Jan 5, 2025

View reviewed changes

Update options.rs

8838feb

m4b approved these changes Jan 5, 2025

View reviewed changes

Update options.rs

1f7cdc5

fix private

m4b merged commit 50fa096 into m4b:master Jan 5, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fail on malformed certificate table parsing #417

Fix fail on malformed certificate table parsing #417

ideeockus commented Jul 29, 2024 •

edited

Loading

m4b left a comment

m4b Aug 12, 2024

m4b Aug 12, 2024

m4b Aug 12, 2024

ideeockus Aug 14, 2024

m4b commented Aug 26, 2024

ideeockus commented Oct 27, 2024

ideeockus commented Dec 8, 2024

m4b Jan 5, 2025

m4b left a comment

m4b Jan 5, 2025

ideeockus Jan 5, 2025

m4b Jan 5, 2025

m4b Jan 5, 2025

m4b left a comment

m4b commented Jan 5, 2025

Fix fail on malformed certificate table parsing #417

Fix fail on malformed certificate table parsing #417

Conversation

ideeockus commented Jul 29, 2024 • edited Loading

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4b commented Aug 26, 2024

ideeockus commented Oct 27, 2024

ideeockus commented Dec 8, 2024

Choose a reason for hiding this comment

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4b left a comment

Choose a reason for hiding this comment

m4b commented Jan 5, 2025

ideeockus commented Jul 29, 2024 •

edited

Loading