tor-gencert --passphrase-fd improperly checks for newline

How to trigger:

$ tor-gencert --create-identity-key --passphrase-fd 0 < /dev/null

It depends on your system what will happen now: assert, not enough memory, or nothing.

This patch properly checks if memchr call returns NULL:

 src/tools/tor-gencert.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/tools/tor-gencert.c b/src/tools/tor-gencert.c
index e833aa9..d4c8c0d 100644
--- a/src/tools/tor-gencert.c
+++ b/src/tools/tor-gencert.c
@@ -103,6 +103,10 @@ load_passphrase(void)
     return -1;
   }
   cp = memchr(buf, '\n', n);
+  if (cp == NULL) {
+    log_err(LD_GENERAL, "Couldn't read from passphrase fd: missing newline");
+    return -1;
+  }
   passphrase_len = cp-buf;
   passphrase = tor_strndup(buf, passphrase_len);
   memwipe(buf, 0, sizeof(buf));

---

Trac:
Username: junglefowl

changed milestone to %Tor: 0.2.8.x-final

added component::core tor/tor crash milestone::Tor: 0.2.8.x-final owner::nickm priority::medium reporter::junglefowl resolution::fixed severity::normal status::closed type::defect version::tor 0.2.7.4-rc labels

Trac:
Milestone: N/A to Tor: 0.2.8.x-final

I had this issue happen to me which resulted in a failed assertion. I created a similar fix before finding this ticket (and patch). Before creating my patch, i read the manual of tor-gencert which states that it stops reading when it encounters a NUL or a newline.

IMO this is unnecessarily limiting. What if a user wants to use output from /dev/urandom as their passphrase? It would depend on the output of /dev/urandom how much of the data would be used as a passphrase. In the worst case the first character is a NUL or a newline byte.

A better solution would be to read until EOF is encountered or the buffer limit is reached (currently at 1024 bytes). This would make the memchr call obsolete and would simplify the code.

Also buf is uninitialized so when nothing is read on stdin, memchr reads into uninitialized memory.

Lastly, how much is tor-gencert used (in comparison to tor --keygen)? Is it worth it to write tests for it? After reading both manuals i see they have different use cases so i guess it's test worthy.

Marking as security / backport because the tor_strndup call could read beyond buf if cp is NULL.

Trac:
Status: new to needs_review
Keywords: N/A deleted, security 027-backport 026-backport added

I don't see how this is security unless the user is sending unauthenticated data as their passphrase? I think it should be fine to just get it in 0.2.8.

I've done a slightly different fix as branch bug17443 in my public repository at https://gitweb.torproject.org/nickm/tor.git/ . Please review?

Oops, forgot to account for the source of the input data. It's not security, just a crash.

Looks good, but (and this may be a different ticket), what if we're running tor-gencert on Windows? If we strip off the '\n', do we get the '\r' as well?

Trac:
Keywords: security 027-backport 026-backport deleted, crash added

Replying to nickm:

I've done a slightly different fix as branch bug17443 in my public repository at https://gitweb.torproject.org/nickm/tor.git/ . Please review?

IMO it would be better, with regards to readability, if the passphrase length was explicitly set to zero if no newline was found and do the pointer subtraction otherwise. Furthermore, buf is still uninitialized. Please initialize it to prevent future problems.

A minor nitpick is the typo in the commit message (//Hnadle// instead of //Handle//).

What about the argument against limiting the passphrase as made in [comment:2 comment 2] or is that for another ticket?

Not setting the length to zero; instead taking everything to the EOF.

Initializing the buffer to zero.

New version in branch 'bug17443_v2' ; how's that one?

What about the argument against limiting the passphrase as made in comment 2 or is that for another ticket?

A few observations there:

• Changing the semantics of tor-gencert could make existing scripts start acting differently.
• The current semantics were meant for approximate compatibility with the semantics of the case where passphrase-fd wasn't given.
• Nobody runs tor-gencert on windows; it's only for authorities.

Trac:
Owner: N/A to nickm
Status: needs_review to accepted

Trac:
Status: accepted to needs_review

Replying to nickm:

Initializing the buffer to zero. This could be simplified to char buf[1024] = {0};.

What about the argument against limiting the passphrase as made in comment 2 or is that for another ticket?

A few observations there:

• Changing the semantics of tor-gencert could make existing scripts start acting differently. Makes sense, reading until EOF is good enough and shouldn't break anything.
• The current semantics were meant for approximate compatibility with the semantics of the case where passphrase-fd wasn't given. A custom password callback function can be passed to the OpenSSL functions which could be reused for the passphrase-fd case. This would guarantee equal semantics. Maybe for another ticket?
• Nobody runs tor-gencert on windows; it's only for authorities. Wouldn't it therefore be important to have some test coverage on this tool?

Going to merge this as an improvement; please feel free to open tickets for related issues noted above.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed
Reviewer: N/A to N/A

closed

moved to tpo/core/tor#17443 (closed)