search

huuck / ADBHoney

Low interaction honeypot designed for Android Debug Bridge over TCP/IP
GNU General Public License v3.0
158 stars 33 forks source link

Proper formatting #1

Closed bontchev closed 5 years ago

bontchev commented 5 years ago

If it wouldn't be too much of a trouble, could you please spend some time to format the program properly?

Python is very peculiar about line indentation, since it determines the beginning and ending of blocks. Please indent each block by exactly 4 spaces. As it currently is, different lines have different indentation, the indentation itself is a mixture of tabs and spaces and it is very difficult to understand where the block begins and where it ends.

For instance, where does the while True: loop in function process_connection() actually end? The indentation suggests that it ends after the second try:...except block - but that can't be the case, because there are continue operators afterwards with no corresponding loop.

There are other oddities in the code, like, it seems to me that the variable filename is sometimes used without being initialized - but maybe I don't understand something because the weird line indentations have confused me about the structure of the program...

huuck commented 5 years ago

Gonna do a full lint next week probably.

And yeah, filename was glued there in a rush as the name of the file is not always sent using the packet sequence.

bontchev commented 5 years ago

Please try to fix the indentation first. This should be very easy for you and it will allow others to better understand the code and make improvements or bug fixes to it.

fe7ch commented 5 years ago

@huuck I've made a PR with fixed indentation & some other basic fixes.

fe7ch commented 5 years ago

Furthermore, there was a crucial problem that the call to conn.close() was unreachable.

huuck commented 5 years ago

@fe7ch thanks a lot mate! Try to fix the filename issue and I'll merge it ASAP

fe7ch commented 5 years ago

@huuck I've already did it. I initialize it with "unknown" to avoid crash with referencing to undefined variable. Later, if we know the real filename, the variable will be set to this name.

huuck commented 5 years ago

image It gets reset to unknown after each read packet :P

fe7ch commented 5 years ago

Oups :D Moved the initialization of the variable out of the loop for now.

huuck commented 5 years ago

@bontchev Any more suggestions or we can close this? Looks like @fe7ch did an awesome job in fixing a lot of this stuff.

ghost commented 5 years ago

Yes, one. Disable download of binaries per command switch so we don't get flagged by ISP's. Although I am not sure yet if the download (sinkhole?) triggers it, or the Honeypot just listening.

On Nov 23, 2018, 13:29, at 13:29, Gabriel Cirlig notifications@github.com wrote:

@bontchev Any more suggestions or we can close this? Looks like @fe7ch did an awesome job in fixing a lot of this stuff.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/huuck/ADBHoney/issues/1#issuecomment-441229219

fe7ch commented 5 years ago

Disable download of binaries per command switch so we don't get flagged by ISP's

ADHoney doesn't download anything on its own for now. The "downloaded" files refers to the files that was uploaded to the honeypot via adb. So you should be safe at least for now ;)

bontchev commented 5 years ago

@fe7ch, thanks a lot for fixing the indentation. Could you please take a look at the other file (protocol.py) too? There are a few minor indentation issues that have to be fixed there too; they are much easier.

@HermanusF, it would be preferable to have a separate issue about this. I've been thinking about this subject too. As @fe7ch said, currently the honeypot does not initiate a download (e.g., when the attacker issues a wget shell command. I think that it should, and you make a good point that it should be optional. In addition, currently the honeypot writes uploaded file to disk every time. This should be done only if the file doesn't already exist. But these should be discussed in a separate issue. Let's keep this one about formatting only.

fe7ch commented 5 years ago

Could you please take a look at the other file (protocol.py) too?

I'll take a look on Monday.

ghost commented 5 years ago

My provider was triggered by my connection having port 5555 open, so catching raw files isn't the "problem" at all, they told me 5 minutes ok. So you can forget my request for me, but I think it's a 'nice to have' feature anyways.

On 23 nov. 2018, at 13:50, fe7ch notifications@github.com wrote:

Disable download of binaries per command switch so we don't get flagged by ISP's

ADHoney doesn't download anything on its own for now. The "downloaded" files refers to the files that was uploaded to the honeypot via adb. So you should be safe at least for now ;)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

huuck commented 5 years ago

Allright then ^^ The downloaded files have no impact anyhow (low interaction and all that), so it's safe to save them to disk.

ghost commented 5 years ago

Correct.

On Nov 23, 2018, 17:22, at 17:22, Gabriel Cirlig notifications@github.com wrote:

Allright then ^^ The downloaded files have no impact anyhow (low interaction and all that), so it's safe to save them to disk anyhow.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/huuck/ADBHoney/issues/1#issuecomment-441279843

bontchev commented 5 years ago

OK, I have fixed protocol.py - as I said, it was much easier. Submitted pull request #19. Once it is merged, I'll close this issue.