February 2018 - tor-commits - lists.torproject.org

[torspec/master] Merge branch 'document_prop278'
by nickm＠torproject.org 09 Feb '18

09 Feb '18

commit 6a649ca3f34c404c3be9107f63bb1011126facc1 Merge: 6bd0a69 1cb56af Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 9 08:38:25 2018 -0500 Merge branch 'document_prop278' dir-spec.txt | 34 ++++++++++++++++------ proposals/000-index.txt | 4 +-- ...78-directory-compression-scheme-negotiation.txt | 2 +- 3 files changed, 28 insertions(+), 12 deletions(-) diff --cc proposals/000-index.txt index 688e2f7,7cfde0f..86fab75 --- a/proposals/000-index.txt +++ b/proposals/000-index.txt @@@ -287,7 -287,7 +287,6 @@@ Proposals by status 217 Tor Extended ORPort Authentication [for 0.2.5.x] 232 Pluggable Transport through SOCKS proxy [in 0.2.6] 260 Rendezvous Single Onion Services [in 0.2.9.3-alpha] - 278 Directory Compression Scheme Negotiation [in 0.3.1.1-alpha] - 284 Hidden Service v3 Control Port CLOSED: 101 Voting on the Tor Directory System [in 0.2.0.x] 102 Dropping "opt" from the directory format [in 0.2.0.x] @@@ -367,7 -367,7 +366,8 @@@ 271 Another algorithm for guard selection [in 0.3.0.1-alpha] 272 Listed routers should be Valid, Running, and treated as such [in 0.2.9.3-alpha, 0.2.9.4-alpha] 274 Rotate onion keys less frequently [in 0.3.1.1-alpha] + 278 Directory Compression Scheme Negotiation [in 0.3.1.1-alpha] + 284 Hidden Service v3 Control Port SUPERSEDED: 112 Bring Back Pathlen Coin Weight 113 Simplifying directory authority administration

1 0

[torspec/master] Add a graf about fingerprinting anonymous directory connections.
by nickm＠torproject.org 09 Feb '18

09 Feb '18

commit 9ddf104ae43c0a1eb88d617cf1a4513af8930561 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 9 08:40:33 2018 -0500 Add a graf about fingerprinting anonymous directory connections. --- dir-spec.txt | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/dir-spec.txt b/dir-spec.txt index 003e400..ece2991 100644 --- a/dir-spec.txt +++ b/dir-spec.txt @@ -3636,11 +3636,16 @@ - "x-tor-lzma" -- The lzma compression algorithm, with a "preset" value no higher than 6. - Clients SHOULD use Accept-Encoding to indicate which of the above - compression algorithms they support. If they omit it (as Tor clients - did before 0.3.1.1-alpha), then the server should serve only "deflate" - or "identity" encoded documents, based on the presence or absence of - the ".z" suffix on the requested URL. + Clients SHOULD use Accept-Encoding on most directory requests to indicate + which of the above compression algorithms they support. If they omit it + (as Tor clients did before 0.3.1.1-alpha), then the server should serve + only "deflate" or "identity" encoded documents, based on the presence or + absence of the ".z" suffix on the requested URL. + + Note that for anonymous directory requests (that is, requests made over + multi-hop circuits, like those for onion service lookups) implementations + SHOULD NOT advertise any Accept-Encoding values other than deflate. To do + so would be to create a fingerprinting opportunity. When receiving multiple documents, clients MUST accept compressed concatenated documents and concatenated compressed documents as

1 0

[tor-browser-spec/master] Addressing Mike's review notes
by gk＠torproject.org 09 Feb '18

09 Feb '18

