Discussion:
[tor-dev] v3 hidden services: inconsistencies between spec and implementation
inkylatenoth
2017-10-28 09:28:30 UTC
Permalink
Whilst implementing v3 hidden services myself I found some
inconsistencies between the specs and the current implementation. I
wanted to share these in case someone from the Tor organization wants to
update the specs and/or the implementation.

# rend-spec-v3.txt

## 2.4

* after decrypting the `superencrypted' object from a descriptor, the
resulting document does not end with the NL character. This means that
it does not strictly conform to the document meta-format described in
section 1.2 of dir-spec.txt.

## A.2

* the blinded key param is defined as H(BLIND_STRING | A | s | B | N).
In practice I found that I had to add a null byte after BLIND_STRING
in order to reach the same value as the C implementation:

param = H(BLIND_STRING | INT_1(\x00) | A | s | B | N)

In all other cases where a string constant is used like this (e.g.
computing the nonce N above), I found that the trailing null byte is
not required.

* when clamping the blinding factor, the second bitwise operation is
`param[31] &= 127' in the spec but `param[31] &= 63' in the C
implementation. These are equivalent in practice when followed by the
third operation (`param[31] |= 64'), but it might be nice to use a
consistent representation for the benefit of human readers.

# 220-ecc-ids-keys.txt

# 2.1

* 'The signature is formed by signing the first N-64 bytes of the
certificate prefixed with the string "Tor node signing key certificate
v1".' I found this to be false; the signatures only validate without
the string prefix.

## A.1

* I realized that the certificate types here are outdated. The
signing-key extension is listed as type [04], when in rend-spec-v3.txt
and the C implementation it is type [08].
George Kadianakis
2017-10-28 14:30:51 UTC
Permalink
Post by inkylatenoth
Whilst implementing v3 hidden services myself I found some
inconsistencies between the specs and the current implementation. I
wanted to share these in case someone from the Tor organization wants to
update the specs and/or the implementation.
Hello inkylatenoth!

That's a great post and thanks for catching all these issues and
innacuracies! We are definitely interested in consistency and fixing the
spec (and implementation if needed).
Post by inkylatenoth
# rend-spec-v3.txt
## 2.4
* after decrypting the `superencrypted' object from a descriptor, the
resulting document does not end with the NL character. This means that
it does not strictly conform to the document meta-format described in
section 1.2 of dir-spec.txt.
Hmm... This might be worth fixing on the implementation if possible (and
if it won't break things). Otherwise, let's patch the spec.
Post by inkylatenoth
## A.2
* the blinded key param is defined as H(BLIND_STRING | A | s | B | N).
In practice I found that I had to add a null byte after BLIND_STRING
param = H(BLIND_STRING | INT_1(\x00) | A | s | B | N)
In all other cases where a string constant is used like this (e.g.
computing the nonce N above), I found that the trailing null byte is
not required.
Ouch. This might be an artifact of the way strings are implemented in C.

I guess a spec patch might be the right thing to do here, otherwise too
much stuff will break.
Post by inkylatenoth
* when clamping the blinding factor, the second bitwise operation is
`param[31] &= 127' in the spec but `param[31] &= 63' in the C
implementation. These are equivalent in practice when followed by the
third operation (`param[31] |= 64'), but it might be nice to use a
consistent representation for the benefit of human readers.
Hmm... Yeah there are various ways to do the clamping for ed25519 keys.

I think we should edit the spec to reflect the clamping we do on the code.
Post by inkylatenoth
# 220-ecc-ids-keys.txt
# 2.1
* 'The signature is formed by signing the first N-64 bytes of the
certificate prefixed with the string "Tor node signing key certificate
v1".' I found this to be false; the signatures only validate without
the string prefix.
Ouch... I think we should edit the spec and consider if there are any
security risks here.
Post by inkylatenoth
## A.1
* I realized that the certificate types here are outdated. The
signing-key extension is listed as type [04], when in rend-spec-v3.txt
and the C implementation it is type [08].
Let's fix the spec here too...

---

Inkylatenoth, let me know if you are interested in drafting a spec/code
patch for the issues you found!!! If you are not interested, I can try
to do them myself at some point in the next weeks (been pretty busy with
stuff lately).

Also, let us know if your independent implementation is a public thing
we should know about. Seems interesting :)

Thanks again!
teor
2017-10-28 21:27:49 UTC
Permalink
Post by George Kadianakis
Post by inkylatenoth
# 220-ecc-ids-keys.txt
Is this the latest version of the ECC ID specification?
Usually, our proposals are integrated into the main spec documents
after they are implemented.
Post by George Kadianakis
Post by inkylatenoth
# 2.1
* 'The signature is formed by signing the first N-64 bytes of the
certificate prefixed with the string "Tor node signing key certificate
v1".' I found this to be false; the signatures only validate without
the string prefix.
Ouch... I think we should edit the spec and consider if there are any
security risks here.
One security risk is that signatures on these certificates are re-usable
in other contexts. For example, if two different parts of the Tor code
believe signed certificates without prefixes, an adversary can take a
certificate signed for one of them, and pass it to the other.
Post by George Kadianakis
Post by inkylatenoth
## A.1
* I realized that the certificate types here are outdated. The
signing-key extension is listed as type [04], when in rend-spec-v3.txt
and the C implementation it is type [08].
Let's fix the spec here too...
This should definitely be integrated into one of the main specs.

T
inkylatenoth
2017-11-03 10:00:44 UTC
Permalink
Post by George Kadianakis
That's a great post and thanks for catching all these issues and
innacuracies! We are definitely interested in consistency and fixing the
spec (and implementation if needed).
No problem! I'm glad you found my post helpful.
Post by George Kadianakis
Post by inkylatenoth
# rend-spec-v3.txt
## 2.4
* after decrypting the `superencrypted' object from a descriptor, the
resulting document does not end with the NL character. This means that
it does not strictly conform to the document meta-format described in
section 1.2 of dir-spec.txt.
Hmm... This might be worth fixing on the implementation if possible (and
if it won't break things). Otherwise, let's patch the spec.
I think it should be possible to fix this in the implementation without
breaking anything.
Post by George Kadianakis
Post by inkylatenoth
# 220-ecc-ids-keys.txt
# 2.1
* 'The signature is formed by signing the first N-64 bytes of the
certificate prefixed with the string "Tor node signing key certificate
v1".' I found this to be false; the signatures only validate without
the string prefix.
Ouch... I think we should edit the spec and consider if there are any
security risks here.
I agree with all of your proposed solutions (spec vs implementation)
except for here, where I would be much more comfortable with an
implementation change. I realize it would be a breaking change, however,
and will understand if you decide to update the spec instead.
Post by George Kadianakis
Inkylatenoth, let me know if you are interested in drafting a spec/code
patch for the issues you found!!! If you are not interested, I can try
to do them myself at some point in the next weeks (been pretty busy with
stuff lately).
At the moment I don't have the time to submit patches, sorry. If you
find the time yourself then I'd be pleased to review/test your patches
to confirm that they solve the inconsistencies I found.
Post by George Kadianakis
Also, let us know if your independent implementation is a public thing
we should know about. Seems interesting :)
I'm not implementing the full protocol, but I certainly will do when
it's finished and open-sourced :)

Loading...