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

feat: support path handle on multi platform #81

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Conversation

Zxilly
Copy link
Contributor

@Zxilly Zxilly commented Jan 26, 2024

Closes: #62

The previous version of gore included operations related to file paths, but it assumed that the user was running on Linux and processing the binary compiled from Linux. This patch adds support for Windows users, as Windows uses a different path separator.

@TcM1911
Copy link
Member

TcM1911 commented Jan 26, 2024

I don't think this is an issue. The internal metadata, uses / in PE files too. Here is some extracted information for a Windows malware compiled on Windows:

$ redress info robbinhood 
OS         windows
Arch       i386
Compiler   1.11 (2018-08-24)
Build ID   S0jQf4QxsZWePWjYbNr_/yPyruULalESiVu0VcSmw/jFkudwhQ0JcJ_Yoy1Xuf/5XZga5N2LpIDGr7nhSbm
GoRoot     C:\Go
Main root  C:/Users/valery/go/src/oldboy
# main     1
# std      60
# vendor   2

Only the GoRoot has \.

Packages also uses /:

$ redress pkg --vendor robbinhood 
Packages:
Name  Version
----  -------
main  

Vendors:
Name                                    Version
----                                    -------
vendor/golang_org/x/crypto/cryptobyte   
vendor/golang_org/x/net/dns/dnsmessage  

Do you have a binary proving it is an issue?

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 26, 2024

To get an easy reproduce, just run the slowtest on a Windows machine. Some path classifier test will fail because they believe they found something like \usr\local\go (yes, the filepath join the path elements with os.Separator:\) but the expected value is /usr/local/go, which is indeed the real string in the binary. I think we should respect the raw data and keep the separator.

I can add some detailed info about how this happened.

When using filepath.Dir to get the directory of the file, it will split the string with / or \ then clean the relative paths, remove the last element and merge them.

The problem happened at the last step. It will join the elements with os.Seperator. But this was determined by the platform of the running golang binary but not the string itself.

I noticed this and attached a debugger for a running instance of gore. Thus, I noticed the separator was \ when it was a byte stream extracted from the binary and become / after the filepath.Dir.

For redress, it use gore internal and also be affected by this.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 27, 2024

@TcM1911 Do you have any comments?

@TcM1911
Copy link
Member

TcM1911 commented Jan 29, 2024

You are right. I wasn't thinking of that condition.

We have test failures due to not knowing of the new go versions. Let's land #78 first. Once it has been merged, rebase so we can ensure the tests are passing.

@TcM1911
Copy link
Member

TcM1911 commented Jan 29, 2024

It looks like some logic is broken. You can run the tests locally by using the -tags slow_test.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 29, 2024

All tests passed on my Windows machine, I will retry with a linux instance.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 29, 2024

I found the problem. In the past, generated package should have a path <autogenerated>, and filepath.Dir parse it as .. The two classifier both write based on this. I will also update the classifier.
image

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a832a33) 75.73% compared to head (1e2db0c) 75.52%.
Report is 15 commits behind head on develop.

Files Patch % Lines
goroot.go 64.28% 2 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #81      +/-   ##
===========================================
- Coverage    75.73%   75.52%   -0.22%     
===========================================
  Files           14       14              
  Lines         1904     2680     +776     
===========================================
+ Hits          1442     2024     +582     
- Misses         252      446     +194     
  Partials       210      210              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 29, 2024

All tests passed now. I updated some tests manually since they are likely get wrong value casued by filepath.Dir("<autogenerate>").

It might be better to regenerate the test cases. It's hard to judge a "." filepath is the raw value or a "<autogenerate>" and "" parsed result.

@TcM1911
Copy link
Member

TcM1911 commented Jan 30, 2024

Actually, instead of implementing the OS free logic, I think we can use the path package instead.

Package path implements utility routines for manipulating slash-separated paths.

The path package should only be used for paths separated by forward slashes, such as the paths in URLs. This package does not deal with Windows paths with drive letters or backslashes; to manipulate operating system paths, use the path/filepath package.

I need to think how we deal with the filepath.Dir("<autogenerate>") issue.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 30, 2024

image

Seems just use path is ok, what's your opinion?

@TcM1911
Copy link
Member

TcM1911 commented Jan 30, 2024

I prefer that over a self-implemented solution.

Sorry for all the back an forth.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 30, 2024

updated

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 30, 2024

for path.Dir() we still need a wrapper

image

@Zxilly
Copy link
Contributor Author

Zxilly commented Jan 31, 2024

Can this PR be merged now? I'm developing on a Windows machine and the false positives on unit tests are annoying

@TcM1911 TcM1911 merged commit 777e08b into goretk:develop Jan 31, 2024
3 checks passed
@TcM1911
Copy link
Member

TcM1911 commented Jan 31, 2024

Yes. It looks good now.

@Zxilly Zxilly deleted the path branch February 1, 2024 10:54
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

Successfully merging this pull request may close these issues.

GetGoRoot return ErrNoGoRootFound when running on windows
3 participants