Create exploit_patch.sh, code execution exploit! critical!

[jon1enforce]

the pickle exploit for code execution is still active, i guess.

Repository contributions to the PyBitmessage project

Code

• Try to refer to github issue tracker or other permanent sources of discussion about the issue.
• It is clear from the diff what you have done, it may be less clear why you have done it so explain why this change is necessary rather than what it does.

Documentation

Use tox -e py27-doc to build a local copy of the documentation.

Tests

• If there has been a change to the code, there's a good possibility there should be a corresponding change to the tests
• To run tests locally use tox or ./run-tests-in-docker.sh

Translations

• For helping with translations, please use Transifex.
• There is no need to submit pull requests for translations.
• For translating technical terms it is recommended to consult the Microsoft Language Portal.

Gitiquette

• Make the pull request against the "v0.6" branch
• PGP-sign the commits included in the pull request
• Use references to tickets, e.g. addresses #123 or fixes #234 in your commit messages
• Try to use a good editor that removes trailing whitespace, highlights potential python issues and uses unix line endings
• If for some reason you don't want to use github, you can submit the patch using Bitmessage to the "bitmessage" chan, or to one of the developers.

[Neustradamus]

To follow this PR, thanks @jon1enforce!

[PeterSurda]

Thank you for this AI assisted report. This isn't really an issue because although pickle.load is used on the knownnodes.dat file inside pickle_deserialize_old_knownnodes, the output is then filtered and invalid data is discarded. Perhaps we could make the filter more thorough, e.g. add an additional check to make sure the stream is an int, like this:

diff --git a/src/network/knownnodes.py b/src/network/knownnodes.py
index c53be2cd..ae16641d 100644
--- a/src/network/knownnodes.py
+++ b/src/network/knownnodes.py
@@ -113,6 +113,8 @@ def addKnownNode(stream, peer, lastseen=None, is_self=False):
             for s in stream:
                 addKnownNode(s, peer, lastseen, is_self)
         return
+    if not isinstance(stream, int):  # invalid
+        return

     rating = 0.0
     if not lastseen:

But even then this is only exploitable if you already have write access to the attack target's config files, so the scenarios where it can be used are very limited.

[jon1enforce]

Yes, this is a AI generated report, and patch.
The point is, that this issue was marked as fixed, but the routines are still active; that makes no sense,
i did not record an exploit on my machine, but it is exploitable, because pickle is not strict enough, it is a big hammer, if you need just a small knife.

Ok, let us make it more professional, try out an exploit code:

malicious_pickle = b"""cos
system
(S'rm -rf /' # Dieser Befehl wird AUSGEFÜHRT!
tR.
"""
pickle.loads(malicious_pickle)

This should delete data from your machine...
Do you want someone to delete data remotely?

The pickle exploit is known very long time, it is also on wikipedia.org... it is not a secret.

[jon1enforce]

"Thank you for this AI assisted report. This isn't really an issue because although pickle.load is used on the knownnodes.dat file inside pickle_deserialize_old_knownnodes, the output is then filtered and invalid data is discarded."

->
please let me know where the invalid data is identified, and how.

->
whitelist or blacklist?

->
whitelist -> a list of valid commands
-> blacklist -> mandatory access control?

• if not isinstance(stream, int): # invalid

•    return
  

Here an exploit code to pass the filter:

import io
file_obj = open('malicious.pickle', 'wb')
pickle.dump(MaliciousPayload(), file_obj)
file_obj.close()
fd = open('malicious.pickle', 'rb').fileno()

vulnerable_function(fd) # Exploit

just implement int() in the file descriptor to pass this filter..the object will be executed, like "rm -rf ./*" and all your data is corrupted. It is very clear and well documentated, that this issue is critical...

pickle is very like "all or nothing", it is a big mistake, except for offline applikations.

use json...

[PeterSurda]

This should delete data from your machine... Do you want someone to delete data remotely?

A data format like this cannot be transmitted remotely, there is no space for it in the protocol. It needs to be generated locally, so an attacker needs to have write access to the file.

