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

pe: make sure authenticode is identical before/after signature #383

Merged
merged 5 commits into from
Jan 1, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 170 additions & 19 deletions src/pe/authenticode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@
// - data directory entry for certtable
// - certtable

use alloc::collections::VecDeque;
use core::ops::Range;
use log::debug;

use super::PE;
use super::{section_table::SectionTable, PE};

static PADDING: [u8; 7] = [0; 7];

impl PE<'_> {
/// [`authenticode_ranges`] returns the various ranges of the binary that are relevant for
Expand All @@ -19,6 +23,7 @@ impl PE<'_> {
ExcludedSectionsIter {
pe: self,
state: IterState::default(),
sections: VecDeque::default(),
}
}
}
Expand All @@ -29,34 +34,49 @@ impl PE<'_> {
pub(super) struct ExcludedSections {
checksum: Range<usize>,
datadir_entry_certtable: Range<usize>,
certtable: Option<Range<usize>>,
certificate_table_size: usize,
end_image_header: usize,
}

impl ExcludedSections {
pub(super) fn new(
checksum: Range<usize>,
datadir_entry_certtable: Range<usize>,
certtable: Option<Range<usize>>,
certificate_table_size: usize,
end_image_header: usize,
) -> Self {
Self {
checksum,
datadir_entry_certtable,
certtable,
certificate_table_size,
end_image_header,
}
}
}

pub struct ExcludedSectionsIter<'s> {
pe: &'s PE<'s>,
state: IterState,
sections: VecDeque<SectionTable>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this mean the iterator allocates every time it's created? this is a little unusual for iterator implementations (most people expect the allocation to occur at their own choosing, e.g., from collect::<Vec<_>>() or whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does. But parsing PE already requires you to allocate (for the section table specifically).
I don't really have a better option here. The spec requires me to sort the section tables, and there is a non-fixed amount of them.

}

#[derive(Debug, PartialEq)]
enum IterState {
Initial,
DatadirEntry(usize),
CertTable(usize),
Final(usize),
ChecksumEnd(usize),
CertificateTableEnd(usize),
HeaderEnd {
end_image_header: usize,
sum_of_bytes_hashed: usize,
},
Sections {
tail: usize,
sum_of_bytes_hashed: usize,
},
Final {
sum_of_bytes_hashed: usize,
},
Padding(usize),
Done,
}

