elf.parse: resolve overflow issue in hash sections #237

quake · 2020-10-06T02:35:43Z

I found an overflow error when running the fuzz test (test case), this PR try to fix the issue by using wrapping operator.

m4b · 2020-10-06T04:39:49Z

Hmmm looks like macos CI is failing (for likely unrelated reasons ??)

quake · 2020-10-06T05:02:50Z

weird, trying revert commit and trigger the CI again

quake · 2020-10-06T05:06:38Z

Hmmm looks like macos CI is failing (for likely unrelated reasons ??)

I'm sure the CI error has nothing to do with this commit, I reverted the commit and got same error.

m4b · 2020-10-06T06:03:43Z

it looks like /Library/Developer/CommandLineTools/usr/bin/dyldinfo doesn't exist anymore, so that's the failure

willglynn · 2020-10-06T06:06:00Z

Ah, I believe that's moved from …/dyldinfo args to /usr/bin/xcrun dyldinfo args.

m4b · 2020-10-06T06:08:57Z

Oh you sweet, sweet cherub @willglynn , swooping in and saving the day; @quake this is annoying, but would you mind changing

goblin/tests/compare_dyldinfos.rs

Line 4 in e94ca6b

    
           let apple = process::Command::new("/Library/Developer/CommandLineTools/usr/bin/dyldinfo")

to xcrun dyldinfo ?

m4b · 2020-10-06T06:09:36Z

actually it may be more complicated, maybe a separate PR is better

quake · 2020-10-06T06:14:34Z

I will create a separate PR to try xcrun, but I suspect that's not the causing, since test output reports:

cargo dyldinfo ["-lazy_bind", "-bind", "/Library/Developer/CommandLineTools/usr/bin/dyldinfo"] output:
err: BadMagic(3405691582)

and /Library/Developer/CommandLineTools/usr/bin/dyldinfo runs well.

it looks like the magic header of dyldinfo is 0xcafebabe (3405691582), and it's ignored in parse_magic_and_ctx fn.

m4b · 2020-10-06T06:25:28Z

Oh wow you're right; fwiw, it should probably be updated to not hard code anyway, since it can fail on other machines, something perhaps like:

diff --git a/tests/compare_dyldinfos.rs b/tests/compare_dyldinfos.rs
index 0c9dcaf..806e004 100644
--- a/tests/compare_dyldinfos.rs
    b/tests/compare_dyldinfos.rs
