Skip to content
This repository has been archived by the owner on Sep 25, 2023. It is now read-only.

Fix string decryption race #80

Closed
tempIssuesWriter opened this issue Feb 22, 2020 · 12 comments
Closed

Fix string decryption race #80

tempIssuesWriter opened this issue Feb 22, 2020 · 12 comments

Comments

@tempIssuesWriter
Copy link

tempIssuesWriter commented Feb 22, 2020

Hi, then we use encryption of strings, a race condition is possible cause we use one global variable for decryption. So why can't we use two global variables of the same length and decrypt the string into a second variable, and then use it?

@tempIssuesWriter tempIssuesWriter changed the title Fix string encryption race Fix string decryption race Feb 22, 2020
@Naville
Copy link
Member

Naville commented Feb 25, 2020

Please rephrase your question with proper english

@tempIssuesWriter
Copy link
Author

Well, now when we enable string encryption of function, we are creating an encrypted global variable for each function where it used. Then, when we decrypting variable at first time, we can get into the thread race if several threads step into this function. As a result, string will be corrupted. My question is: why can`t wee just creating two global variables for one string, decrypting first variable into the second, and then use second. We allocate more memory for global variables, but we avoid possible crashes of a program. In my case crash is worst case. Sorry for my bad english.

@tempIssuesWriter
Copy link
Author

tempIssuesWriter commented Feb 25, 2020

For example, my implementation:
main.ll.txt
main.c.txt

@Naville
Copy link
Member

Naville commented Feb 25, 2020

Right, this is a well documented feature. This will probably stay like that in forseeable future for the following reasons:

  • Decrypting into another variable raises other issues:
    • If decrypting into stack memory, then languages like ObjC that keeps internal OS-level references to that memory will yields even more weird behaviour at very least or more likely an unexpected crash somewhere.
    • If decrypting into malloc() ed memory, we need proper deallocation to avoid memory leak, which involves a pile of Cross-TU analysis shit and is almost impossible for us to handle nicely especially with external library calls that IR is not available
    • If decrypting into a GlobalVariable, same racing condition issue exists.
    • If we use TLS to emulate thread local global variable, TLS isn't well supported on some platforms AFAIK plus the potential performance impact of using EmuTLS layers is a burden wayyy too heavy to force on Hikari's users.
  • If you are thinking letting Hikari adding explicit pthread API calls:
    • This is platform-dependent, which is something I tend to avoid with the design of Hikari
    • It leaks patterns for attackers to very easily bypass this obfuscation by hooking related APIs and a small amount of extra work.
    • Again, forcing such performance impact is not something I take lightly

As a result, the suggested solution in this case is to add your own locking mechanism in Application code

@Naville
Copy link
Member

Naville commented Feb 25, 2020

That being said, it's pretty trivial to add pthread support and I'd be more than happy to review your code, it's just not something I personally think is a very smart move

@tempIssuesWriter
Copy link
Author

If we use two global variables, a thread race is possible only if one of the threads modifies a variable in the program logic, it is much better than an uncontrolled thread race on decryption. Global strings are often used for reading.
In more details. In the case of a decryption race, both threads decrypt byte by byte without changing the encrypted data. Therefore, in the worst-case scenario, they simply duplicate the already decrypted bytes.

@tempIssuesWriter
Copy link
Author

tempIssuesWriter commented Feb 25, 2020

I do not think that this is a smart solution, but the use of any lock entails a binding to the api of the operating system. And lock locking will occur on every function call. Moreover, there will be a race to initialize the mutex. You can of course embed mutex initialization at compile time, but how many mutex formats can you support this way? You can certainly implement your own spinlock, but it will look like a dirty hack.

@tempIssuesWriter
Copy link
Author

tempIssuesWriter commented Feb 25, 2020

Thank you for your attention. In any case, for my own purposes I have to use my solution (:
Change the application code to obfuscate it in my opinion an anti-pattern. In this case, it is better to additionally use a packer to protect the strings.

@Naville
Copy link
Member

Naville commented Feb 25, 2020

Ah I see, noted as future enhancement

@tempIssuesWriter
Copy link
Author

Thank you for your time. I already wrote the code, but I tested it only on Linux. Do I need to make a pull request for your review?

@Naville
Copy link
Member

Naville commented Feb 25, 2020

Yes please

@tempIssuesWriter
Copy link
Author

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants