Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destiny app should identify itself as destiny #193

Open
donpui opened this issue Nov 16, 2022 · 19 comments
Open

Destiny app should identify itself as destiny #193

donpui opened this issue Nov 16, 2022 · 19 comments
Labels
Tech Debt Technical debt, small improvements to code, libraries

Comments

@donpui
Copy link
Contributor

donpui commented Nov 16, 2022

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

  • AgentString = "go-william"
  • AgentVersion = "v1.0.6"

Propose to identify as:

  • AgentString = "destiny"
  • AgentVersion = "1.0" - where version depends on destiny app release, without patch version

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

  • AgentString = "destiny-android"
  • AgentString = "destiny-linux"
  • AgentString = "destiny-macos"
  • AgentString = "destiny-windows"
  • AgentString = "destiny-ios"

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

@donpui
Copy link
Contributor Author

donpui commented Nov 30, 2022

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

@meejah
Copy link
Collaborator

meejah commented Nov 30, 2022

Ideally we wouldn't leak the exact version either.

@donpui
Copy link
Contributor Author

donpui commented Dec 1, 2022

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
Copy link
Collaborator

meejah commented Dec 2, 2022

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
Copy link
Contributor

wuan commented Dec 2, 2022

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
Copy link
Contributor

btlogy commented Dec 2, 2022

@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:

  • to detect bug related to some specific product/version,
  • to assess the impact of compatibility breaking change on the server,
  • to assess the impact of a security flaw, (just like the exploit scenario above BTW)!

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:

  • Destiny/x.y.z go-william/x.y.z Android/X
  • Destiny/x.y.z go-william/x.y.z IOs/X
  • ...

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

@meejah
Copy link
Collaborator

meejah commented Dec 2, 2022

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
Copy link
Contributor

wuan commented Dec 2, 2022

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
Copy link
Contributor

wuan commented Dec 2, 2022

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
Copy link
Collaborator

meejah commented Dec 2, 2022

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
Copy link
Collaborator

meejah commented Dec 2, 2022

We are talking about a client application here.

It's also p2p.

@wuan
Copy link
Contributor

wuan commented Dec 2, 2022

Two misconceptions here:

  • Winden uses WebSocket over TLS, so there will be no unencrypted traffic and
  • The implementation and version fields are part of the Magic Wormhole protocol, which is hopefully using it's own encryption to protect this data. This is not a Http header.

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
Copy link
Contributor

wuan commented Dec 2, 2022

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
Copy link
Contributor Author

donpui commented Dec 2, 2022

Adding "why" not to be closed again:

  • monitoring agent version allows to understand how many users use one or another version. If majority users still use old version, we could perform additional notifications, marketing campaign, or use other channel to reach users to encourage update (the best would be to have proper update mechanism).

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
Copy link
Contributor

btlogy commented Dec 3, 2022

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
Copy link
Contributor Author

donpui commented Dec 5, 2022

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
Copy link
Contributor

wuan commented Dec 5, 2022

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

@donpui donpui added the Tech Debt Technical debt, small improvements to code, libraries label Dec 16, 2022
@hacklschorsch
Copy link
Member

hacklschorsch commented Apr 25, 2023

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...
  • 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
Copy link
Contributor

wuan commented Apr 25, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Technical debt, small improvements to code, libraries
Projects
None yet
Development

No branches or pull requests

5 participants