Skip to content

Commit

Permalink
Fix message encoding bug
Browse files Browse the repository at this point in the history 
- prevent loading invalid message types
  • Loading branch information
@PeterSurda
PeterSurda committed Feb 13, 2018
1 parent 96ea36c commit 3a8016d
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions src/messagetypes/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ def encode(self):

def constructObject(data):
try:
classBase = eval(data[""] + "." + data[""].title())
except NameError:
logger.error("Don't know how to handle message type: \"%s\"", data[""])
m = import_module("messagetypes." + data[""])
classBase = getattr(m, data[""].title())
except (NameError, ImportError):
logger.error("Don't know how to handle message type: \"%s\"", data[""], exc_info=True)
return None
try:
returnObj = classBase()
Expand Down

30 comments on commit 3a8016d

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's allows a remote execution, but it probably crashed for most people before it could execute anything. In the logs I see attempts to run a windows executable and to steal electrum wallet files.

@rfreemobile
Copy link

@rfreemobile rfreemobile commented on 3a8016d Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSurda god damn. Using eval() should be illegal by law.

This is used in encode() ... but still it is called on the data coming from network then?

Requesting CVE number for it? Remote arbitrary code execution in widely used application...

@copumpkin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't feel ideal. Although it isn't arbitrary code anymore, you still let untrusted data specify which method to call on an object. Is this a temporary fix or the final one?

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am open for suggestions on how to improve it. I haven't build the binaries yet, possibly I'll release 0.6.3.1 with an improved fix, but I had to move quickly.

@copumpkin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably assert that title() is one of the known/expected values before invoking it, or something like that. In the longer run, it's probably better to make it a more formal message parser: see langsec/weird machines and so on: http://langsec.org/

@tintinweb
Copy link

@tintinweb tintinweb commented on 3a8016d Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah as @copumpkin mentioned this could need some input validation.

and maybe also check the other evals? Usually there is not many reasons to use eval.

@copumpkin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, eval is a huge no-no in security-sensitive software (so all of it, really). In some cases the input might effectively be trusted, but it's easier to just outlaw eval so people don't have to stare extra hard at code using it to make sure it's not stupid.

@g1itch
Copy link
Collaborator

@g1itch g1itch commented on 3a8016d Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did someone used message type Vote ever? @copumpkin title(), if you have no such file or no such class in it - it will be ignored.

@copumpkin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm sorry, I misunderstood what was going on. It still seems good to explicitly validate this sort of thing. If only so that future readers can see when behavior like this changes, and can be sure that it's actually running against an explicitly defined message schema rather than relying on reflective behavior against a living codebase.

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@g1itch vote doesn't really do anything, it's an example.

@copumpkin I'll restrict extended encoding to "message" type for the time being, it can be loosened later after a more thorough review.

@tigusoft
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSurda @rfree-d
applied for CVE, seems bug was first in tagged version v0.6.2 and now fixed in v0.6.3

https://docs.google.com/spreadsheets/d/1PlDOsZ4Q36JU4Dz9zyBB2F3814dScppCRCe1muCT7JI/edit#gid=1009122160

(search "bitmessage")

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@celmar01
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ValdikSS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent serious damage from such kind of vulnerabilities, you should write SELinux/AppArmor/Firejail profiles and bundle it with the application.

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be happy to accept pull requests for security profiles.

@KOLANICH
Copy link

@KOLANICH KOLANICH commented on 3a8016d Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSurda, I wanna know how this have happened that such a backdory function like eval have appeared in such a security-critical application as bitmessage. I wanna know which measures are you going to take to make this kind of backdoors never appear again.

IMHO at least we need a blacklist of functions which must not be used in bm and any of it dependencies. All the PRs using them must be automatically rejected. All the PR's introducing new dependencies must be rejected. Dependencies and all their dependencies must be checked and forked and freezed and installed only from this project's repos, not from pip. Only after creating an own fork of dependencies which involves manual checking a dependency may be used.

