LeastAuthority / destiny

Destiny – Cross-platform Magic Wormhole graphical client
MIT License
216 stars 12 forks source link

Destiny app should identify itself as destiny #193

Open donpui opened 1 year ago

donpui commented 1 year ago

Destiny app should identify itself as destiny, when connecting to Magic Wormhole mailbox server. Currently it identify as:

Propose to identify as:

or if possible, be more specific and identify app platform like:

Identification is used in wormhole-william library version/version.go file

donpui commented 1 year ago

@ewanas as we use flutter and dart plugin, how the best this should be implemented?

meejah commented 1 year ago

Ideally we wouldn't leak the exact version either.

donpui commented 1 year ago

Ideally we wouldn't leak the exact version either.

Not sure if I understood you. We should not send release/tag version to AgentVersion? Or you meant something else?

meejah commented 1 year ago

Yes, I mean that. Knowing the precise version is often valuable when applying network exploits. That advise is pretty common for servers, at least. Perhaps it's somewhat less relevant for clients, but ...

wuan commented 1 year ago

Yes, I mean that. Knowing the precise version is often valuable when applying network exploits. That advise is pretty common for servers, at least. Perhaps it's somewhat less relevant for clients, but ...

Seems that you are referring to that it is a bad idea when a webserver advertises it's exact version. The issue here is about introducing the analogous of a UserAgent header (which is well established in the Web-Space).

btlogy commented 1 year ago

@meejah has a point about avoiding to be too specific in general.

It is obviously a bad idea to leak the product and version of a server that is publicly available. In a similar way, if a client use a flawed lib, attackers impersonating a server could possibly make good use of this info. In the later, it looks harder and less likely, but in both case, it is about the surface (if that's the term?).

So, what are the risk in the case of Destiny? Assuming that the leaking communication should always be encrypted in transport and only be decrypted on our side, that surface looks pretty small. Unless the user chose a different backend, like discussed in the case of F-Droid Non-Free Network Services issue?

Thus, yep, possibly harmful and more likely in the future I guess! Right?

Extract from the spec:

Each product identifier consists of a name and optional version.
...
A sender SHOULD limit generated product identifiers to what is necessary to identify the product

The reason why we want to identify the destiny product from other (Winden) clients is well known (and still debated). But why would we need this product and version for (today)? Speaking of my own (operational) experience:

I guess the decision will be discuss elsewhere, in the event we choose to define an AgentString following the spec, we might consider something like:

Which is useful... but come at some price.

meejah commented 1 year ago

I think the most-granular that has direct benefit to us is any major version that has different network behavior. For example, if a newer version implemented an as-yet-unimplemented piece of the network protocol.

For example, as deployed it does not currently support re-connecting to the mailbox. So leaking the major version where that starts might be useful. For example, let's just say it was in 1.x.y where it continues to not reconnect, but 2.a.b is where it started: we could include "1" or "2" in the network-advertised version.

Note too that the default mailbox uses ws:// transport so if (when?) we allow users to change that, it's an additional reason to not leak the version.

("Attack surface" I think is the term @btlogy is looking for..?)

wuan commented 1 year ago

We are talking about a client application here. This is the same like the well known User Agent header.

By adding this we even see if users use outdated clients which might be affected by known security issues and can notify them in case.

We will be quite limited by the format, as the client_versions table only contains two string fields implementation and version:

CREATE TABLE `client_versions`
(
 `app_id` VARCHAR,
 `side` VARCHAR, -- for deduplication of reconnects
 `connect_time` INTEGER, -- seconds since epoch, rounded to "blur time"
 -- the client sends us a 'client_version' tuple of (implementation, version)
 -- the Python client sends e.g. ("python", "0.11.0")
 `implementation` VARCHAR,
 `version` VARCHAR
);

Those implementation fields are typical: go-william, python and now winden.app

And for versions we have values like: v1.0.6, 0.1.2, etc.

And the feature in general is already in use. This issue is not about adding this information.

wuan commented 1 year ago

Note too that the default mailbox uses ws:// transport so if (when?) we allow users to change that, it's an additional reason to not leak the version.

@meejah , can you give more details on that line?

meejah commented 1 year ago

Re: the default relay, anything in the network path can read unencrypted websocket traffic (i.e. the exact version headers). So many more devices than "the sever" can see your precise version in that case. Or, what details do you want?

I understand that web browsers like to leak their exact version, but that has also caused a bunch of problems (both security related and non-security).

Ben and I are just trying to inform about best-practices here, which these days lean away from including precise version information (because it can help attackers).

meejah commented 1 year ago

We are talking about a client application here.

It's also p2p.

wuan commented 1 year ago

Two misconceptions here:

The existing magic wormhole clients already set implementation and version fields. If we think that this is a problem we should open tickets to discuss this with the community of the corresponding projects.

wuan commented 1 year ago

We are talking about a client application here.

It's also p2p.

The implementation and version information are used only on the side of the mailbox protocol, which is not P2P. The relay part which can be P2P does not transmit version information.

donpui commented 1 year ago

Adding "why" not to be closed again:

As we touched security perspective, if we know, that specific version is vulnerable, we could theoretically block usage on our mailbox server (yet, we don't have such functionality, but adding if should not be something hard on critical situation).

If there is vulnerable application version, even without knowing exact version, attacker can blindly try exploit. In this particular case, attacker should be mailbox or relay server imposter. Again, preventing usage of such version, will most likely force users to update.

We were already identifying, just we are switching from wormhole-william version to destiny or winden versioning.

Also last security audit didn't detect this as an issue. However we can consult with our security experts to have more opinions.

btlogy commented 1 year ago

Thank you all for sharing. This issue also reminds me that I need to better understand how MW works before weighting on such arguments... Without that knowledge, and assuming we do not plan to block or notify the users if the client is known to be vulnerable, what about only showing major and minor version and drop patch level (if not too difficult)?

Ex: destiny-android/1.0

Even letting the security perspective aside, this could also reduce the chance of identification?

donpui commented 1 year ago

Thank you all for sharing. This issue also reminds me that I need to better understand how MW works before weighting on such arguments... Without that knowledge, and assuming we do not plan to block or notify the users if the client is known to be vulnerable, what about only showing major and minor version and drop patch level (if not too difficult)?

Ex: destiny-android/1.0

Even letting the security perspective aside, this could also reduce the chance of identification?

Learning is part of discussion. We can drop patch version, if it helps feel safer. On statistic side we will have more aggregated usage, which can be sufficient.

wuan commented 1 year ago

Learning is part of discussion. We can drop patch version, if it helps feel safer. On statistic side we will have more aggregated usage, which can be sufficient.

And we get rid of a lot of noise, as some Python version even adds more information here:

image
hacklschorsch commented 1 year ago

Unfortunately I already clicked the message box away, but trying to send for the first time on my Windows 11 laptop gave me, as it should, a big "Do you want to allow this network access..." dialogue by the Windows firewall, and I clicked (I am paraphrasing)

Allow A new Flutter project. access to the internet using

  • [ ] private networks...
  • [x] public networks...

Would be a much more professional look if someone would take 30 mins to update all the strings.

I am running version 1.0.0 according to the Settings dialogue, so I downloaded (from here) and tried to update, but the installer tells me I already am running the latest version:

image
wuan commented 1 year ago

Thanks @hacklschorsch for reporting this. Can you create a separate ticket for that issue. The original issue is about how the apps should advertise details to the server, comparable to a user-agent header.