commit 612532d025c083ee7be793c5d34851138c929d32 Author: Georg Koppen <gk(a)torproject.org> Date: Fri Feb 9 09:40:54 2018 +0000 Addressing Mike's review notes --- design-doc/design.xml | 92 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/design-doc/design.xml b/design-doc/design.xml index 43c4cb9..a58d906 100644 --- a/design-doc/design.xml +++ b/design-doc/design.xml @@ -1301,10 +1301,17 @@ We isolate the content and image cache to the URL bar domain by setting </para> <para> -Furthermore there is the Cache API (CacheStorage). That one is currently not -available in Tor Browser as we do not allow third party cookies and are in -Private Browsing Mode by default.  +Furthermore there is the <ulink url="https://developer.mozilla.org/en-US/docs/Web/API/CacheStorage"> +CacheStorage API</ulink>. That one is currently not available in Tor Browser as +we do not allow third party cookies and are in Private Browsing Mode by default. +As the cache entries are written to disk the CacheStorage API +<ulink url="https://bugzilla.mozilla.org/show_bug.cgi?id=1173467">got disabled</ulink> +in that mode in Firefox, similar to how IndexedDB is handled. There are +<ulink url="https://bugzilla.mozilla.org/show_bug.cgi?id=1117808">thoughts</ulink> +about enabling it by providing a memory-only database but that is still work +in progress. But even if users are leaving the Private Browsing Mode and are +enabling third party cookies the storage is isolated to the URL bar domain by +<command>privacy.firstparty.isolate</command> set to <command>true</command>. </para> <para> Finally, we have the asm.js cache. The cache entry of the script is (among @@ -1337,6 +1344,22 @@ to prevent linkability between sites. We achieve this by setting </para> </listitem> + <listitem><command>IndexedDB Storage</command> + <para> + +IndexedDB storage for third party domains MUST be isolated to the URL bar +domain, to prevent linkability between sites. By default +<ulink url="https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API"> +IndexedDB storage</ulink> is disabled as Tor Browser is using Firefox's Private +Browsing Mode and does not allow third party cookies. There are +<ulink url="https://bugzilla.mozilla.org/show_bug.cgi?id=781982">thoughts</ulink> +about enabling this API in Private Browsing Mode as well but that is still work +in progress. However, if users are leaving this mode and are enabling third +party cookies, isolation to the URL bar is achieved, though, by +<command>privacy.firstparty.isolate</command> set to <command>true</command>. + + </para> + </listitem> <listitem><command>Flash cookies</command> <para><command>Design Goal:</command> @@ -1831,9 +1854,14 @@ and version differences into its analysis of the fingerprintability of a population is largely useless for evaluating either attacks or defenses. Unfortunately, this includes popular large-scale studies such as <ulink url="https://panopticlick.eff.org/">Panopticlick</ulink> and <ulink -url="https://amiunique.org/">Am I Unique</ulink>. - +url="https://amiunique.org/">Am I Unique</ulink>. To gather usable data about +Tor Browser's fingerprinting defenses we launched a Google Summer of Code +project in 2016, called <ulink url="https://github.com/plaperdr/fp-central"> +FPCentral</ulink>, with the aim to provide us an own testbed. We set this up +during 2017 and <ulink url="https://fpcentral.tbb.torproject.org/">have it +available now</ulink> for further integration into our quality assurance efforts +and possible research into improving our fingerprinting defenses and measuring +their effectiveness. </para> </listitem> @@ -1954,9 +1982,9 @@ url="https://panopticlick.eff.org/">Panopticlick</ulink> or <ulink url="https://amiunique.org/">Am I Unique</ulink> could report the entropy and uniqueness rates for all users of a single user agent version, without the need for complicated statistics about the variance of the measured behaviors. - - +<ulink url="https://fpcentral.tbb.torproject.org/fp">FPCentral</ulink> is trying +to achieve that for Tor Browser by providing feedback on acceptable browser +properties and giving guidance on possible improvements. </para> <para> @@ -2471,10 +2499,14 @@ those possible issues with disabling the Battery Status API by setting It is possible to get the system uptime of a Tor Browser user by querying the <command>Event.timestamp</command> property. We avoid this by setting <command> -dom.event.highrestimestamp.enabled</command> to <command>true</command>. - +dom.event.highrestimestamp.enabled</command> to <command>true</command>. This +might seem to be counterintuitive at first glance but the effect of setting +that preference to <code>true</code> is a +<ulink url="https://trac.torproject.org/projects/tor/ticket/17046#comment:8"> +normalization</ulink> of <code>evt.timestamp</code> and +<code>new Event('').timeStamp</code>. Together with clamping the timer +resolution to 100ms this provides an effective means against system uptime +fingerprinting. </para> </listitem> @@ -2491,10 +2523,6 @@ changed by the keyboard layout nor by the modifier state. On the other hand the generated by that key. This is dependent on things like keyboard layout, locale and modifier keys. - - </para> <para><command>Design Goal:</command> @@ -2514,6 +2542,19 @@ are currently returning an empty <command>KeyboardEvent.code</command> and a <command>KeyboardEvent.keyCode</command> of <command>0</command>. Moreover, neither <command>Alt</command> or <command>Shift</command>, or <command>AltGr</command> keyboard events are reported to content. + + </para> + <para> + +We are currently not taking the actually deployed browser locale or the locale +indicated by a loaded document into account when spoofing the keyboard layout. +We think that would be the right thing to do in the longer run, to mitigate +possible usability issues and broken functionality on websites. Similarily to +how users of non-english Tor Browser bundles right now can choose between +keeping the Accept header spoofed or not they would then be able to keep a +spoofed english keyboard or a spoofed one depending on the actual Tor Browser +locale or language of the document. + </para> </listitem> <listitem><command>User Agent and HTTP Headers</command> @@ -2588,7 +2629,7 @@ and <command>document.timeline.currentTime</command>. </para> <para> -While clamping the clock resolution to 100ms is a step towards mitigating +While clamping the clock resolution to 100ms is a step towards mitigating timing-based side channel fingerprinting, it is by no means sufficient. It turns out that it is possible to subvert our clamping of explicit clocks by using <ulink url="https://www.usenix.org/system/files/conference/usenixsecurity16/sec16_paper…"> @@ -2838,8 +2879,9 @@ is a Firefox feature to view web pages clutter-free and easily adjusted to own needs and preferences. To avoid fingerprintability risks we make Tor Browser users uniform by setting <command>reader.parse-on-load.enabled</command> to <command>false</command> and <command>browser.reader.detectedFirstArticle</command> -to <command>true</command>. - +to <command>true</command>. This makes sure that documents are not parsed on +load as this is disabled on some devices due to memory consumption and we +pretend that everybody has already been using that feature in the past. </para> </listitem> @@ -3010,10 +3052,10 @@ After closing all tabs, we then clear the searchbox and findbox text and emit state). Then we manually clear the following state: HTTP auth, SSL state, crypto tokens, OCSP state, site-specific content preferences (including HSTS state), the undo tab history, content and image cache, offline and memory cache, -offline storage, IndexedDB storage, asm.js cache, cookies, DOM storage, the -safe browsing key, the Google wifi geolocation token (if it exists), and the -domain isolator state. We also clear NoScript's site and temporary permissions, -and all other browser site permissions. +offline storage, Cache storage, IndexedDB storage, asm.js cache, cookies, +DOM storage, the safe browsing key, the Google wifi geolocation token (if it +exists), and the domain isolator state. We also clear NoScript's site and +temporary permissions, and all other browser site permissions. </para> <para>

1 0

[metrics-web/master] Point out that OnionPerf measures v2 onion services.
by karsten＠torproject.org 09 Feb '18

09 Feb '18