I also have some ideas about building security into python interpreter, I'm currently trying to convert my thoughts into a document.

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KOLANICH You can apply for an audit/hardening job: #1136

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KOLANICH And the bug appeared because I am a guy who decided to take care of an abandoned project that I found very important not realising the huge scope of such an endeavor. Luckily there are more contributors in the meantime who do review my code, and the workflow has slightly improved.

@tigusoft
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CVE is assigned: CVE-2018-1000070 (still not published, just reserved)

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-1000070

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CVE published.

@sigoa
Copy link
Contributor

@sigoa sigoa commented on 3a8016d Mar 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanna know which measures are you going to take to make this kind of backdoors never appear again.

well, codacy will throw B307 alert if eval() is being used anywhere. @KOLANICH

@KOLANICH
Copy link

@KOLANICH KOLANICH commented on 3a8016d Mar 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigoa not only, I'm currently working on a draft of the spec combining taint checking, mandatory access control, capability-based security and permissions into a framework enforcing that

  • no data from network and other untrusted sources will be passed into security-sensitive functions like eval
  • no sensitive data will be leaked into untrusted destinations like net
  • no sensitive data resulted in invocation of a privileged API will be available to any module not having a permission to read results from that API
  • every module will have to explicitly declare the API inputs to which that it controls
  • and I hope that the framework can be applied to real-world applications

but I have some problems with proofs because the functions to explicitly change taints spoil all the picture, in presence of these functions I cannot prove anything, in absence the scheme is complete garbage unusable in real world apps. For example a private key is a sensitive piece of data, that's why everything touched by it will also be marked as sensitive, even the digital signature which is in fact by default not sensitive assumming no vulnrs in crypto. To solve this issue we need to mark it as non-sensitive, but doing so spoils the whole picture, we cannot prove that the system is secure anymore.

For now I recommend to start implementing taint checking using some frameworks available for python.

@sigoa
Copy link
Contributor

@sigoa sigoa commented on 3a8016d Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, its tricky 🌵 , ain't it. maybe some QUBE OS users made some progress, I don't know.

@sigoa
Copy link
Contributor

@sigoa sigoa commented on 3a8016d Mar 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSurda please add label "SECURITY" to this ISSUE , so one can find it better / sortability , since this is no laughing matter as we know. thanks 👍
... well you can't , because it is a RELEASE not an ISSUE . 🏓

see secu issues here: security

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @KOLANICH , the security audit job is still open, but contributions that either harden the code directly, or provide a framework for hardening too. Since the vulnerability was revealed, I launched a multi-pronged approach, here are some of the highlights:

Infrastructure

  • infrastructure (build, bootstrap, website, ...) was hardened and separated from the development environment. This way, even if I get hacked, other people can continue developing, binaries can continue to be built, people can connect to the network. I now have a separate laptop for accessing the infrastructure

  • my development environment was also hardened and I increased the use of smart cards for authentication and encryption

