Opened 4 years ago

Closed 4 years ago

#17443 closed defect (fixed)

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

Reported by: junglefowl Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.4-rc
Severity: Normal Keywords: crash
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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));

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 4 years ago by cypherpunks

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.

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:3 Changed 4 years ago by teor

Keywords: security 027-backport 026-backport added
Status: newneeds_review

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

comment:4 Changed 4 years ago by nickm

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.

comment:5 Changed 4 years ago by 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?

comment:6 Changed 4 years ago by teor

Keywords: crash added; security 027-backport 026-backport removed

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?

comment:7 in reply to:  5 ; Changed 4 years ago by cypherpunks

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 or is that for another ticket?

comment:8 in reply to:  7 ; Changed 4 years ago by nickm

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.

comment:9 Changed 4 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

comment:10 Changed 4 years ago by nickm

Status: acceptedneeds_review

comment:11 in reply to:  8 Changed 4 years ago by cypherpunks

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?

comment:12 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.