Expand All @@ -76,24 +96,155 @@ impl<'s> Iterator for ExcludedSectionsIter<'s> {
loop {
match self.state {
IterState::Initial => {
self.state = IterState::DatadirEntry(sections.checksum.end);
return Some(&bytes[..sections.checksum.start]);
// 3. Hash the image header from its base to immediately before the start of the
// checksum address, as specified in Optional Header Windows-Specific Fields.
let out = Some(&bytes[..sections.checksum.start]);
debug!("hashing {:#x} {:#x}", 0, sections.checksum.start);

// 4. Skip over the checksum, which is a 4-byte field.
debug_assert_eq!(sections.checksum.end - sections.checksum.start, 4);
self.state = IterState::ChecksumEnd(sections.checksum.end);

return out;
}
IterState::DatadirEntry(start) => {
self.state = IterState::CertTable(sections.datadir_entry_certtable.end);
return Some(&bytes[start..sections.datadir_entry_certtable.start]);
IterState::ChecksumEnd(checksum_end) => {
// 5. Hash everything from the end of the checksum field to immediately before the start
// of the Certificate Table entry, as specified in Optional Header Data Directories.
let out =
Some(&bytes[checksum_end..sections.datadir_entry_certtable.start]);
debug!(
"hashing {checksum_end:#x} {:#x}",
sections.datadir_entry_certtable.start
);

// 6. Get the Attribute Certificate Table address and size from the Certificate Table entry.
// For details, see section 5.7 of the PE/COFF specification.
// 7. Exclude the Certificate Table entry from the calculation
self.state =
IterState::CertificateTableEnd(sections.datadir_entry_certtable.end);

return out;
}
IterState::CertificateTableEnd(start) => {
// 7. Exclude the Certificate Table entry from the calculation and hash everything from
// the end of the Certificate Table entry to the end of image header, including
// Section Table (headers). The Certificate Table entry is 8 bytes long, as specified
// in Optional Header Data Directories.
let end_image_header = sections.end_image_header;
let buf = Some(&bytes[start..end_image_header]);
debug!("hashing {start:#x} {:#x}", end_image_header - start);

// 8. Create a counter called SUM_OF_BYTES_HASHED, which is not part of the signature.
// Set this counter to the SizeOfHeaders field, as specified in
// Optional Header Windows-Specific Field.
let sum_of_bytes_hashed = end_image_header;

self.state = IterState::HeaderEnd {
end_image_header,
sum_of_bytes_hashed,
};

return buf;
}
IterState::HeaderEnd {
end_image_header,
sum_of_bytes_hashed,
} => {
// 9. Build a temporary table of pointers to all of the section headers in the
// image. The NumberOfSections field of COFF File Header indicates how big
// the table should be. Do not include any section headers in the table whose
// SizeOfRawData field is zero.
let mut sections: VecDeque<SectionTable> = self
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd not expect allocations in the iterator itself; (same comment as above); is there anyway to avoid allocating? (if there's not, or it's too difficult, that's fine, just something to maybe document?)

.pe
.sections
.iter()
.filter(|section| section.size_of_raw_data != 0)
.cloned()
.collect();

// 10. Using the PointerToRawData field (offset 20) in the referenced SectionHeader
// structure as a key, arrange the table's elements in ascending order. In
// other words, sort the section headers in ascending order according to the
// disk-file offset of the sections.
sections
.make_contiguous()
.sort_by_key(|section| section.pointer_to_raw_data);

self.sections = sections;

self.state = IterState::Sections {
tail: end_image_header,
sum_of_bytes_hashed,
};
}
IterState::CertTable(start) => {
if let Some(certtable) = sections.certtable.as_ref() {
self.state = IterState::Final(certtable.end);
return Some(&bytes[start..certtable.start]);
IterState::Sections {
mut tail,
mut sum_of_bytes_hashed,
} => {
// 11. Walk through the sorted table, load the corresponding section into memory,
// and hash the entire section. Use the SizeOfRawData field in the SectionHeader
// structure to determine the amount of data to hash.
if let Some(section) = self.sections.pop_front() {
let start = section.pointer_to_raw_data as usize;
let end = start + section.size_of_raw_data as usize;
tail = end;

// 12. Add the section’s SizeOfRawData value to SUM_OF_BYTES_HASHED.
sum_of_bytes_hashed += section.size_of_raw_data as usize;

debug!("hashing {start:#x} {:#x}", end - start);
let buf = &bytes[start..end];

// 13. Repeat steps 11 and 12 for all of the sections in the sorted table.
self.state = IterState::Sections {
tail,
sum_of_bytes_hashed,
};

return Some(buf);
} else {
self.state = IterState::Final {
sum_of_bytes_hashed,
};
}
}
IterState::Final {
sum_of_bytes_hashed,
} => {
// 14. Create a value called FILE_SIZE, which is not part of the signature.
// Set this value to the image’s file size, acquired from the underlying
// file system. If FILE_SIZE is greater than SUM_OF_BYTES_HASHED, the
// file contains extra data that must be added to the hash. This data
// begins at the SUM_OF_BYTES_HASHED file offset, and its length is:
// (File Size) - ((Size of AttributeCertificateTable) + SUM_OF_BYTES_HASHED)
//
// Note: The size of Attribute Certificate Table is specified in the second
// ULONG value in the Certificate Table entry (32 bit: offset 132,
// 64 bit: offset 148) in Optional Header Data Directories.
let file_size = bytes.len();
if file_size > sum_of_bytes_hashed {
let extra_data_start = sum_of_bytes_hashed;
let len =
file_size - sections.certificate_table_size - sum_of_bytes_hashed;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file_size - sections.certificate_table_size - sum_of_bytes_hashed;
file_size - sections.certificate_table_size + sum_of_bytes_hashed;

Copy link
Contributor Author

@baloo baloo Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec calls for:

(File Size) - ((Size of AttributeCertificateTable) + SUM_OF_BYTES_HASHED)
              ^                                                         ^

I can go with file_size - (sections.certificate_table_size + sum_of_bytes_hashed), but your suggestion is wrong I think.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably wrong :) I thought it was more important to point out the error.


debug!("hashing {extra_data_start:#x} {len:#x}",);
let buf = &bytes[extra_data_start..extra_data_start + len];

self.state = IterState::Padding(buf.len());
return Some(buf);
} else {
self.state = IterState::Final(start)
self.state = IterState::Done;
}
}
IterState::Final(start) => {
IterState::Padding(hash_size) => {
self.state = IterState::Done;
return Some(&bytes[start..]);

if hash_size % 8 != 0 {
let pad_size = 8 - hash_size % 8;

debug!("hashing {hash_size:#x} {pad_size:#x}");
return Some(&PADDING[..pad_size]);
}
}
IterState::Done => return None,
}
Expand Down
13 changes: 6 additions & 7 deletions src/pe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ impl<'a> PE<'a> {
}

// Parse attribute certificates unless opted out of
let certtable = if opts.parse_attribute_certificates {
let certificate_table_size = if opts.parse_attribute_certificates {
if let Some(&certificate_table) =
optional_header.data_directories.get_certificate_table()
{
Expand All @@ -241,20 +241,19 @@ impl<'a> PE<'a> {
certificate_table.size,
)?;

let start = certificate_table.virtual_address as usize;
let end = start + certificate_table.size as usize;
Some(start..end)
certificate_table.size as usize
} else {
None
0
}
} else {
None
0
};

authenticode_excluded_sections = Some(authenticode::ExcludedSections::new(
checksum,
datadir_entry_certtable,
certtable,
certificate_table_size,
optional_header.windows_fields.size_of_headers as usize,
));
}
Ok(PE {
Expand Down
Loading