Development

  • all new code, including mine, has to be reviewed before being merged

  • better integration with codacy, and I fine-tuned some settings

  • existing code will have all the codacy complaints fixed before I do further development (well, maybe some can't be fixed entirely, but they will be treated)

  • design documentation will be reverse-enginered from the code so that it's easier for new developers/reviewers to understand what's happening

  • I'll do less development and work more on design/organisational layer

Other things

  • I spent about 2 weeks reading through CVs an interviewing developers, and recruited a company to help me with the development. This means more people fixing coding issues and reviewing the code

  • I sent some money to tip4commit so that contributors can be better rewarded, and I want to institute a bug bounty program

  • I got rid of ALL the evals in the code. There are still some calls, e.g. the apinotify or the make for autobuilding the C PoW. These are more difficult to get rid of, but at least they can be hardened somewhat

@sigoa Yep, can't assign a label.

@KOLANICH
Copy link

@KOLANICH KOLANICH commented on 3a8016d Mar 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

design documentation will be reverse-enginered from the code so that it's easier for new developers/reviewers to understand what's happening

Could I ask you to write the docs to the message format in Kaitai Struct language (parser can be auto-generated further from the docs, it is machine-readable formal language)? Here is my draft (I have reverse-engineered it from the code and existing docs, but have not tested it anyhow) https://github.com/KOLANICH/kaitai_struct_formats/blob/bitmessage/network/bitmessage.ksy, I expect you to check and test it and develop further. If you wanna check it against a pcap dump inthe same repo you can find a parser for pcap and pcapng formats and my lib https://github.com/KOLANICH/Pipeline.py (which in fact was created for the similar task ) may be useful too.

increased the use of smart cards

which ones?

I now have a separate laptop for accessing the infrastructure

I hope without any backdoors by Intel/AMD/longsoon/any other chineese/taiwanese/russian company/etc?

interviewing developers, and recruited a company to help me with the development

I see you are serious with it. Are you going to make business on it? But how are you going to make it sustainable? This will require money and I see no legitimate (backdors are certainly illegitimate) sources of money providing enough money for such a project (you know, most of people don't need decentralised systems, they only need fashionable centralised things: whatsapp, viber, telegram, instagram, prisma, Alexa, Cortana, Siri, iPhone, etc, and without a big market company will likely go bankrupt), maybe except a grant by Mozilla.

@sigoa
Copy link
Contributor

@sigoa sigoa commented on 3a8016d Mar 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mozilla has kind of funny attitudes. 🤡

@sigoa
Copy link
Contributor

@sigoa sigoa commented on 3a8016d Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could @PeterSurda pls link CODACY into wiki here ? thx 👍

@PeterSurda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I ask you to write the docs to the message format in Kaitai Struct

I'll look at it in more detail. It looks like a wire protocol specification, which is fine, but I want specification of both the Bitmessage system design as well as how PyBitmessage is put together (like what are the threads, what are they doing, and so on). For example the wire protocol specification cannot specify when an inv is being sent, what the handshake sequence looks like, and so on. It's also unclear to me at this point how TLS is supposed to be specified in Kaitai Struct.

which ones? (smartcards, e.d.)

So far Yubikey and Nitrokey are being tested. I'd also consider Trezor and Nano Ledger S but they look like an overkill. I also have a separate code signing card (Win/OSX), forgot who the manufacturer is, but I want to replace it as I'm not happy with it.

I hope without any backdoors by Intel/AMD/longsoon/any other chineese/taiwanese/russian company/etc?

I don't know any laptops guaranteed without backdoors, apart from some very old models which aren't practically usable anymore, or they can't be bought in a physical shop in Europe. The one I bought has a CPU that according to specifications doesn't have TXE. If that means that the ME is entirely absent I don't know. The closest to what I want is probably Purism Librem (which uses Coreboot and has ME disabled) but I don't know how to get it in a physical shop in Europe.

Are you going to make business on it? But how are you going to make it sustainable? This will require money and I see no legit sources of money providing enough money for such a project, maybe except a grant by Mozilla.

I've been thinking about revenue possibilities for quite some time and looking for a co-founder for about a year. I now found someone and we're investigating a particular business plan (he on the business side, me on the technical side). The details will be announced in due time. There are other possible business plans that can be investigated when we see growth. Supporting the project through grants is a possibility but I don't see it as a sustainable one.

@KOLANICH
Copy link

@KOLANICH KOLANICH commented on 3a8016d Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a wire protocol specification

It's binary formats spec, not protocol. Protocol spec is yet to be created, IMHO verilog shkuld be used for protocols specs.

but I want specification of both the Bitmessage system design as well as how PyBitmessage is put together

I only advised a part suitable for describing message format.

It's also unclear to me at this point how TLS is supposed to be specified in Kaitai Struct.

I have not implemented tls messages. And I guess BM has nothing to do with tls itself, tls should be managed by tls libraries, not by PBM itself.

I now found someone

I hope it's not government agencies or cybercriminals.

Please sign in to comment.