Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coff: String table parsing returns more strings than exist in the table #309

Closed
ghost opened this issue May 27, 2022 · 1 comment
Closed

Comments

@ghost
Copy link

ghost commented May 27, 2022

Platform: windows 19044.1706
goblin version: 0.5.1

Expected behavior

When I parse the string table in a coff file, I expect it to return me just the strings that exist in the symbol table.

Actual behavior

The string table parsing code instead returns three extra strings in the beginning of the list of strings.
The problem seems to be due to the fact we don't update the offset before reading the strings.

Repro

You'll need to have the nasm (https://nasm.us/) assembler installed to assemble the program below:

bits 64
default rel

segment .text
global main

extern ExitProcess
main:
	xor rax, rax
	call ExitProcess

Assemble the program using this command: nasm -f win64 repro.asm -o repro.obj

And this is an example of a Rust program that reads a Coff file and dumps all the strings found in its string table:

use goblin::pe::Coff;

use std::fs::File;
use std::io::BufReader;
use std::io::Read;

fn dump_strtab(coff: &Coff) {
	let strtab = coff.strings.to_vec().unwrap();
	println!("{:?}", strtab);
}

fn main() {
	if let filepath = std::env::args().skip(1).next() {
		let f = File::open(filepath.unwrap()).unwrap();
		let mut reader = BufReader::new(f);
		let mut buffer = Vec::new();
		reader.read_to_end(&mut buffer);

		match Coff::parse(buffer.as_slice()) {
			Ok(coff) => dump_strtab(&coff),
			Err(e) => println!("{:?}", e)
		}
	}
}

Run the program with cargo run repro.obj.

Actual output

["\u{10}", "", "", "ExitProcess"]

Expected output

["ExitProcess"]

The bug seems to be in the header.rs file (https://docs.rs/goblin/0.5.1/src/goblin/pe/header.rs.html#177). The code doesn't advance the offset by 4 (number of bytes for the string table size field). It ends up reading the 4 bytes as 3 different strings, which makes sense. The size should be 16 (0x10), so the first two bytes it reads make 0x10 and end at the third byte (0x0). The two other strings come from reading the subsequent 0x0.

m4b pushed a commit that referenced this issue Jun 5, 2022
* Fix issue #309 - Advance offset by string table field size

There's a bug in master where the code ends up misreading the symbol table because we don't advance the offset prior to reading the strings.

This change fixes the issue by adding the correct value to the offset and also includes a unit test that covers this case.
@m4b
Copy link
Owner

m4b commented Jun 11, 2022

@track-5 i think this is closed now? please re-open if not fixed, and thanks for the pr!

@m4b m4b closed this as completed Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant