Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13111 closed defect (implemented)

Tor fails to start if onion keys are zero length

Reported by: ioerror Owned by: teor
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I'm using a very unstable embedded device to run a transparent Tor router/relay/hidden service. In some cases - the device will reset the clock to 1970 (hooray, no RTC!) or the clock will be otherwise incorrect (hooray, low battery on the RTC!). Furthermore, the device will sometimes be unplugged at random (solar panel dies, janitor unplugs it, etc) and this results in a state/keys directory that looks like as follows:

Sep 10 13:31:48.000 [notice] Tor 0.2.4.23 (git-417f0cacd726e549) opening log file.
Sep 10 13:31:48.000 [notice] Not disabling debugger attaching for unprivileged users.
Sep 10 13:31:48.000 [notice] Parsing GEOIP IPv4 file /usr/share/tor/geoip.
Sep 10 13:31:48.000 [notice] Parsing GEOIP IPv6 file /usr/share/tor/geoip6.
Sep 10 13:31:49.000 [notice] Configured to measure statistics. Look for the *-stats files that will first be written to the data directory in 24 hours from now.
Sep 10 13:31:49.000 [warn] crypto error while Error parsing private key: no start line (in PEM routines:PEM_read_bio)
Sep 10 13:31:49.000 [err] Error loading private key.
Sep 10 13:31:49.000 [err] do_main_loop(): Bug: Error initializing keys; exiting
root@debian:/var/lib/tor/keys# ls -al
total 20
drwx--S--- 2 debian-tor debian-tor 4096 Aug 15 14:56 .
drwx--S--- 5 debian-tor debian-tor 4096 Sep 10 13:31 ..
-rw------- 1 debian-tor debian-tor  887 Sep 10 13:30 secret_id_key
-rw------- 1 debian-tor debian-tor    0 Sep 10 13:30 secret_onion_key
-rw------- 1 debian-tor debian-tor  891 Sep 10 13:30 secret_onion_key.old
-rw------- 1 debian-tor debian-tor    0 Sep 10 13:30 secret_onion_key_ntor
-rw------- 1 debian-tor debian-tor   96 Sep 10 13:30 secret_onion_key_ntor.old

If I remove the zero byte files and restart Tor, all is well:

root@debian:/var/lib/tor/keys# rm secret_onion_key*
/etc/init.d/tor restart

I think that Tor should notice that the files are zero bytes in length and gracefully generate the keys.

Child Tickets

Attachments (1)

zero_length_keys.sh (2.4 KB) - added by teor 3 years ago.
Test #13111 behaviour, use "-d" for deleted files, default is zero-length files

Download all attachments as: .zip

Change History (26)

comment:1 Changed 3 years ago by nickm

Keywords: tor-relay lorax added
Milestone: Tor: 0.2.6.x-final

comment:2 Changed 3 years ago by teor

ioerror, I'm looking into this.

Is this a general thing where we should treat any zero-length file as if it doesn't exist?
Or is it just an issue for the keys?