please let me know where the invalid data is identified, and how.

It's right after pickle.load().

[jon1enforce]

ok, PeterSurda

let us find out, if it is critical or not - i have any exploits here, please add me to your account and write a message, i will try out something, if you agree - it is legal ;-)

In fact, i don't know if this works well, but we will know it after the exploit trial on you.
Are you ready? I will not try to delete files on your machine, but i will try out something, -
It is just a pen-test, just for research, so we will finally know.
Let us try out:
BM-NC1pVjD8idc9JGmHpk2wM7e28Uz7K559

[PeterSurda]

Ok I ran some tests and apparently you can cause a code execution during pickle.load, so a filter isn't a good fix. I still don't understand however how this can be triggered remotely though. Can you explain which part of the code can generate the vulnerable data to be written into knownnodes.dat?

[jon1enforce]

i made a repository for a pen-test:

https://github.com/jon1enforce/exploits/tree/main

I don't believe in this exploits, but we can make it.

I must find out more "weaked or undefined" points in the protocol.py, because remote code exectution is not as easy than people think, it needs compromised dependencies, and also weakness in the protocol, for example a message that generates an error.
special conditions can be useful, like things bitmessage cannot handle.

Never try never know.

the protocol.py is the heart of bitmessage it is the entry point,
Maybe an overflow message?
MAX_OBJECT_PAYLOAD_SIZE = 2**18
no proof of length:
parserPos = 20 + decodeVarint(data[20:30])[1]

buffer 
-> 
expiresTime = unpack('>Q', data[8:16])[0]  
objectType = unpack('>I', data[16:20])[0]  

MAGIC !!!
magic = 0xE9BEB4D9

error payload

payload += encodeVarint(len(errorText))
payload += errorText  

negative values:
parserPos = 20 + decodeVarint(data[20:30])[1]

But pickle is not in the error message handling, pickle is localy exploitable, but maybe not remote, i guess.
So pybitmessage is still safe ;-)

[PeterSurda]

Well I still don't see it. decodeVarint only returns non-negative values because the unpack uses unsigned format characters. Pickle isn't used on payload or error messages. Trying to access payload with an offset larger than the actual data results in an empty string (this isn't C, this is python). And to scan for nodes, you can just use UDP.

[jon1enforce]

The project has made good progress by addressing the original remote code execution vulnerability - replacing the eval() call was definitely the right move!

However, I noticed the current implementation might still benefit from further hardening. The dynamic import approach, while better than eval(), still processes untrusted data directly.

Original code (fixed):
classBase = eval(data[""] + "." + data[""].title()) # Was critical

Current approach:
m = import_module("messagetypes." + data[""])
classBase = getattr(m, data[""].title())

Wouldn't implementing a strict whitelist of allowed message types be a great next step? This would provide deterministic security and prevent any potential edge cases with the dynamic loading.

Just thinking out loud about how we can make this already good project even more secure! What do you all think?"

Reference:

./messagetypes/init.py

This simple approach makes this "harder":
EDIT classBase = getattr(import_module(".{}".format(data[""]), name), data[""].title())
-> NEW:
ALLOWED_TYPES = {
'getpubkey': Getpubkey,
'pubkey': Pubkey,
'msg': Msg,
'broadcast': Broadcast
}

msg_type = data.get("", "").lower()
if msg_type in ALLOWED_TYPES:
return ALLOWED_TYPESmsg_type

[PeterSurda]

Wouldn't implementing a strict whitelist of allowed message types be a great next step?

There already is a whitelist.

I'm closing this because it's 99% useless AI nonsense.

[jon1enforce]

ok, close it.

Iam not native english, i let AI assist on translations, but the issue is not non-sense... Maybe it is a misconception.
If there is already a whitelist, close it, but the security issue isn't solved this way. The right way is to try out exploit the weaked protocol.py and messagetypes, or exploit error handling, and see if this leads to damage or not.
But if nobody will try it out nobody will complain ;-) maybe bitmessage is to small to care. just a small target, because it is not used by millions...