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

Maybe unsound in get_pe_metadata #9

Closed
lwz23 opened this issue Dec 11, 2024 · 2 comments
Closed

Maybe unsound in get_pe_metadata #9

lwz23 opened this issue Dec 11, 2024 · 2 comments

Comments

@lwz23
Copy link

lwz23 commented Dec 11, 2024

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub fn get_pe_metadata (module_ptr: *const u8) -> Result<PeMetadata,String>
{
    let mut pe_metadata= PeMetadata::default();

    unsafe {

        let e_lfanew = *((module_ptr as usize + 0x3C) as *const u32);
        pe_metadata.pe = *((module_ptr as usize + e_lfanew as usize) as *const u32);
.............................................

Considering that this function is in lib.rs, and get_pe_metadata is also a pub function. I assume that users can directly call this function. This potential situation could result in *((module_ptr as usize + 0x3C) as *const u32) being operating on a null pointer, I guess it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
I suggest Several possible fixes:

  1. If there is no external usage for get_pe_metadata, it should not marked as pub.
  2. get_pe_metadata method should add additional check for null pointer.
  3. mark get_pe_metadata method as unsafe and proper doc to let users know that they should provide valid Pointers.
@lwz23
Copy link
Author

lwz23 commented Dec 16, 2024

ping

@Kudaes
Copy link
Owner

Kudaes commented Dec 18, 2024

Hello mate,

From the point of view of rust safety, you are completely right. That public function could be called from any new code developed on top of mine and potentially lead to a null pointer.
However, this is a closed proof of concept and I do not plan to expand it further or refactor it at all. Feel free to fork the project and fix it at will!

Thank you, mate!

@Kudaes Kudaes closed this as completed Dec 18, 2024
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

2 participants