So far, my (partial) analysis is:

  • empty keys - overwrite
  • empty torrc - ok (use defaults)
  • empty caches - overwrite (if we don't already)

comment:3 Changed 3 years ago by ioerror

I think we should treat a zero length file as an error and re-generate and overwrite them, yes.

comment:4 Changed 3 years ago by teor

Keywords: easy needs-tests added
Owner: set to teor
Status: newassigned

Ok, I'll check all the cases where we stat a file path, and categorise them into:

  • overwrite empty
  • empty ok/valid
  • fail on empty (if needed)

Then I guess we'd better unit test this somehow :-)

comment:5 Changed 3 years ago by teor

Keywords: needs-tests removed
Status: assignedneeds_review

I've created a git branch that fixes this issue:
Branch: bug13111-empty-key-files
Repository: ​​​https://github.com/teor2345/tor.git

New Function:

  • int64_t file_size(const char *fname) - like file_status().
    • Returns the size of a (regular) file, or data waiting to be read on a FIFO, in bytes. Ignores the sizes returned for any other type of file and returns -1.
    • Should this be uint64_t?
    • I'm using -1 to signal error, but using 0 is also a possibility, requiring the user to check errno and/or file_status() as well.

Bug Fixes:

  • empty RSA & curve25519 key files - overwrite empty key files rather than failing to start tor

Improved Performance: (slightly?)

  • empty stats file while reading in extrainfo for router descriptor - skip reading file
  • empty router / extra info store files - skip reload
  • empty state file - skip load

Unit Tests:
After running make check, make test, benchmarks, and chutney --flavour bridges+ipv6, the file_size() function has been run 48 times. I'm ok with that level of coverage.

Testing:

Check desired behaviour: zero-length key file -> regenerate

src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
^C # when the keys have been generated
ls /tmp/tor/keys/
rm /tmp/tor/keys/secret_id_key
touch /tmp/tor/keys/secret_id_key
rm /tmp/tor/keys/secret_onion_key
touch /tmp/tor/keys/secret_onion_key
rm /tmp/tor/keys/secret_onion_key_ntor
touch /tmp/tor/keys/secret_onion_key_ntor
src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345

Ensure previous behaviour: no key file -> regenerate

src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
^C # when the keys have been generated
ls /tmp/tor/keys/
rm /tmp/tor/keys/secret_id_key
rm /tmp/tor/keys/secret_onion_key
rm /tmp/tor/keys/secret_onion_key_ntor
src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345

comment:6 Changed 3 years ago by nickm

Quick idea for simplification: What if we made a new return value for file_status() called FN_EMPTY ? Would that make the code simpler on the whole, or more complex?

comment:7 Changed 3 years ago by teor

Actually, I had the same idea, and was halfway through the change using FN_EMPTY, when I realised that it would complicate any existing code that used FN_FILE and expected empty files to be treated as normal files.

I had a list of these locations, but I cut them from my last comment for length. They were: torrc, replace_file, and rotate_onion_key. The code ended up being significantly more complicated, and it seemed to me that I was overloading "file type" with "file size".

comment:8 Changed 3 years ago by teor

Keywords: easy removed

Nick, I've created a git branch using enum FN_EMPTY in file_status() as you suggested, rather than my original strategy of creating a new file_size() function:
Branch: bug13111-empty-key-files-fn-empty
Repository: ​​​https://github.com/teor2345/tor.git

This avoids cluttering the API, and eliminates a bug in the previous branch, where a FIFO with no data to read would be treated like an empty file (when, really, only regular empty files should be treated as empty).

And despite my expectations, the implementation is cleaner and much less error-prone. Developers will have to explicitly handle the empty file case from now on, making the code clearer, too.

Bug Fixes:

  • empty RSA & curve25519 key files - overwrite empty key files rather than failing to start tor
  • empty RSA & curve25519 key files - don't overwrite a .old key file with an empty file
  • missing RSA .old key file - stop generating a fresh .old RSA key file
  • stop crashing when a NULL filename is passed to file_status() - return FN_ERROR instead

Improved Performance: (slightly?)

  • empty stats file while reading in extrainfo for router descriptor - skip reading file
  • empty router / extra info store files - skip reload
  • empty state file - skip load
  • empty key files - skip load
  • zero-length filename passed to file_status() - return FN_ERROR immediately

Unit Tests:
All unit tests pass.

Coverage:
After running make check, make test, benchmarks, and chutney --flavour bridges+ipv6, the modified file_status() function has been run 471 times.

Testing:

Check desired behaviour: zero-length key file -> regenerate

src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
^C # when the keys have been generated
ls -lh  /tmp/tor/keys/
rm /tmp/tor/keys/secret_id_key
touch /tmp/tor/keys/secret_id_key
rm /tmp/tor/keys/secret_onion_key
touch /tmp/tor/keys/secret_onion_key
rm /tmp/tor/keys/secret_onion_key_ntor
touch /tmp/tor/keys/secret_onion_key_ntor
src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
^C # when the keys have been regenerated
ls -lh /tmp/tor/keys/

Ensure previous behaviour: no key file -> regenerate

src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
^C # when the keys have been generated
ls -lh /tmp/tor/keys/
rm /tmp/tor/keys/secret_id_key
rm /tmp/tor/keys/secret_onion_key
rm /tmp/tor/keys/secret_onion_key_ntor
src/or/tor --ShutdownWaitLength 0 --DataDirectory /tmp/tor --ORPort 12345
ls -lh /tmp/tor/keys/

Nick, should I incorporate these tests into the unit tests? Or maybe the extra (python-based) tests?
If so, how can I do that, and which file should I edit?

comment:9 Changed 3 years ago by nickm

Excellent!

To add new python-based unit tests, write a .py program in src/test that does them, and then add it to the list of tests in src/test/include.am.

comment:10 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

On review: The code looks okay. Just let me know when the tests are ready.

comment:11 Changed 3 years ago by teor

nickm, how do I stop tor after it has created keys, but before it connects to the network?
My original tests included a manual ^C at approximately the right time, which doesn't quite work in python.

comment:12 Changed 3 years ago by teor

If dirserv_read_guardfraction_file can handle reading an empty file, and we merge #13111 with #9321 in 0.2.6, this code in #9321:

+      if (file_status(options->GuardfractionFile) != FN_FILE) {
+        REJECT("GuardfractionFile set but not a file? Failing");
+      }
+
+      dirserv_read_guardfraction_file(options->GuardfractionFile, NULL);

will need to be updated to:

+      if (file_status(options->GuardfractionFile) != FN_FILE && file_status(options->GuardfractionFile) != FN_EMPTY) {
+        REJECT("GuardfractionFile set but not a file? Failing");
+      }
+
+      dirserv_read_guardfraction_file(options->GuardfractionFile, NULL);
Last edited 3 years ago by teor (previous) (diff)

comment:13 in reply to:  11 Changed 3 years ago by nickm

Replying to teor:

nickm, how do I stop tor after it has created keys, but before it connects to the network?
My original tests included a manual ^C at approximately the right time, which doesn't quite work in python.

The "disablenetwork" option should keep tor from making any network connections. Would that suit your needs?

comment:14 Changed 3 years ago by teor

Any particular reason it needs to be a python script rather than a shell script?
(Apart from the fact that some people really don't like arbitrary shell scripts.)

comment:15 Changed 3 years ago by teor

Also, nickm, I can't seem to get tor to exit after it has generated the keys, it just says:

Jan 08 02:00:03.000 [notice] Your Tor server's identity key fingerprint is 'Unnamed 0178F5D27DF366AD7BCFA6C3B8C77F5C9D5B00A7'
Jan 08 02:00:03.000 [notice] Bootstrapped 0%: Starting
Jan 08 02:00:03.000 [notice] Delaying directory fetches: DisableNetwork is set.

I want it to die after the first message, without having to do anything risky like:

(sleep 5; cat /tmp/tor.$$/pid | xargs kill;)
tor <arguments>

comment:16 Changed 3 years ago by teor

I have attached my first attempt at the behaviour I want in sh (well, probably bash). I'll need some help translating the trickier bits into python.

Or should I just use the control connection to shut tor down?
(How do I do that in python?)

Changed 3 years ago by teor

Attachment: zero_length_keys.sh added

Test #13111 behaviour, use "-d" for deleted files, default is zero-length files

comment:17 in reply to:  14 Changed 3 years ago by nickm

Replying to teor:

Any particular reason it needs to be a python script rather than a shell script?
(Apart from the fact that some people really don't like arbitrary shell scripts.)

Not really. Windows testing with shell isn't convenient, but that's about it. shell would be okay.

To avoid the sleep; kill how about starting Tor in the background, and using $! to get its pid?

Notes:

  • you should put $DATA_DIR in quotes wherever you use it.


comment:18 Changed 3 years ago by teor

Cc: nickm added
Keywords: lorax removed
Status: needs_revisionneeds_review

Nick, I've updated the existing branch to include the following tests:

  • check tor launches and overwrites empty files with new keys (bug fix)
  • check tor creates new keys for missing files (existing behaviour)
  • check tor doesn't overwrite existing, non-empty keys (existing behaviour)

This is included in make check-local

I've also merged the latest git, including merging a single line which was changed in this branch and in the latest git (origin added an argument, this branch changed an argument and added a comment). Will this need a rebase?

Branch: bug13111-empty-key-files-fn-empty
Repository: ​​​​https://github.com/teor2345/tor.git

(However, I'm still not ready for new work yet. I still have to do #14067 + #13401.)

comment:19 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged it; thanks!

comment:20 Changed 3 years ago by teor

Resolution: fixed
Status: closedreopened

weasel reported the following issues in #tor-dev on 12 Jan 2015:

[22:35] <weasel> nickm: that zero length keys thing broke running make check with out-of-tree builds.
[22:37] <weasel> that script also appears to be broken because it uses a fixed orport, so if you build many things at once it will blow up.
[23:47] <teor> weasel: It never actually needs the ORPort, so we can just use --DisableNetwork
[23:50] <teor> weasel: Not sure what out-of-tree builds are or how to fix that issue - can you elaborate?

comment:21 Changed 3 years ago by teor

[23:54] <teor> weasel: it already uses --DisableNetwork, so there will never be any port conflict.
[23:54] <teor> Adding a comment to that effect

Nick, what is an out-of-tree build, and how can I fix the issue?
Is it to do with the assumption that the CWD will always be tor?

comment:22 Changed 3 years ago by teor

Status: reopenedneeds_review

OK, fixed to use a tor path relative to the script itself. This should support out-of-tree builds.

Branch: bug13111-empty-key-files-fn-empty
Repository: ​​​​​https://github.com/teor2345/tor.git

comment:23 Changed 3 years ago by nickm

I think this is backwards; we want to take the path to the script with respect to the source dir.

See 038804e13dbed8160e41e57

comment:24 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

comment:25 Changed 3 years ago by teor

FWIW, the unit tests for this code assume that tor will produce different keys every time it regenerates them. If our random source is badly broken (e.g. returns all 0s), the tests will fail.

This is a feature, not a bug :-)

Note: See TracTickets for help on using tickets.