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

Refactoring of stub.c #11

Open
shinokaro opened this issue Feb 9, 2024 · 4 comments
Open

Refactoring of stub.c #11

shinokaro opened this issue Feb 9, 2024 · 4 comments
Assignees

Comments

@shinokaro
Copy link
Collaborator

@Largo

The implementation of stub.c had security issues, such as the use of strcat with fixed-length buffers, and the code complexity made it difficult to review partial fixes due to the mixture of TCHAR and char types.

I spent a month thoroughly understanding stub.c and its design, and I have been refactoring stub.exe to make it secure and the code more readable. The refactoring follows these guidelines:

  • Use char type for string operations because the archive path strings are in UTF-8, allowing for paths with multibyte characters.
  • Use heap memory for file path operations to prevent buffer overflows, enabling support for paths longer than MAX_PATH.
  • Improve the archive data format to be more secure and compact.

This effort is still ongoing, but the implementation of stub has become safer than before. However, several more changes are needed to complete the secure implementation.

stub.c compiles successfully with Mingw32 and Mingw64 on Windows. Please let me know if there are any compilation issues with Wine.

@Largo
Copy link
Owner

Largo commented Feb 14, 2024

Thank you so much. I looked into it and it looks good!
I'm preparing a release right now.

@shinokaro
Copy link
Collaborator Author

The source code of the stub has now been made readable by humans.

The current implementation standardizes file path handling to UTF-8 and uses a variable-length buffer for path concatenation, eliminating the dependency on MAX_PATH and the possibility of buffer overruns. Additionally, security risks associated with script execution have been resolved. The stub cannot execute any scripts located externally.

However, security risks that have existed since before the fork from OCRA have not yet been addressed. In the next step, we will resolve these remaining security risks.

@shinokaro
Copy link
Collaborator Author

The commit at 72c376d resolved a potential security issue, preventing files from being written outside the designated directory and execution of external scripts via 'stub'.

@shinokaro
Copy link
Collaborator Author

The refactoring of stub.c has been completed in the commit a7b8347.
The clear security issues in the implementation have been resolved. However, since security standards in actual operation vary by organization, I believe there are still areas for improvement based on external feedback.

The next step for stub is to handle paths with multibyte character strings. Users outside English-speaking countries use characters from their own languages in their usernames, which appear in the temporary paths. Additionally, I believe there is a need to replace WIN32API calls with standard C functions.

@shinokaro shinokaro self-assigned this Aug 6, 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