Doing this means that any attempt to use a read-only store would
crash as soon as it found that the consensus was usable.
It seems that this bug was introduced at some point doing all the
dirmgr refactors we did over the past year. Perhaps there should be
a test for running with a read-only store.
Fixes#779
Previously, if somebody wrote this code, an attacker could easily
use it to cause an OOM panic:
```
let n = r.take_u64();
let items: Vec<Foo> = r.extract_n(n as usize)?;
```
The first line of defense here is not to write protocols like that:
we don't actually _have_ any 32-bit counters in our protocol
AFAICT.
The second line of defense is to pre-check `n` for reasonableness
before calling `extract_n`.
Here we add a third line of defense: whereas previously we would do
`Vec::with_capacity(n)` in `extract_n`, we now allocate an initial
capacity of `min(n, r.remaining())`. This ensures that the size of
the allocation can't exceed the remaining length of the message,
which (for our cell types at least) should prevent it from
overflowing or running OOM.
These Arcs are all "downward", referencing items from layers lower in
the stack. So they don't cause cycles.
There was going to be a cycle involving the `OnionConnector` upcall
trait, but we have just abolished that.
Abolish CircMgr::get_or_launch_onion_client and everything to support
it. We have decided that `.onion` diversion ccan't/shouldn't occur in
tor-circmgr. Probably, it should occur much higher up - arti-client
maybe - since it will sometimes need ambient authority (KS_hsc_*).
Now all knowledge of HS connections is in tor-hsclient. This
gets rid of a layering inversion and the trait needed for tor-circmgr
to do the upcall to tor-hsclient.
This is getting rather confusing; simply reformtting it won't do, I
think. Also there would be much rightward drift.
So move the meat out into the new function.
(And introduce a convenience alias for its captures.)
Docs and reformatting will follow in a moment.
Explicit drops don't work. Instead, introduce a scope.
We need two scopes, actually: one where we do the initial table
wrangling, and one for the retries after relock.
So we must put the meat in a closure so we can reuse it.
And we must return the flow control as an enum. Bah, etc.
Avoid reformatting this for the moment. This makes the delta legible...
This is still correct from a lock hierarchy pov. It moves the guard
relock to the end, which is going to be necessary since it is going to
have to move right outside the loop.
Provide a more cookied "secret keys for use to connecting to a
particular HS" type, with a builder.
This wants to use config stuff, so oughtn't to be in tor-*crypto.
The individual types remain there.
*If* we're going to retain any HS knowledge in circmgr, it definitely
doesn't need to know about per-operation client secrets.
(Maybe there might be ambient secrets, used for .onion diversion, but
they don't need to be in this API.)
Otherwise there has to be a state entry in the circmgr *and* a state
entry in the hs connector, for every HS. This division of
responsibilit will be confusing.
The HS code will then be more completely just a layer on top of circmgr.
This makes our implementation behave the same as the C tor
implementation, by validating all of the expiration and signatures
on the certificates in the inner document.
(It is still not semantically necessary to check these certs: the
document in which they appear is already signed by the key with
which they are allegedly signed.)
Closes#744
The old code parsed and encoded a signature and a mac... but there
was no way to actually set them properly. Now EstablishIntro is
built around an EstablishIntroBody, and has the ability to check
signatures and macs.
Because there is no way to handle one of these messages if we can't
check the signature, we no longer accept unrecognized `auth_key` types
in this message.
I've added a test to make sure that we can validate a message from the
C tor implementation, and a test to make sure we can validate our
own cells. I also had to modify the previous tests so that their
keys were well-formed.
These crates didn't have any changes until now, when I bumped
the versions of some other crates they depend on:
tor-consdiff
arti-hyper
arti-bench
arti-testing
These crates have had small code changes, but no API additions:
tor-config
tor-socksproto
tor-cert
tor-chanmgr
tor-ptmgr
tor-guardmgr
tor-circmgr
tor-dirclient
tor-dirmgr
arti
tor-hsservice
tor-congestion
These crates have had API extensions:
fs-mistrust
tor-llcrypto
tor-bytes
tor-checkable
tor-linkspec
tor-netdoc
tor-persist
arti-client
These are API breaks, but the crates themselves are currently
100% experimental, so there's no need to bump the minor versions
according to our semver rules.
The four values of times taken in a particular test were changed to both
be human readable and have comments explaining their significance (they
are all important moments after the Unix Epoch for freedom)
Unlike linkspec, this doesn't validate the actual contents of the
specifiers. We'll use this so we can handle the linkspec list for an
introduction point in an HsDesc, and just pass it on when
constructing our circuits.
I haven't added any accessor or constructor functions, because I
don't expect to need them.
It said to check whether C enforces an absence of extraneous bytes
at the end of the link specifiers. It does, in
`hs_desc.c:decode_link_specifiers()`, where it says:
```
if (link_specifier_list_parse(&specs, decoded,
(size_t) decoded_len) < decoded_len) {
goto err;
}
```
The comparison with "decoded_len" checks whether all the bytes were
decoded.
Some code in our tests that worked fine with time 0.3.17 no
longer works with 0.3.19, despite the semver.
See https://github.com/time-rs/time/issues/552 for the upstream bug.
This change makes sure that open streams and half-closed streams
have the same stream-type-dependent state machines with respect to
which cells are acceptable.
Fixes#774.
Fixes#769.
The role of CmdChecker is to verify that messages are arriving at
the appropriate sequence on a stream, with respect to the other
messages that have been received. Once the stream becomes
half-closed, the CmdChecker is also in charge of consuming incoming
messages on the stream and making sure that they are well-formed.
There's a blanket impl of Itertor for &mut impl Iterator, so this
isn't necessary, and it prevents us passing iterators by value
producing syntactic vinegar.
This includes a partial solution for #769, but also turned up
another bug (#774) while I was working on it. I'll close them both
once I have a real solution.
We'll use this to router relay messages on a circuit to the
appropriate stream, and hand them to that stream, without parsing
the message until the stream has been determined.
We now manipulate raw relay cell bodies as (an alias for)
`Box<[u8;509]>` rather than as (an alias for) `[u8;509]`. This
enables us to do much less copying. It will become more important
soon, as we defer parsing relay cell bodies even longer.
Related to #7.
We also use SliceWriter to avoid allocating a Vec<> for every relay
message we want to encode, and instead encode directly into the
cell.
Because the API assumes that many writes are infallible, this writer
takes ownership of the backing object, and will only return it to
you if you didn't run over the end.
I'm going to use this to save some allocations in relay cell bodies
Several HS message types have an extension list type. They all use
the same framing for extensions, but each of them has separate
extension types and separate extension namespaces.
This commit simplifies establish_intro a little, and adds support
for maintaining unrecognized extension types--at the expense of some
new internal code.
Some of the HS message types have a lot of dependent types, like
extensions and options for those extensions, and so on. Except when
those extensions are portable across cell types, it makes sense
to put them in their own modules.
We want to fuzz these parsers, but there's no currently way to get
at the parsers for inner documents without going through a lot of
encryption. (Coverage-guided fuzzers are powerful, but they
can't find SHA3 preimages.)
There will be two call sites to demonstrate it.
Eventually maybe this will want to be in tor-basic-utils, since it
doesn't depend on any of the tor-netdoc types. But it would be
sensible to wait until the situation with PeekableIterator and
Itertools is improved.
For now we make this #[doc(hidden)] to avoid it becoming part of our
stable API.
This one was generated (by dgoulet) using `ClientDescEncKey`
encryption. Its information is:
```
Address: paozpdhgz2okvc6kgbxvh2bnfsmt4xergrtcl4obkhopyvwxkpjzvoad.onion
Time period: 19397
Client:
paozpdhgz2okvc6kgbxvh2bnfsmt4xergrtcl4obkhopyvwxkpjzvoad:descriptor:x25519:SDZNMD4RP4SCH4EYTTUZPFRZINNFWAOPPKZ6BINZAC7LREV24RBQ
Service:
descriptor:x25519:SACGOAEODFGCYY22NYZV45ZESFPFLDGLMBWFACKEO34XGHASSAMQ
```
Fixes an instance of #768.
The problem with the test vectors is that I used a random time
period number (1234) and the default-in-tor period length (1440)
without checking whether 1440 _meant_ minutes or seconds. I'll add
another test to Tor to make sure that the time period matches now.
With this change, I can test Tor-generated hsdescs with encryption,
so I'm fairly confident that the new behavior is correct.
Unlike C tor, we treat unrecognized commands as reason to kill off
the connection entirely. That's fine; if we need to add an
unrecognized command in the future, we can use VERSIONS to negotiate
it.
Also, if someday we want this code to support relay channels as
well, we can use some type trickery to have that work too.
Actually, to avoid making a breaking change, I'm deprecating
BadMessage and creating a new InvalidMessage variant that takes a
Cow. This way I don't need to track every crate that re-exposes
tor_bytes::Error and call this a breaking change in those.
Making this change will allow tor_bytes errors to be much more
helpful.
Every FooMsg type now implements Into<AnyFooMsg>, and
TryFrom<FooMsg>.
Additionally, it now implements From<X> for every distinct type that
it supports. This last part lets us discard a bunch of code.
Unfortunately, I needed some downright hackish trickery in order to
get these macros to avoid generating `From<AnyFooMsg> for AnyFooMsg`
and conflicting with the blanket implementation.
The trickery to deal with RelayEarly and Relay being the same type
was not necessarily worth it; I will be separating them and removing
said trickery in the next commit.
This change lets us use ChannelCodec to encode and decode any
restricted channel message type we want. (Later on, we'll turn the
related Codec class in tor-proto into a more type-restricted version
of this.)
* Provide an accessor for the HSDIR flag
* Provide a function for testing a relay for hsdir inclusion
* Provide an iterator on NetDir that returns the hsdirs
* Implement Netdir::compute_rings in terms of a new
HsDirRing::compute, that currently does nothing.
* Actually call Netdir::compute_rings (since now it doesn't panic).
* Make Netdir::compute_rings not be pub. We do this unconditionally,
rather than exposing the distinction between a netdir-without-hsdir
and a netdir-with-hsdir.
The file which contains this type is called hsdir_params.rs. We have
a general problem with slight confusion about when to includen "dir"
and when to include "ring".
Resolve this in favour of the rule now added to the module-level doc
comment.
These variables are going to be struct fields, which will sort of
enforce consistent naming. The struct fields are going to appear in a
moment. We'll call the fields "current" and "secondary" after the
naming in the test cases.
And import hsdir_params::HsRingParams, which we're going to make more
references to.
* Remove the return value, which was not used anywhere.
Also remove the code to calculate the return value.
* Take an Arc<NetDir> rather than a reference. We are going to want
this for HS support. This has no overall effect on the lifetime of
the4 Arc, which was owned at the one call site and then imediately
dropped.
* Change the documentation to explain what the function's role is in
the netdir API, rather than the fiddly details of what it actually
does internally. Relegate the latter to a code comment.
(When we have HS, this will do more, or, at least, make further
arrangements.)
Locally, the only functional effect is that now we refuse to handle
non-whole-number-of-minutes lengths - but since the consensus
parameter can't represent those, there's no overall functional change.
We need to make sure any `#[cfg(feature=...)]` attributes are
applied not only to our variant declarations, but also to the
branches in the match statements that deal with them.
Doing this will make it much easier to implement a macro that
generates restricted instances of the Msg types (for #525).
The Body change is a breaking change. I don't think anybody else
implements Body, but in theory they could.
These are generalizations of RelayCell and ChanCell respectively,
that allow using an arbitrary message type in place of the fully
general RelayMsg and ChanMsg types. Doing this is a prerequisite
for usefully implementing arti#525.
These functions consume a checkable wrapper, and return a new
checkable wrapper with mapped contents but the same not-yet-checked
constraints.
As documented, They are "dangerous" because the provided function
gets access to the contents before they are checked; the caller has
to make sure that the provided function doesn't expose their
contents inappropriately.
There are some places where I note certificates which are not
currently validated, because there is no cryptographic point in
doing so. We should either document that this is okay, or validate
the certificates anyway.
This code might benefit from refactoring to make it prettier.
It turns out that C Tor doesn't add a newline at the end of the
middle layer of an onion service descriptor. I've made a spec MR
(torspec!109) to document this: here, it's time to work around the
issue.
I generated this using C tor (latest main) and a Chutney network
about a week ago.
The subcredential is:
78210A0D2C72BB7A0CAF606BCD938B9A3696894FDDDBC3B87D424753A7E3DF37
The HS_blind_id is:
43CC0D62FC6252F578705CA645A46109E265290343B1137E90189744B20B3F2D
Now we require that, for all `SectionRules`, either the caller say
how to handle unrecognized tokens (using `.add(UNRECOGNIZED...)`),
or that they explicitly reject unrecognized tokens (using
`reject_unrecognized`()`.)
This solution uses an assert!() rather than an Error to indicate
failure. I say that's fine, since
1. This is a crate-internal API.
2. We never dynamically construct SectionRules according to
different behavior: they are always prefabricated in a fixed
code block. Thus, if we test a parser at all, we will make
sure that its SectionRules are well-formed.
I considered and explicitly rejected a solution where the builder
had to be finalized with separate methods `build_strict()` or
`build_tolerant()`: It's too easy IMO for the caller to forget what
these call means.
Prevents further recurrences of #752.
Closes#752.
This fixes an instance of bug#752. Previously, we would reject any
AuthCert that contained an unexpected keyword. (Fortunately, this
data format does not change very often.)
In !948 we renamed a couple of accessor functions, which is a
breaking change in `tor-cell`'s API.
In retrospect, perhaps we should have deprecated the old names and
added the new ones, so we wouldn't have to break the API. (This is
the only API break AFAICT since 1.1.0.)
These changes influence behavior, but not effect compatibility.
(If I messed up, and any crate except for `arti` has non-breaking
API changes, that's still fine, since they are all version
0.x.)
Requring `Display` is wrong here, because if this is actually an
Error, Display would be wrong because it doesn't display causes.
As it happens, the `error` parameter is only ever `&str`.
This logic is a bit tricky, so I've tried to document it and add
fairly good tests. The silver lining is that the external API for
all of this logic will make it invisible and hidden.
There are some cases where I added functions that I think might
eventually get lowered into MdConsensus: But I don't want to lower
too much right now, since the convention for our netdoc accessors is
that they are fairly unsophisticated, and they show you the document
as it is.
Closes#686
These are deliberately unsophisticated accessors, in that they return Option<>
rather than filling in missing values with the documented
fallbacks. It seems better to leave a way to distinguish the absent
case in the API.
Change ErrorHint so that, internally, it just holds an enum with a
lightweight reference to whatever parts of the error it needs to
generate a hint. Then we can move the formatting logic into a
Display function for ErrorHint, and do away with ErrorDetail entirely.
Move the "hint" function into Error, and use Option rather than Result.
(I'm using Option here because it's not really an error case not to have
a hint; we just either have a hint, or we don't.)
Clippy now complains about `let _ = (expr_producing_a_future);`,
which is probably smart, since maybe you wanted to await that future
and ignore the result. So it seems that the right way to get rid of
an unwanted Receiver is now to drop it explicitly.
Closes#749
This warning kind of snuck up on us! (See #748) For now, let's
disable it. (I've cleaned it up in a couple of examples, since
those are meant to be more idiomatic and user-facing.)
Closes#748.
This required rewriting some of our error handling code in
command-line processing, since the toml crate now displays and
reports errors differently. (Admittedly, this code still is kind of
ugly, but at least it is nicely hidden.)
It turns out that the credential is only calculated as an
intermediate result in order to blind keys and produce the
subcredential. As such, it has no need to leave the hscrypto module.
This commit changes the shebang in all shell scripts from absolute
paths (such as `/bin/bash` or `/usr/bin/python3`) to the `/usr/bin/env`
binary with the accompanying interpreter as it's argument.
The reason for this are as follows:
- NixOS cannot work with absolute paths
- BSD systems install their packages in /usr/local/bin
Previously, the offset was set to 12 hours unconditionally (like the
spec says). But based on a conversation on tor-dev, it seems that
the offset should actually be 12 times the voting interval.
I'm also opening an MR to change the spec.
Rename from _keyword_, since this actually checks the *multiple*
keywords that can appear in an object's BEGIN line.
Make this clear in the doc comment.
Expose it for use by the netdoc builder.
This is in lieu of upgrading to the latest base64 crate, which has
a different API from the old one. Since we have to migrate either
way, we might as well use base64ct everywhere.
I don't think that most of these cases _require_ constant-time
base64, but it won't hurt.
This is a little tricky, but I think that we're not actually
exposing too much here. I expect we'll need to tweak this stuff
between now and our final version.
These are available unconditionally, since they represent
comparatively little storage and processing effort.
See param-spec.txt section 8 for the original information here.
* It is the NetDir's responsibility to tell the caller what the time
period is.
* There can be up to two secondary time periods.
* Each time period has a single SRV.
* Secondary time periods only apply for onion services, when they
publish.
* When publishing, the correct input is a time period.
These are a bit complex internally, but the API they present is
pretty simple. I've left some discussion of points where the design
isn't totally fleshed out, and where we need to look harder at the
spec.
Part of #716.
SharedRandVal now holds only the 32-byte random value itself; the
"number of commits" field is in SharedRandStatus.
This commit also makes the SharedRandVal be exactly 32 bytes, since
we've set it to that value in the spec.
This module has types and operations needed in multiple places
for an onion service implementation. There are a bunch of
TODO hs-crypto comments that we'll need to fill in.
The `Ed25519Identity` and `RsaIdentity` types are not precisely
always used as relay identifiers: they are more generally used as
_key_ identifiers.
This will become relevant as `RsaIdentity` is used for authority
keys (as in authorities' VoterInfo blocks), and as `Ed25519Identity`
is used as the identifier behind an onion service key.
This is based on logs that I added locally while I was trying to
debug some startup issues. Hopefully they'll make things easier the
next time there's something to debug.
Part of #677.
Closes#719.
Due to a difference between ed25519-dalek and ed25519-donna,
converting these secret keys directly to public keys does not work.
I've documented this in a "Limitations" section.
This type provides a common implementation for types that are
implemented as arrays of bytes that should only be compared
with constant-time comparisons.