commit a3dd5b4eba62f875dac934575ef671c63c4f5b10 Author: Karsten Loesing <karsten.loesing(a)gmx.net> Date: Thu Feb 8 17:06:41 2018 +0100 Point out that OnionPerf measures v2 onion services. --- src/main/resources/web/json/metrics.json | 4 ++-- src/main/resources/web/jsps/stats.jsp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/resources/web/json/metrics.json b/src/main/resources/web/json/metrics.json index 61d51b9..0b8aca7 100644 --- a/src/main/resources/web/json/metrics.json +++ b/src/main/resources/web/json/metrics.json @@ -348,7 +348,7 @@ "id": "torperf", "title": "Time to download files over Tor", "type": "Graph", - "description": "This graph shows overall performance when downloading static files of different sizes over Tor. The graph shows the range of measurements from first to third quartile, and highlights the median. The slowest and fastest quarter of measurements are omitted from the graph.", + "description": "This graph shows overall performance when downloading static files of different sizes over Tor, either from a server on the public internet or from a version 2 onion server. The graph shows the range of measurements from first to third quartile, and highlights the median. The slowest and fastest quarter of measurements are omitted from the graph.", "function": "plot_torperf", "parameters": [ "start", @@ -365,7 +365,7 @@ "id": "torperf-failures", "title": "Timeouts and failures of downloading files over Tor", "type": "Graph", - "description": "This graph shows the fraction of timeouts and failures when downloading static files of different sizes over Tor. A timeout occurs when a download does not complete within the scheduled time, in which case it is aborted in order not to overlap with the next scheduled download. A failure occurs when the download completes, but the response is smaller than expected.", + "description": "This graph shows the fraction of timeouts and failures when downloading static files of different sizes over Tor, either from a server on the public internet or from a version 2 onion server. A timeout occurs when a download does not complete within the scheduled time, in which case it is aborted in order not to overlap with the next scheduled download. A failure occurs when the download completes, but the response is smaller than expected.", "function": "plot_torperf_failures", "parameters": [ "start", diff --git a/src/main/resources/web/jsps/stats.jsp b/src/main/resources/web/jsps/stats.jsp index c8fef15..c224a06 100644 --- a/src/main/resources/web/jsps/stats.jsp +++ b/src/main/resources/web/jsps/stats.jsp @@ -401,8 +401,8 @@ measurements. If this column contains the empty string, all measurements are included, regardless of which service performed them.</li> <li>server: Either "public" if the request was made to a server on -the public internet, or "onion" if the request was made to an onion -server.</li> +the public internet, or "onion" if the request was made to a version 2 +onion server.</li> <li>q1: First quartile of time until receiving the last byte in milliseconds.</li>

1 0

[tor/master] Wrap types in protover.rs.
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit 124caf28e6db1e7bf8cdfef25c55760c81fb91b5 Author: Corey Farwell <coreyf(a)rwell.org> Date: Wed Dec 20 23:34:05 2017 -0500 Wrap types in protover.rs. https://trac.torproject.org/projects/tor/ticket/24030 Introduce new wrapper types: - `SupportedProtocols` - `Versions` Introduce a type alias: - `Version` (`u32`) --- src/rust/protover/protover.rs | 307 ++++++++++++++++++++++-------------------- 1 file changed, 164 insertions(+), 143 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 5e281a3e9..af0049a41 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -98,94 +98,120 @@ pub fn get_supported_protocols() -> String { SUPPORTED_PROTOCOLS.join(" ") } -/// Translates a vector representation of a protocol list into a HashMap -fn parse_protocols<P, S>( - protocols: P, -) -> Result<HashMap<Proto, HashSet<u32>>, &'static str> -where - P: Iterator<Item = S>, - S: AsRef<str>, -{ - let mut parsed = HashMap::new(); - - for subproto in protocols { - let (name, version) = get_proto_and_vers(subproto.as_ref())?; - parsed.insert(name, version); +pub struct SupportedProtocols(HashMap<Proto, Versions>); + +impl SupportedProtocols { + /// # Examples + /// + /// ``` + /// use protover::SupportedProtocols; + /// + /// let supported_protocols = SupportedProtocols::from_proto_entries_string( + /// "HSDir=1-2 HSIntro=3-4" + /// ); + /// ``` + pub fn from_proto_entries_string( + proto_entries: &str, + ) -> Result<Self, &'static str> { + Self::from_proto_entries(proto_entries.split(" ")) } - Ok(parsed) -} -/// Translates a string representation of a protocol list to a HashMap -fn parse_protocols_from_string<'a>( - protocol_string: &'a str, -) -> Result<HashMap<Proto, HashSet<u32>>, &'static str> { - parse_protocols(protocol_string.split(" ")) -} - -/// Translates supported tor versions from a string into a HashMap, which is -/// useful when looking up a specific subprotocol. -/// -/// # Returns -/// -/// A `Result` whose `Ok` value is a `HashMap<Proto, <u32>>` holding all -/// subprotocols and versions currently supported by tor. -/// -/// The returned `Result`'s `Err` value is an `&'static str` with a description -/// of the error. -/// -fn tor_supported() -> Result<HashMap<Proto, HashSet<u32>>, &'static str> { - parse_protocols(SUPPORTED_PROTOCOLS.iter()) -} + /// ``` + /// use protover::SupportedProtocols; + /// + /// let supported_protocols = SupportedProtocols::from_proto_entries([ + /// "HSDir=1-2", + /// "HSIntro=3-4", + /// ].iter()); + /// ``` + pub fn from_proto_entries<I, S>(protocol_strs: I) -> Result<Self, &'static str> + where + I: Iterator<Item = S>, + S: AsRef<str>, + { + let mut parsed = HashMap::new(); + for subproto in protocol_strs { + let (name, version) = get_proto_and_vers(subproto.as_ref())?; + parsed.insert(name, version); + } + Ok(SupportedProtocols(parsed)) + } -/// Get the unique version numbers supported by a subprotocol. -/// -/// # Inputs -/// -/// * `version_string`, a string comprised of "[0-9,-]" -/// -/// # Returns -/// -/// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique -/// version numbers. If there were ranges in the `version_string`, then these -/// are expanded, i.e. `"1-3"` would expand to `HashSet<u32>::new([1, 2, 3])`. -/// The returned HashSet is *unordered*. -/// -/// The returned `Result`'s `Err` value is an `&'static str` with a description -/// of the error. -/// -/// # Errors -/// -/// This function will error if: -/// -/// * the `version_string` is empty or contains an equals (`"="`) sign, -/// * the expansion of a version range produces an error (see -/// `expand_version_range`), -/// * any single version number is not parseable as an `u32` in radix 10, or -/// * there are greater than 2^16 version numbers to expand. -/// -fn get_versions(version_string: &str) -> Result<HashSet<u32>, &'static str> { - if version_string.is_empty() { - return Err("version string is empty"); + /// Translates supported tor versions from a string into a HashMap, which + /// is useful when looking up a specific subprotocol. + /// + /// # Returns + /// + /// A `Result` whose `Ok` value is a `HashMap<Proto, <Version>>` holding all + /// subprotocols and versions currently supported by tor. + /// + /// The returned `Result`'s `Err` value is an `&'static str` with a + /// description of the error. + /// + fn tor_supported() -> Result<Self, &'static str> { + Self::from_proto_entries(SUPPORTED_PROTOCOLS.iter().map(|n| *n)) } +} - let mut versions = HashSet::<u32>::new(); +type Version = u32; + +/// Set of versions for a protocol. +#[derive(Debug, PartialEq, Eq)] +pub struct Versions(HashSet<Version>); + +impl Versions { + /// Get the unique version numbers supported by a subprotocol. + /// + /// # Inputs + /// + /// * `version_string`, a string comprised of "[0-9,-]" + /// + /// # Returns + /// + /// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique + /// version numbers. If there were ranges in the `version_string`, then these + /// are expanded, i.e. `"1-3"` would expand to `HashSet<u32>::new([1, 2, 3])`. + /// The returned HashSet is *unordered*. + /// + /// The returned `Result`'s `Err` value is an `&'static str` with a description + /// of the error. + /// + /// # Errors + /// + /// This function will error if: + /// + /// * the `version_string` is empty or contains an equals (`"="`) sign, + /// * the expansion of a version range produces an error (see + /// `expand_version_range`), + /// * any single version number is not parseable as an `u32` in radix 10, or + /// * there are greater than 2^16 version numbers to expand. + /// + fn from_version_string( + version_string: &str, + ) -> Result<Self, &'static str> { + if version_string.is_empty() { + return Err("version string is empty"); + } - for piece in version_string.split(",") { - if piece.contains("-") { - for p in expand_version_range(piece)? { - versions.insert(p); + let mut versions = HashSet::<Version>::new(); + + for piece in version_string.split(",") { + if piece.contains("-") { + for p in expand_version_range(piece)? { + versions.insert(p); + } + } else { + versions.insert(u32::from_str(piece).or( + Err("invalid protocol entry"), + )?); } - } else { - versions.insert(u32::from_str(piece).or( - Err("invalid protocol entry"), - )?); - } - if versions.len() > MAX_PROTOCOLS_TO_EXPAND as usize { - return Err("Too many versions to expand"); + if versions.len() > MAX_PROTOCOLS_TO_EXPAND as usize { + return Err("Too many versions to expand"); + } } + Ok(Versions(versions)) } - Ok(versions) } @@ -205,7 +231,7 @@ fn get_versions(version_string: &str) -> Result<HashSet<u32>, &'static str> { /// fn get_proto_and_vers<'a>( protocol_entry: &'a str, -) -> Result<(Proto, HashSet<u32>), &'static str> { +) -> Result<(Proto, Versions), &'static str> { let mut parts = protocol_entry.splitn(2, "="); let proto = match parts.next() { @@ -218,7 +244,7 @@ fn get_proto_and_vers<'a>( None => return Err("invalid protover entry"), }; - let versions = get_versions(vers)?; + let versions = Versions::from_version_string(vers)?; let proto_name = proto.parse()?; Ok((proto_name, versions)) @@ -245,19 +271,18 @@ fn contains_only_supported_protocols(proto_entry: &str) -> bool { Err(_) => return false, }; - let currently_supported: HashMap<Proto, HashSet<u32>> = - match tor_supported() { - Ok(n) => n, - Err(_) => return false, - }; + let currently_supported = match SupportedProtocols::tor_supported() { + Ok(n) => n.0, + Err(_) => return false, + }; let supported_versions = match currently_supported.get(&name) { Some(n) => n, None => return false, }; - vers.retain(|x| !supported_versions.contains(x)); - vers.is_empty() + vers.0.retain(|x| !supported_versions.0.contains(x)); + vers.0.is_empty() } /// Determine if we support every protocol a client supports, and if not, @@ -303,7 +328,7 @@ pub fn all_supported(protocols: &str) -> (bool, String) { /// /// * `list`, a string representation of a list of protocol entries. /// * `proto`, a `Proto` to test support for -/// * `vers`, a `u32` version which we will go on to determine whether the +/// * `vers`, a `Version` version which we will go on to determine whether the /// specified protocol supports. /// /// # Examples @@ -321,21 +346,19 @@ pub fn all_supported(protocols: &str) -> (bool, String) { pub fn protover_string_supports_protocol( list: &str, proto: Proto, - vers: u32, + vers: Version, ) -> bool { - let supported: HashMap<Proto, HashSet<u32>>; - - match parse_protocols_from_string(list) { - Ok(result) => supported = result, + let supported = match SupportedProtocols::from_proto_entries_string(list) { + Ok(result) => result.0, Err(_) => return false, - } + }; let supported_versions = match supported.get(&proto) { Some(n) => n, None => return false, }; - supported_versions.contains(&vers) + supported_versions.0.contains(&vers) } /// As protover_string_supports_protocol(), but also returns True if @@ -365,23 +388,21 @@ pub fn protover_string_supports_protocol_or_later( proto: Proto, vers: u32, ) -> bool { - let supported: HashMap<Proto, HashSet<u32>>; - - match parse_protocols_from_string(list) { - Ok(result) => supported = result, + let supported = match SupportedProtocols::from_proto_entries_string(list) { + Ok(result) => result.0, Err(_) => return false, - } + }; let supported_versions = match supported.get(&proto) { Some(n) => n, None => return false, }; - supported_versions.iter().any(|v| v >= &vers) + supported_versions.0.iter().any(|v| v >= &vers) } /// Fully expand a version range. For example, 1-3 expands to 1,2,3 -/// Helper for get_versions +/// Helper for Versions::from_version_string /// /// # Inputs /// @@ -483,10 +504,9 @@ fn find_range(list: &Vec<u32>) -> (bool, u32) { /// /// A `String` representation of this set in ascending order. /// -fn contract_protocol_list<'a>(supported_set: &'a HashSet<u32>) -> String { - let mut supported: Vec<u32> = supported_set.iter() - .map(|x| *x) - .collect(); +fn contract_protocol_list<'a>(supported_set: &'a HashSet<Version>) -> String { + let mut supported: Vec<Version> = + supported_set.iter().map(|x| *x).collect(); supported.sort(); let mut final_output: Vec<String> = Vec::new(); @@ -522,8 +542,8 @@ fn contract_protocol_list<'a>(supported_set: &'a HashSet<u32>) -> String { /// /// # Returns /// -/// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique -/// version numbers. +/// A `Result` whose `Ok` value is a `HashSet<Version>` holding all of the +/// unique version numbers. /// /// The returned `Result`'s `Err` value is an `&'static str` with a description /// of the error. @@ -534,12 +554,12 @@ fn contract_protocol_list<'a>(supported_set: &'a HashSet<u32>) -> String { /// /// * The protocol string does not follow the "protocol_name=version_list" /// expected format -/// * If the version string is malformed. See `get_versions`. +/// * If the version string is malformed. See `Versions::from_version_string`. /// fn parse_protocols_from_string_with_no_validation<'a>( protocol_string: &'a str, -) -> Result<HashMap<String, HashSet<u32>>, &'static str> { - let mut parsed: HashMap<String, HashSet<u32>> = HashMap::new(); +) -> Result<HashMap<String, Versions>, &'static str> { + let mut parsed: HashMap<String, Versions> = HashMap::new(); for subproto in protocol_string.split(" ") { let mut parts = subproto.splitn(2, "="); @@ -554,7 +574,7 @@ fn parse_protocols_from_string_with_no_validation<'a>( None => return Err("invalid protover entry"), }; - let versions = get_versions(vers)?; + let versions = Versions::from_version_string(vers)?; parsed.insert(String::from(name), versions); } @@ -602,21 +622,22 @@ pub fn compute_vote( // } // means that FirstSupportedProtocol has three votes which support version // 1, and one vote that supports version 2 - let mut all_count: HashMap<String, HashMap<u32, usize>> = HashMap::new(); + let mut all_count: HashMap<String, HashMap<Version, usize>> = + HashMap::new(); // parse and collect all of the protos and their versions and collect them for vote in list_of_proto_strings { - let this_vote: HashMap<String, HashSet<u32>> = + let this_vote: HashMap<String, Versions> = match parse_protocols_from_string_with_no_validation(&vote) { Ok(result) => result, Err(_) => continue, }; for (protocol, versions) in this_vote { - let supported_vers: &mut HashMap<u32, usize> = + let supported_vers: &mut HashMap<Version, usize> = all_count.entry(protocol).or_insert(HashMap::new()); - for version in versions { + for version in versions.0 { let counter: &mut usize = supported_vers.entry(version).or_insert(0); *counter += 1; @@ -690,20 +711,18 @@ fn write_vote_to_string(vote: &HashMap<String, String>) -> String { /// let is_supported = is_supported_here(Proto::Link, 1); /// assert_eq!(true, is_supported); /// ``` -pub fn is_supported_here(proto: Proto, vers: u32) -> bool { - let currently_supported: HashMap<Proto, HashSet<u32>>; - - match tor_supported() { - Ok(result) => currently_supported = result, +pub fn is_supported_here(proto: Proto, vers: Version) -> bool { + let currently_supported = match SupportedProtocols::tor_supported() { + Ok(result) => result.0, Err(_) => return false, - } + }; let supported_versions = match currently_supported.get(&proto) { Some(n) => n, None => return false, }; - supported_versions.contains(&vers) + supported_versions.0.contains(&vers) } /// Older versions of Tor cannot infer their own subprotocols @@ -755,48 +774,50 @@ pub fn compute_for_old_tor(version: &str) -> String { #[cfg(test)] mod test { + use super::Version; + #[test] - fn test_get_versions() { + fn test_versions_from_version_string() { use std::collections::HashSet; - use super::get_versions; + use super::Versions; - assert_eq!(Err("version string is empty"), get_versions("")); - assert_eq!(Err("invalid protocol entry"), get_versions("a,b")); - assert_eq!(Err("invalid protocol entry"), get_versions("1,!")); + assert_eq!(Err("version string is empty"), Versions::from_version_string("")); + assert_eq!(Err("invalid protocol entry"), Versions::from_version_string("a,b")); + assert_eq!(Err("invalid protocol entry"), Versions::from_version_string("1,!")); { - let mut versions: HashSet<u32> = HashSet::new(); + let mut versions: HashSet<Version> = HashSet::new(); versions.insert(1); - assert_eq!(Ok(versions), get_versions("1")); + assert_eq!(versions, Versions::from_version_string("1").unwrap().0); } { - let mut versions: HashSet<u32> = HashSet::new(); + let mut versions: HashSet<Version> = HashSet::new(); versions.insert(1); versions.insert(2); - assert_eq!(Ok(versions), get_versions("1,2")); + assert_eq!(versions, Versions::from_version_string("1,2").unwrap().0); } { - let mut versions: HashSet<u32> = HashSet::new(); + let mut versions: HashSet<Version> = HashSet::new(); versions.insert(1); versions.insert(2); versions.insert(3); - assert_eq!(Ok(versions), get_versions("1-3")); + assert_eq!(versions, Versions::from_version_string("1-3").unwrap().0); } { - let mut versions: HashSet<u32> = HashSet::new(); + let mut versions: HashSet<Version> = HashSet::new(); versions.insert(1); versions.insert(2); versions.insert(5); - assert_eq!(Ok(versions), get_versions("1-2,5")); + assert_eq!(versions, Versions::from_version_string("1-2,5").unwrap().0); } { - let mut versions: HashSet<u32> = HashSet::new(); + let mut versions: HashSet<Version> = HashSet::new(); versions.insert(1); versions.insert(3); versions.insert(4); versions.insert(5); - assert_eq!(Ok(versions), get_versions("1,3-5")); + assert_eq!(versions, Versions::from_version_string("1,3-5").unwrap().0); } } @@ -852,7 +873,7 @@ mod test { use super::contract_protocol_list; { - let mut versions = HashSet::<u32>::new(); + let mut versions = HashSet::<Version>::new(); assert_eq!(String::from(""), contract_protocol_list(&versions)); versions.insert(1); @@ -863,14 +884,14 @@ mod test { } { - let mut versions = HashSet::<u32>::new(); + let mut versions = HashSet::<Version>::new(); versions.insert(1); versions.insert(3); assert_eq!(String::from("1,3"), contract_protocol_list(&versions)); } { - let mut versions = HashSet::<u32>::new(); + let mut versions = HashSet::<Version>::new(); versions.insert(1); versions.insert(2); versions.insert(3); @@ -879,7 +900,7 @@ mod test { } { - let mut versions = HashSet::<u32>::new(); + let mut versions = HashSet::<Version>::new(); versions.insert(1); versions.insert(3); versions.insert(5); @@ -892,7 +913,7 @@ mod test { } { - let mut versions = HashSet::<u32>::new(); + let mut versions = HashSet::<Version>::new(); versions.insert(1); versions.insert(2); versions.insert(3);

1 0

[tor/master] Merge remote-tracking branch 'frewsxcv/frewsxcv-protover'
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit d9826b0a30f42754dc5764ce02c7b0271d996c92 Merge: af049657e 124caf28e Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 8 17:45:17 2018 -0500 Merge remote-tracking branch 'frewsxcv/frewsxcv-protover' src/rust/protover/protover.rs | 295 ++++++++++++++++++++++-------------------- 1 file changed, 152 insertions(+), 143 deletions(-) diff --cc src/rust/protover/protover.rs index 069b1088c,af0049a41..826f1b73f --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@@ -105,102 -94,124 +105,116 @@@ impl FromStr for Proto /// /// "HSDir=1-1 LinkAuth=1" /// -pub fn get_supported_protocols() -> String { - SUPPORTED_PROTOCOLS.join(" ") +pub fn get_supported_protocols() -> &'static str { + // The `len() - 1` is to remove the NUL byte. + // The `unwrap` is safe becauase we SUPPORTED_PROTOCOLS is under + // our control. + str::from_utf8(&SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1]) + .unwrap() } - /// Translates a vector representation of a protocol list into a HashMap - fn parse_protocols<P, S>( - protocols: P, - ) -> Result<HashMap<Proto, HashSet<u32>>, &'static str> - where - P: Iterator<Item = S>, - S: AsRef<str>, - { - let mut parsed = HashMap::new(); - - for subproto in protocols { - let (name, version) = get_proto_and_vers(subproto.as_ref())?; - parsed.insert(name, version); + pub struct SupportedProtocols(HashMap<Proto, Versions>); + + impl SupportedProtocols { - /// # Examples - /// - /// ``` - /// use protover::SupportedProtocols; - /// - /// let supported_protocols = SupportedProtocols::from_proto_entries_string( - /// "HSDir=1-2 HSIntro=3-4" - /// ); - /// ``` - pub fn from_proto_entries_string( - proto_entries: &str, - ) -> Result<Self, &'static str> { - Self::from_proto_entries(proto_entries.split(" ")) - } - - /// ``` - /// use protover::SupportedProtocols; - /// - /// let supported_protocols = SupportedProtocols::from_proto_entries([ - /// "HSDir=1-2", - /// "HSIntro=3-4", - /// ].iter()); - /// ``` + pub fn from_proto_entries<I, S>(protocol_strs: I) -> Result<Self, &'static str> + where + I: Iterator<Item = S>, + S: AsRef<str>, + { + let mut parsed = HashMap::new(); + for subproto in protocol_strs { + let (name, version) = get_proto_and_vers(subproto.as_ref())?; + parsed.insert(name, version); + } + Ok(SupportedProtocols(parsed)) } - Ok(parsed) - } - /// Translates a string representation of a protocol list to a HashMap - fn parse_protocols_from_string<'a>( - protocol_string: &'a str, - ) -> Result<HashMap<Proto, HashSet<u32>>, &'static str> { - parse_protocols(protocol_string.split(" ")) - } - - /// Translates supported tor versions from a string into a HashMap, which is - /// useful when looking up a specific subprotocol. - /// - /// # Returns - /// - /// A `Result` whose `Ok` value is a `HashMap<Proto, <u32>>` holding all - /// subprotocols and versions currently supported by tor. - /// - /// The returned `Result`'s `Err` value is an `&'static str` with a description - /// of the error. - /// - fn tor_supported() -> Result<HashMap<Proto, HashSet<u32>>, &'static str> { - parse_protocols(get_supported_protocols().split(" ")) - } - /// Translates supported tor versions from a string into a HashMap, which - /// is useful when looking up a specific subprotocol. ++ /// Translates a string representation of a protocol list to a ++ /// SupportedProtocols instance. + /// - /// # Returns ++ /// # Examples + /// - /// A `Result` whose `Ok` value is a `HashMap<Proto, <Version>>` holding all - /// subprotocols and versions currently supported by tor. ++ /// ``` ++ /// use protover::SupportedProtocols; + /// - /// The returned `Result`'s `Err` value is an `&'static str` with a - /// description of the error. ++ /// let supported_protocols = SupportedProtocols::from_proto_entries_string( ++ /// "HSDir=1-2 HSIntro=3-4" ++ /// ); ++ /// ``` ++ pub fn from_proto_entries_string( ++ proto_entries: &str, ++ ) -> Result<Self, &'static str> { ++ Self::from_proto_entries(proto_entries.split(" ")) ++ } + - /// Get the unique version numbers supported by a subprotocol. - /// - /// # Inputs - /// - /// * `version_string`, a string comprised of "[0-9,-]" - /// - /// # Returns - /// - /// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique - /// version numbers. If there were ranges in the `version_string`, then these - /// are expanded, i.e. `"1-3"` would expand to `HashSet<u32>::new([1, 2, 3])`. - /// The returned HashSet is *unordered*. - /// - /// The returned `Result`'s `Err` value is an `&'static str` with a description - /// of the error. - /// - /// # Errors - /// - /// This function will error if: - /// - /// * the `version_string` is empty or contains an equals (`"="`) sign, - /// * the expansion of a version range produces an error (see - /// `expand_version_range`), - /// * any single version number is not parseable as an `u32` in radix 10, or - /// * there are greater than 2^16 version numbers to expand. - /// - fn get_versions(version_string: &str) -> Result<HashSet<u32>, &'static str> { - if version_string.is_empty() { - return Err("version string is empty"); ++ /// Translate the supported tor versions from a string into a ++ /// HashMap, which is useful when looking up a specific ++ /// subprotocol. + /// + fn tor_supported() -> Result<Self, &'static str> { - Self::from_proto_entries(SUPPORTED_PROTOCOLS.iter().map(|n| *n)) ++ Self::from_proto_entries_string(get_supported_protocols()) } + } - let mut versions = HashSet::<u32>::new(); + type Version = u32; + + /// Set of versions for a protocol. + #[derive(Debug, PartialEq, Eq)] + pub struct Versions(HashSet<Version>); + + impl Versions { + /// Get the unique version numbers supported by a subprotocol. + /// + /// # Inputs + /// + /// * `version_string`, a string comprised of "[0-9,-]" + /// + /// # Returns + /// + /// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique + /// version numbers. If there were ranges in the `version_string`, then these + /// are expanded, i.e. `"1-3"` would expand to `HashSet<u32>::new([1, 2, 3])`. + /// The returned HashSet is *unordered*. + /// + /// The returned `Result`'s `Err` value is an `&'static str` with a description + /// of the error. + /// + /// # Errors + /// + /// This function will error if: + /// + /// * the `version_string` is empty or contains an equals (`"="`) sign, + /// * the expansion of a version range produces an error (see + /// `expand_version_range`), + /// * any single version number is not parseable as an `u32` in radix 10, or + /// * there are greater than 2^16 version numbers to expand. + /// + fn from_version_string( + version_string: &str, + ) -> Result<Self, &'static str> { + if version_string.is_empty() { + return Err("version string is empty"); + } - for piece in version_string.split(",") { - if piece.contains("-") { - for p in expand_version_range(piece)? { - versions.insert(p); + let mut versions = HashSet::<Version>::new(); + + for piece in version_string.split(",") { + if piece.contains("-") { + for p in expand_version_range(piece)? { + versions.insert(p); + } + } else { + versions.insert(u32::from_str(piece).or( + Err("invalid protocol entry"), + )?); } - } else { - versions.insert(u32::from_str(piece).or( - Err("invalid protocol entry"), - )?); - } - if versions.len() > MAX_PROTOCOLS_TO_EXPAND as usize { - return Err("Too many versions to expand"); + if versions.len() > MAX_PROTOCOLS_TO_EXPAND as usize { + return Err("Too many versions to expand"); + } } + Ok(Versions(versions)) } - Ok(versions) }

1 0

[tor/master] Stop claiming that compute_for_old_tor() returns pairs
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit af049657eb4426f4c1e7c8aa603c6303c9a884cf Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 8 17:36:08 2018 -0500 Stop claiming that compute_for_old_tor() returns pairs --- src/rust/protover/protover.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index f3a5ea23e..069b1088c 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -730,8 +730,8 @@ pub fn is_supported_here(proto: Proto, vers: u32) -> bool { /// /// # Returns /// -/// A `&'static [u8]` whose value is series of pairs, comprising of the protocol name -/// and versions that it supports. The string takes the following format: +/// A `&'static [u8]` encoding a list of protocol names and supported +/// versions. The string takes the following format: /// /// "HSDir=1-1 LinkAuth=1" ///

1 0

[tor/master] protover: Fix memleak in Rust implementation.
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit 7ea9e080c58f16fecefa5a0a20406635a2034366 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Feb 6 02:31:04 2018 +0000 protover: Fix memleak in Rust implementation. * FIXES #25127: https://bugs.torproject.org/25127. --- changes/bug25127 | 7 +++++++ src/rust/protover/ffi.rs | 29 ++++++++++++++++---------- src/rust/protover/protover.rs | 47 +++++++++++++++++++++++++++---------------- 3 files changed, 55 insertions(+), 28 deletions(-) diff --git a/changes/bug25127 b/changes/bug25127 new file mode 100644 index 000000000..3438ed125 --- /dev/null +++ b/changes/bug25127 @@ -0,0 +1,7 @@ + o Minor bugfixes (Rust FFI): + - Fix a minor memory leak which would happen whenever the C code would call + the Rust implementation of protover_get_supported_protocols(). This was + due to the C version returning a static string, whereas the Rust version + newly allocated a CString to pass accross the FFI boundary. Consequently, + the C code was not expecting to need to free() what it was given. Fixes + bug 25127; bugfix on 0.3.2.1-alpha. diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 3eb22c933..5fefa8f7c 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -137,18 +137,25 @@ pub extern "C" fn protocol_list_supports_protocol_or_later( /// Provide an interface for C to translate arguments and return types for /// protover::get_supported_protocols #[no_mangle] -pub extern "C" fn protover_get_supported_protocols() -> *mut c_char { - // Not handling errors when unwrapping as the content is controlled - // and is an empty string - let empty = CString::new("").unwrap(); - - let supported = get_supported_protocols(); - let c_supported = match CString::new(supported) { - Ok(n) => n, - Err(_) => return empty.into_raw(), - }; +pub extern "C" fn protover_get_supported_protocols() -> *const c_char { + let supported: &'static CStr; + + // If we're going to pass it to C, there cannot be any intermediate NUL + // bytes. An assert is okay here, since changing the const byte slice + // in protover.rs to contain a NUL byte somewhere in the middle would be a + // programming error. + assert!(!SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1].contains(&0x00)); + assert!(SUPPORTED_PROTOCOLS[SUPPORTED_PROTOCOLS.len() - 1] == 0x00); + + // It's okay to call the "unchecked" version of the function because + // we can see that the bytes we're passing into it 1) are valid UTF-8, + // 2) have no intermediate NUL bytes, and 3) are terminated with a NUL + // byte. + unsafe { + supported = CStr::from_bytes_with_nul_unchecked(SUPPORTED_PROTOCOLS); + } - c_supported.into_raw() + supported.as_ptr() } /// Provide an interface for C to translate arguments and return types for diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index cf6fb78e2..00fe08408 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -3,12 +3,14 @@ use external::c_tor_version_as_new_as; +use std::str; use std::str::FromStr; use std::fmt; use std::collections::{HashMap, HashSet}; use std::ops::Range; use std::string::String; + /// The first version of Tor that included "proto" entries in its descriptors. /// Authorities should use this to decide whether to guess proto lines. /// @@ -22,21 +24,29 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` const MAX_PROTOCOLS_TO_EXPAND: u32 = 500; -/// Currently supported protocols and their versions +/// Currently supported protocols and their versions, as a byte-slice. +/// +/// # Warning +/// +/// This byte-slice ends in a NUL byte. This is so that we can directly convert +/// it to an `&'static CStr` in the FFI code, in order to hand the static string +/// to C in a way that is compatible with C static strings. +/// +/// Rust code which wishes to accesses this string should use +/// `protover::get_supported_protocols()` instead. /// /// C_RUST_COUPLED: src/or/protover.c `protover_get_supported_protocols` -const SUPPORTED_PROTOCOLS: &'static [&'static str] = &[ - "Cons=1-2", - "Desc=1-2", - "DirCache=1-2", - "HSDir=1-2", - "HSIntro=3-4", - "HSRend=1-2", - "Link=1-5", - "LinkAuth=1,3", - "Microdesc=1-2", - "Relay=1-2", -]; +pub(crate) const SUPPORTED_PROTOCOLS: &'static [u8] = + b"Cons=1-2 \ + Desc=1-2 \ + DirCache=1-2 \ + HSDir=1-2 \ + HSIntro=3-4 \ + HSRend=1-2 \ + Link=1-5 \ + LinkAuth=1,3 \ + Microdesc=1-2 \ + Relay=1-2\0"; /// Known subprotocols in Tor. Indicates which subprotocol a relay supports. /// @@ -94,8 +104,11 @@ impl FromStr for Proto { /// /// "HSDir=1-1 LinkAuth=1" /// -pub fn get_supported_protocols() -> String { - SUPPORTED_PROTOCOLS.join(" ") +pub fn get_supported_protocols() -> &'static str { + unsafe { + // The `len() - 1` is to remove the NUL byte. + str::from_utf8_unchecked(&SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1]) + } } /// Translates a vector representation of a protocol list into a HashMap @@ -134,7 +147,7 @@ fn parse_protocols_from_string<'a>( /// of the error. /// fn tor_supported() -> Result<HashMap<Proto, HashSet<u32>>, &'static str> { - parse_protocols(SUPPORTED_PROTOCOLS.iter()) + parse_protocols(get_supported_protocols().split(" ")) } /// Get the unique version numbers supported by a subprotocol. @@ -625,7 +638,7 @@ pub fn compute_vote( } let mut final_output: HashMap<String, String> = - HashMap::with_capacity(SUPPORTED_PROTOCOLS.len()); + HashMap::with_capacity(get_supported_protocols().split(" ").count()); // Go through and remove verstions that are less than the threshold for (protocol, versions) in all_count {

1 0

[tor/master] protover: Fix memleak in Rust impl of protover_compute_for_old_tor.
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit b85436c5961d1b2168c3954a7e718e03a121473f Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Feb 6 14:15:33 2018 +0000 protover: Fix memleak in Rust impl of protover_compute_for_old_tor. * FIXES #25127: https://bugs.torproject.org/25127 * ADDS a new module to the Rust tor_util crate for small utilities for working with static strings between languages. * CHANGES the return type of protover_compute_for_old_tor to point to immutable data. * CHANGES the code from the previous commit to use the new static string utilities. --- src/rust/protover/ffi.rs | 40 ++++++++++++++------- src/rust/protover/lib.rs | 1 + src/rust/protover/protover.rs | 33 ++++++++--------- src/rust/tor_util/lib.rs | 2 +- src/rust/tor_util/strings.rs | 82 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 32 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 5fefa8f7c..5519b75ea 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -12,6 +12,9 @@ use std::ffi::CString; use protover::*; use smartlist::*; use tor_allocate::allocate_and_copy_string; +use tor_util::strings::byte_slice_is_c_like; +use tor_util::strings::empty_static_cstr; + /// Translate C enums to Rust Proto enums, using the integer value of the C /// enum to map to its associated Rust enum @@ -144,8 +147,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { // bytes. An assert is okay here, since changing the const byte slice // in protover.rs to contain a NUL byte somewhere in the middle would be a // programming error. - assert!(!SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1].contains(&0x00)); - assert!(SUPPORTED_PROTOCOLS[SUPPORTED_PROTOCOLS.len() - 1] == 0x00); + assert!(byte_slice_is_c_like(SUPPORTED_PROTOCOLS)); // It's okay to call the "unchecked" version of the function because // we can see that the bytes we're passing into it 1) are valid UTF-8, @@ -200,15 +202,15 @@ pub extern "C" fn protover_is_supported_here( /// Provide an interface for C to translate arguments and return types for /// protover::compute_for_old_tor #[no_mangle] -pub extern "C" fn protover_compute_for_old_tor( - version: *const c_char, -) -> *mut c_char { - // Not handling errors when unwrapping as the content is controlled - // and is an empty string - let empty = String::new(); +pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const c_char { + let supported: &'static CStr; + let elder_protocols: &'static [u8]; + let empty: &'static CStr; + + empty = empty_static_cstr(); if version.is_null() { - return allocate_and_copy_string(&empty); + return empty.as_ptr(); } // Require an unsafe block to read the version from a C string. The pointer @@ -217,10 +219,24 @@ pub extern "C" fn protover_compute_for_old_tor( let version = match c_str.to_str() { Ok(n) => n, - Err(_) => return allocate_and_copy_string(&empty), + Err(_) => return empty.as_ptr(), }; - let supported = compute_for_old_tor(&version); + elder_protocols = compute_for_old_tor(&version); + + // If we're going to pass it to C, there cannot be any intermediate NUL + // bytes. An assert is okay here, since changing the const byte slice + // in protover.rs to contain a NUL byte somewhere in the middle would be a + // programming error. + assert!(byte_slice_is_c_like(elder_protocols)); + + // It's okay to call the "unchecked" version of the function because + // we can see that the bytes we're passing into it 1) are valid UTF-8, + // 2) have no intermediate NUL bytes, and 3) are terminated with a NUL + // byte. + unsafe { + supported = CStr::from_bytes_with_nul_unchecked(elder_protocols); + } - allocate_and_copy_string(&supported) + supported.as_ptr() } diff --git a/src/rust/protover/lib.rs b/src/rust/protover/lib.rs index 5a5dea440..fe8c0f9bb 100644 --- a/src/rust/protover/lib.rs +++ b/src/rust/protover/lib.rs @@ -26,6 +26,7 @@ extern crate libc; extern crate smartlist; extern crate external; extern crate tor_allocate; +extern crate tor_util; mod protover; pub mod ffi; diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 00fe08408..1680d3394 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -10,6 +10,7 @@ use std::collections::{HashMap, HashSet}; use std::ops::Range; use std::string::String; +use tor_util::strings::NUL_BYTE; /// The first version of Tor that included "proto" entries in its descriptors. /// Authorities should use this to decide whether to guess proto lines. @@ -724,11 +725,11 @@ pub fn is_supported_here(proto: Proto, vers: u32) -> bool { /// /// # Inputs /// -/// * `version`, a string comprised of "[0-9,-]" +/// * `version`, a string comprised of "[0-9a-z.-]" /// /// # Returns /// -/// A `String` whose value is series of pairs, comprising of the protocol name +/// A `&'static [u8]` whose value is series of pairs, comprising of the protocol name /// and versions that it supports. The string takes the following format: /// /// "HSDir=1-1 LinkAuth=1" @@ -737,33 +738,27 @@ pub fn is_supported_here(proto: Proto, vers: u32) -> bool { /// only for tor versions older than FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS. /// /// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` -pub fn compute_for_old_tor(version: &str) -> String { - if c_tor_version_as_new_as( - version, - FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS, - ) - { - return String::new(); +pub fn compute_for_old_tor(version: &str) -> &'static [u8] { + if c_tor_version_as_new_as(version, FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS) { + return NUL_BYTE; } if c_tor_version_as_new_as(version, "0.2.9.1-alpha") { - let ret = "Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 \ - Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2"; - return String::from(ret); + return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 \ + Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } if c_tor_version_as_new_as(version, "0.2.7.5") { - let ret = "Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ - Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2"; - return String::from(ret); + return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ + Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } if c_tor_version_as_new_as(version, "0.2.4.19") { - let ret = "Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ - Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2"; - return String::from(ret); + return b"Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ + Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2\0"; } - String::new() + + NUL_BYTE } #[cfg(test)] diff --git a/src/rust/tor_util/lib.rs b/src/rust/tor_util/lib.rs index 42fa9d5ad..12cb3896b 100644 --- a/src/rust/tor_util/lib.rs +++ b/src/rust/tor_util/lib.rs @@ -3,9 +3,9 @@ //! Small module to announce Rust support during startup for demonstration //! purposes. -//! extern crate libc; extern crate tor_allocate; pub mod ffi; +pub mod strings; diff --git a/src/rust/tor_util/strings.rs b/src/rust/tor_util/strings.rs new file mode 100644 index 000000000..9321ce4f8 --- /dev/null +++ b/src/rust/tor_util/strings.rs @@ -0,0 +1,82 @@ +// Copyright (c) 2016-2017, The Tor Project, Inc. */ +// See LICENSE for licensing information */ + +//! Utilities for working with static strings. + +use std::ffi::CStr; + +/// A byte-array containing a single NUL byte (`b"\0"`). +pub const NUL_BYTE: &'static [u8] = b"\0"; + +/// Determine if a byte slice is a C-like string. +/// +/// These checks guarantee that: +/// +/// 1. there are no intermediate NUL bytes +/// 2. the last byte *is* a NUL byte +/// +/// # Warning +/// +/// This function does _not_ guarantee that the bytes represent any valid +/// encoding such as ASCII or UTF-8. +/// +/// # Examples +/// +/// ``` +/// # use tor_util::strings::byte_slice_is_c_like; +/// # +/// let bytes: &[u8] = b"foo bar baz"; +/// +/// assert!(byte_slice_is_c_like(&bytes) == false); +/// +/// let bytes: &[u8] = b"foo\0bar baz"; +/// +/// assert!(byte_slice_is_c_like(&bytes) == false); +/// +/// let bytes: &[u8] = b"foo bar baz\0"; +/// +/// assert!(byte_slice_is_c_like(&bytes) == true); +/// ``` +pub fn byte_slice_is_c_like(bytes: &[u8]) -> bool { + if !bytes[..bytes.len() - 1].contains(&0x00) && bytes[bytes.len() - 1] == 0x00 { + return true; + } + false +} + +/// Get a static `CStr` containing a single `NUL_BYTE`. +/// +/// # Examples +/// +/// When used as follows in a Rust FFI function, which could be called +/// from C: +/// +/// ``` +/// # extern crate libc; +/// # extern crate tor_util; +/// # +/// # use tor_util::strings::empty_static_cstr; +/// use libc::c_char; +/// use std::ffi::CStr; +/// +/// pub extern "C" fn give_c_code_an_empty_static_string() -> *const c_char { +/// let empty: &'static CStr = empty_static_cstr(); +/// +/// empty.as_ptr() +/// } +/// +/// # fn main() { +/// # give_c_code_an_empty_static_string(); +/// # } +/// ``` +/// +/// This equates to an "empty" `const char*` static string in C. +pub fn empty_static_cstr() -> &'static CStr { + let empty: &'static CStr; + + unsafe { + empty = CStr::from_bytes_with_nul_unchecked(NUL_BYTE); + } + + empty +}

1 0

[tor/master] Remove new unsafe {} use.
by nickm＠torproject.org 08 Feb '18

08 Feb '18

commit d8307cb0e99d28daa4011e4e9d94e3f8c56cba23 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 8 17:26:26 2018 -0500 Remove new unsafe {} use. Rationale: this helps for performance only, but we don't actually have any reason to think that the checks here are performance-critical. Let's not normalize the use of unsafe {}. --- src/rust/protover/ffi.rs | 12 ++++-------- src/rust/protover/protover.rs | 9 +++++---- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 5519b75ea..d724c102d 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -149,13 +149,11 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { // programming error. assert!(byte_slice_is_c_like(SUPPORTED_PROTOCOLS)); - // It's okay to call the "unchecked" version of the function because + // It's okay to unwrap the result of this function because // we can see that the bytes we're passing into it 1) are valid UTF-8, // 2) have no intermediate NUL bytes, and 3) are terminated with a NUL // byte. - unsafe { - supported = CStr::from_bytes_with_nul_unchecked(SUPPORTED_PROTOCOLS); - } + supported = CStr::from_bytes_with_nul(SUPPORTED_PROTOCOLS).unwrap(); supported.as_ptr() } @@ -230,13 +228,11 @@ pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const // programming error. assert!(byte_slice_is_c_like(elder_protocols)); - // It's okay to call the "unchecked" version of the function because + // It's okay to unwrap the result of this function because // we can see that the bytes we're passing into it 1) are valid UTF-8, // 2) have no intermediate NUL bytes, and 3) are terminated with a NUL // byte. - unsafe { - supported = CStr::from_bytes_with_nul_unchecked(elder_protocols); - } + supported = CStr::from_bytes_with_nul(elder_protocols).unwrap(); supported.as_ptr() } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 1680d3394..f3a5ea23e 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -106,10 +106,11 @@ impl FromStr for Proto { /// "HSDir=1-1 LinkAuth=1" /// pub fn get_supported_protocols() -> &'static str { - unsafe { - // The `len() - 1` is to remove the NUL byte. - str::from_utf8_unchecked(&SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1]) - } + // The `len() - 1` is to remove the NUL byte. + // The `unwrap` is safe becauase we SUPPORTED_PROTOCOLS is under + // our control. + str::from_utf8(&SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1]) + .unwrap() } /// Translates a vector representation of a protocol list into a HashMap

1 0