freenet/fred-staging — pull request #228 by bertm (2014-05-22 11:07:49+00:00)
Title: Probe.respond(LINK_LENGTHS) may yield invalid link lengths, leaking node location

*Regarding /src/freenet/node/probe/Probe.java*

In some cases the link length probe response contains one or more invalid (negative) link lengths. Such cases have been identified to exist in the wild (reportedly, about 1% of all responses include any of such values).

Negative link lengths can be returned when the PeerManager.connectedPeers() is not exactly up-to-date, and includes (disconnected?) peers with invalid location (which is for some reason fixed to -1.0 instead of something more sensible like NaN). In such a case, the negative link length is (approximately) randomly drawn from normal distribution N(–*loc*, (0.01 × *loc*)²), where *loc* denotes the probed node's own location, hence leaking the node's location.

Unless this is intended behaviour (with which I would strongly disagree), this may be trivially solved by using PeerManager.getPeerLocationDoubles(false), which already eliminates invalid peer locations from the list (except for cases involving concurrent modification).

*Aside from the above, observe the following:*
Due to the choice of random perturbation mode (multiplicative gaussion noise), other invalid link lengths (larger than 0.5) may also be produced. Also, because of the multiplicative perturbation, short links are perturbed significantly less than long links (the example speaks of adding 0.002 noise for 1 sigma, but this only holds for link length 0.2 – for link length 1e-3, one sigma only perturbs the link length with 1e-5). In my humble opinion this approach is counterintuitive at best.

To me, more intuitive would be additively perturbing each valid peer location (i.e. adding a 'noise' value from N(0, 0.01²)), normalizing the peer location (i.e. fmod 1.0), then calculating the distance to the node's location. This would also ensure that link lengths reported are always at most 0.5 (and, except for the above situation, also at least 0, but that property already holds in the current multiplicative approach).