@@ -1,7  1,19 @@
 use std::process;
 
 fn get_realpath(cmd: &str) -> String {
     let output = process::Command::new("/usr/bin/xcrun")
         .arg("-f")
         .arg(cmd)
         .output()
         .expect("can get realpath");
     std::str::from_utf8(&output.stdout)
         .expect("output is valid utf8")
         .to_string()
 }
 
 pub fn compare(args: Vec<&str>) {
-    let apple = process::Command::new("/Library/Developer/CommandLineTools/usr/bin/dyldinfo")
     let apple = process::Command::new("/usr/bin/xcrun")
         .arg("dyldinfo")
         .args(&args)
         .output()
         .expect("run Apple dyldinfo");
@@ -39,39  51,28 @@ pub fn compare(args: Vec<&str>) {
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_binds() {
-    compare(vec![
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
-    compare(vec![
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/clang",
-    ]);
     let dyldinfo = get_realpath("dyldinfo");
     let clang = get_realpath("dyldinfo");
     compare(vec!["-bind", &dyldinfo]);
     compare(vec!["-bind", &clang]);
     compare(vec!["-bind", "/usr/bin/tmutil"]);
 }
 
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_lazy_binds() {
-    compare(vec![
-        "-lazy_bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
-    compare(vec![
-        "-lazy_bind",
-        "/Library/Developer/CommandLineTools/usr/bin/clang",
-    ]);
     let dyldinfo = get_realpath("dyldinfo");
     let clang = get_realpath("dyldinfo");
     compare(vec!["-lazy_bind", &dyldinfo]);
     compare(vec!["-lazy_bind", &clang]);
     compare(vec!["-lazy_bind", "/usr/bin/tmutil"]);
 }
 
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_combined_options() {
-    compare(vec![
-        "-lazy_bind",
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
     let dyldinfo = get_realpath("dyldinfo");
     compare(vec!["-lazy_bind", "-bind", &dyldinfo]);
 }
 
 #[cfg(not(target_os = "macos"))]

m4b · 2020-10-06T07:05:23Z

src/elf/mod.rs

@@ -348,18  348,18 @@ if_sylvan! {

    fn gnu_hash_len(bytes: &[u8], offset: usize, ctx: Ctx) -> error::Result<usize> {
        let buckets_num = bytes.pread_with::<u32>(offset, ctx.le)? as usize;
-        let min_chain = bytes.pread_with::<u32>(offset   4, ctx.le)? as usize;
-        let bloom_size = bytes.pread_with::<u32>(offset   8, ctx.le)? as usize;
+        let min_chain = bytes.pread_with::<u32>(offset.wrapping_add(4), ctx.le)? as usize;


ok back to this PR; so what are the semantics if this overflows? This seems like a subtle bug now (I guess it was always present) Also, won't this only crash in debug mode, but will wrap overflow in release? Ditto for the other explicit overflow wraps

I'm using wrapping_xxx here because I've seen other code do it too, for example,

goblin/src/mach/imports.rs

Line 234 in 2188699

let seg_offset = bind_info.seg_offset.wrapping_add(addr);

, semantically, maybe overflowing_xxx is better?

yes, i think your intuition is right, was kind of just wondering out loud what pros/cons are of the explict wrap, and what it means for the gnu hash table calculations to overflow (i just assume it means the binary is bad, better not to crash then i suppose?)

Does gnu_hash_len calculate total size of GNU_HASH table?
Then performing some checks like

goblin/src/elf/gnu_hash.rs

Lines 116 to 135 in ece3ce6

let hashtab = &hashtab[16..];

{

// SAFETY: Condition to check for an overflow

// size_of(chains) size_of(buckets) size_of(bloom_filter) == size_of(hashtab)

if dynsyms.len() <= symindex as usize {

return Err("symindex must be smaller than dynsyms.len()");

}

let chains_size = (dynsyms.len() - symindex as usize).checked_mul(U32_SIZE);

let buckets_size = (nbuckets as usize).checked_mul(U32_SIZE);

let bloom_size = (maskwords as usize).checked_mul(INT_SIZE);

let total_size = match (chains_size, buckets_size, bloom_size) {

(Some(a), Some(b), Some(c)) => {

a.checked_add(b).and_then(|t| t.checked_add(c))

}

_ => None,

};

match total_size {

Some(size) if size == hashtab.len() => {}

is Ok.

m4b · 2020-10-06T07:06:19Z

@lzutao perhaps you have some thoughts on behavior of overflow wrapping, you did the work on the gnu hash rewrite

m4b · 2021-01-31T05:59:47Z

@quake are you still interested in getting this merged? let me know whenever you have a chance, just triaging old PRs here, thanks!

quake force-pushed the elf-parse-overflow branch from f7ade0d to 5a14577 Compare October 6, 2020 05:08

elf.parse: resolve overflow issue in hash sections

4491020

quake force-pushed the elf-parse-overflow branch from 5a14577 to 4491020 Compare October 6, 2020 06:55

m4b reviewed Oct 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

elf.parse: resolve overflow issue in hash sections #237

elf.parse: resolve overflow issue in hash sections #237

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

quake commented Oct 6, 2020

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

willglynn commented Oct 6, 2020

m4b commented Oct 6, 2020

m4b commented Oct 6, 2020

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

m4b Oct 6, 2020

quake Oct 6, 2020

m4b Oct 6, 2020

tesuji Oct 6, 2020

m4b commented Oct 6, 2020

m4b commented Jan 31, 2021

	let hashtab = &hashtab[16..];
	{
	// SAFETY: Condition to check for an overflow
	// size_of(chains) size_of(buckets) size_of(bloom_filter) == size_of(hashtab)

	if dynsyms.len() <= symindex as usize {
	return Err("symindex must be smaller than dynsyms.len()");
	}
	let chains_size = (dynsyms.len() - symindex as usize).checked_mul(U32_SIZE);
	let buckets_size = (nbuckets as usize).checked_mul(U32_SIZE);
	let bloom_size = (maskwords as usize).checked_mul(INT_SIZE);

	let total_size = match (chains_size, buckets_size, bloom_size) {
	(Some(a), Some(b), Some(c)) => {
	a.checked_add(b).and_then(\|t\| t.checked_add(c))
	}
	_ => None,
	};
	match total_size {
	Some(size) if size == hashtab.len() => {}

elf.parse: resolve overflow issue in hash sections #237

Are you sure you want to change the base?

elf.parse: resolve overflow issue in hash sections #237

Conversation

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

quake commented Oct 6, 2020

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

willglynn commented Oct 6, 2020

m4b commented Oct 6, 2020

m4b commented Oct 6, 2020

quake commented Oct 6, 2020

m4b commented Oct 6, 2020

m4b Oct 6, 2020

Choose a reason for hiding this comment

quake Oct 6, 2020

Choose a reason for hiding this comment

m4b Oct 6, 2020

Choose a reason for hiding this comment

tesuji Oct 6, 2020

Choose a reason for hiding this comment

m4b commented Oct 6, 2020

m4b commented Jan 31, 2021